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

On Thursday, January 23, 2025 4:43 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Thu, 23 Jan 2025 at 12:35, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Wed, 22 Jan 2025 at 09:00, Zhijie Hou (Fujitsu)
> > <houzj(dot)fnst(at)fujitsu(dot)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.

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.

Best Regards,
Hou zj