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

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