Re: create subscription with (origin = none, copy_data = on) - Mailing list pgsql-hackers

24.01.2025 07:22, Shlok Kyal пишет:
> On Thu, 23 Jan 2025 at 17:54, Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
>> On Thursday, January 23, 2025 4:43 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>>> On Thu, 23 Jan 2025 at 12:35, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>>>> On Wed, 22 Jan 2025 at 09:00, Zhijie Hou (Fujitsu)
>>>> <houzj.fnst@fujitsu.com> wrote:
>>>>> Thanks for the patch.
>>>>>
>>>>> I agree that covering the partitioned table case when checking the
>>>>> non-local origin data on publisher is an improvement. But I think
>>>>> adding or extending the SQL functions may not be the appropriate way
>>>>> to fix because the new functions cannot be used in older PG version and is
>>> also not backpatchable.
>>>>> I am thinking it would be better to use the existing
>>>>> pg_partition_ancestors() and pg_partition_tree() to verify the same,
>>>>> which can be used in all supported PG versions and is also backpatchable.
>>>>>
>>>>> And here is another version which fixed the issue like that. I have
>>>>> not added tests for it, but I think it's doable to write the
>>>>> something like the testcases provided by Sergey. This patch does not
>>>>> fix the foreign tabel as that seems to be a separate issue which can be fixed
>>> independtly.
>>>>> Hi Sergey, if you have the time, could you please verify whether
>>>>> this patch resolves the partition issue you reported? I've confirmed
>>>>> that it passes the partitioned tests in the scripts, but I would
>>>>> appreciate your confirmation for the same.
>>>> Hi Hou-san,
>>>>
>>>> I tested the patch, and it is working fine on HEAD.
>>>> I also tried to apply the patches to back branches PG17 and PG 16. But
>>>> the patch does not apply.
>>>>
>>>> This 'origin' option was added in PG 16. So, this patch will not be
>>>> required for PG 15 and back branches.
>>>>
>>> I have created a patch which applies to both PG17 and PG 16. The
>>> v6-0002 is the test patch. It applies to all the branches (HEAD, PG17,
>>> PG16) correctly.
>> Thanks for the patch. I think the testcases could be improved.
>>
>> It's not clear why a separate schema is created for tables. I assume it was
>> initially intended to test TABLES IN SCHEMA but was later modified. If the
>> separate schema is still necessary, could you please add comments to clarify
>> its purpose?
>> Besides, the new table name 'ts' seems a bit unconventional. It would be better
>> to align with the naming style of existing test cases for consistency and
>> clarity.
> I think the schema is not required. I have removed it.
>
>> Also, Sergey had suggested adding an more test to verify scenarios where the
>> table's ancestors are subscribed. It appears this hasn't been added yet. I
>> think it would be better to add it.
> I have added the test in the latest patch.
>
> Thanks and Regards,
> Shlok Kyal

Hello!

I think there is another problem - publisher can modify publication 
during the execution of "CREATE SUBSCRIPTION" (Rarely, this situation 
occurred in my tests.)

I.e.:

1. Publisher: CREATE PUBLICATION ... FOR TABLE t1;

2. Subscriber (starts CREATE SUBSCRIPTION ...): check_publications_origin()

3. Publisher: ALTER PUBLICATION ... ADD TABLE t2;

4. Subscriber (still process CREATE SUBSCRIPTION ...): fetch_table_list()

So, we check publication with only t1, but fetch t1,t2

I think we must start transaction on publisher while executing 
CreateSubscription() on subscriber (Or may be take an lock on publisher )

Thoughts?