Re: create subscription with (origin = none, copy_data = on)

22.01.2025 18:41, Shlok Kyal пишет:
> On Wed, 22 Jan 2025 at 09:00, Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>> On Tuesday, January 21, 2025 1:31 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>>
>> Hi,
>>
>>> On Mon, 20 Jan 2025 at 17:31, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>> On Sat, Jan 18, 2025 at 10:31 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>>>>> Attached patch has the fix for this issue which includes the
>>>>> partition tables also for the publication now and throws a warning
>>>>> appropriately.
>>>>>
>>>> The corresponding query (see "To find which tables might potentially
>>>> include non-local origins .." on [1]) on the create_subscription doc
>>>> page.
>>>> BTW, the proposed fix is not backpatcheable as it changes the catalog
>>>> which requires catversion bump. However, as this is a WARNING case, if
>>>> we can't find a fix that can't be backpatched, we can fix it in
>>>> HEAD-only.
>>> I could not find a way to fix the back version without changing the catalog
>>> version.
>>>
>>> The attached v3 version has the changes for the same.
>> 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 have created a patch to add a test for the patch.
>
> v5-0001 : same as v4-0001
> v5-0002: adds the testcase
>
> Thanks and Regards,
> Shlok Kyal

Hi!

Sorry, I can't do it right now, but I think we need add testcase where
ancestor of published table have different origin.

Also we still don't care about foreign partitions  (as I wrote earlier
we should raise an ERROR for such publications). This checking must be
done at publication creation in check_publication_add_relation(), but I
not sure about publication for all tables/for tables in schema because
one foreign table will block publication creation

Thoughts?