Horton

Thread

Commits

  1. Fix a WARNING for data origin discrepancies.

  1. create subscription with (origin = none, copy_data = on)

    Sergey Tatarintsev <s.tatarintsev@postgrespro.ru> — 2025-01-17T08:30:04Z

    Hi, hackers!

I am looking at subscription creation command:

CREATE SUBSCRIPTION sub CONNECTION '...' PUBLICATION pub WITH (origin = 
none, copy_data = on);

For now we log a warning if the publisher has subscribed to the same 
table from some other publisher.
However, in case of publication with publish_via_partition_root option, 
we will not raise such warinigs
because SQL command in check_publications_origin() checks only directly 
published tables.
For example:

CREATE TABLE t(id int) PARTITION BY RANGE(id);
CREATE TABLE part1 PARTITION OF t FOR VALUES FROM (0) TO (5);
CREATE TABLE part2 PARTITION OF t FOR VALUES FROM (5) TO (10);
-- subscribe to part2
CREATE SUBSCRIPTION sub_part2 CONNECTION '...' PUBLICATION pub_part2;
CREATE PUBLICATION pub_t FOR TABLE t;
CREATE PUBLICATION pub_t_via_root FOR TABLE t WITH 
(publish_via_partition_root);

and now this command will raise a warning:
CREATE SUBSCRIPTION sub1 CONNECTION '...' PUBLICATION pub_t WITH (origin 
= none, copy_data = on);

but not this:
CREATE SUBSCRIPTION sub1 CONNECTION '...' PUBLICATION pub_t_via_root 
WITH (origin = none, copy_data = on);

We also do not take into account cases of foreign partitions:
CREATE TABLE t(id int) PARTITION BY RANGE(id);
CREATE TABLE part1 PARTITION OF t FOR VALUES FROM (0) TO (5);
CREATE FOREIGN TABLE part2 PARTITION OF t FOR VALUES FROM (5) TO (10) 
SERVER fdw_server;
CREATE PUBLICATION pub_t FOR TABLE t;

Maybe we should raise WARNING (or even ERROR) in such cases?
I would also note that the (origin = none) will work as expected, but in 
case of (origin = any)
it will lead to inappropriate behavior - we will perform an initial sync 
of "t", but we unable to
replicate further updates for "part2".

I have attached patch, demonstrating this problems

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

    vignesh C <vignesh21@gmail.com> — 2025-01-17T16:00:42Z

    On Fri, 17 Jan 2025 at 14:00, Sergey Tatarintsev
<s.tatarintsev@postgrespro.ru> wrote:
>
> Hi, hackers!
>
> I am looking at subscription creation command:
>
> CREATE SUBSCRIPTION sub CONNECTION '...' PUBLICATION pub WITH (origin =
> none, copy_data = on);
>
> For now we log a warning if the publisher has subscribed to the same
> table from some other publisher.
> However, in case of publication with publish_via_partition_root option,
> we will not raise such warinigs
> because SQL command in check_publications_origin() checks only directly
> published tables.

Yes, I agree that we are checking only the directly published tables
which is why there is no warning in this case. I'm working on a fix to
change the check_publications_origin to check accordingly.

> For example:
>
> CREATE TABLE t(id int) PARTITION BY RANGE(id);
> CREATE TABLE part1 PARTITION OF t FOR VALUES FROM (0) TO (5);
> CREATE TABLE part2 PARTITION OF t FOR VALUES FROM (5) TO (10);
> -- subscribe to part2
> CREATE SUBSCRIPTION sub_part2 CONNECTION '...' PUBLICATION pub_part2;
> CREATE PUBLICATION pub_t FOR TABLE t;
> CREATE PUBLICATION pub_t_via_root FOR TABLE t WITH
> (publish_via_partition_root);
>
> and now this command will raise a warning:
> CREATE SUBSCRIPTION sub1 CONNECTION '...' PUBLICATION pub_t WITH (origin
> = none, copy_data = on);
>
> but not this:
> CREATE SUBSCRIPTION sub1 CONNECTION '...' PUBLICATION pub_t_via_root
> WITH (origin = none, copy_data = on);
>
> We also do not take into account cases of foreign partitions:
> CREATE TABLE t(id int) PARTITION BY RANGE(id);
> CREATE TABLE part1 PARTITION OF t FOR VALUES FROM (0) TO (5);
> CREATE FOREIGN TABLE part2 PARTITION OF t FOR VALUES FROM (5) TO (10)
> SERVER fdw_server;
> CREATE PUBLICATION pub_t FOR TABLE t;
>
> Maybe we should raise WARNING (or even ERROR) in such cases?

Currently we do not support replication of foreign tables. This is
mentioned in logical replication restriction sections at [1].

> I would also note that the (origin = none) will work as expected, but in
> case of (origin = any)
> it will lead to inappropriate behavior - we will perform an initial sync
> of "t", but we unable to
> replicate further updates for "part2".

I noticed the same behavior with both origins as none and any. i.e
initial sync is ok and then replication of foreign table part2 will
not work which is because of the above restriction that I mentioned.
Just to be sure that I'm not checking a different scenario, could you
share the test for this case.

[1] - https://www.postgresql.org/docs/devel/logical-replication-restrictions.html

Regards,
Vignesh

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

    Sergey Tatarintsev <s.tatarintsev@postgrespro.ru> — 2025-01-18T03:08:43Z

    17.01.2025 23:00, vignesh C пишет:
> On Fri, 17 Jan 2025 at 14:00, Sergey Tatarintsev
> <s.tatarintsev@postgrespro.ru> wrote:
>> Hi, hackers!
>>
>> I am looking at subscription creation command:
>>
>> CREATE SUBSCRIPTION sub CONNECTION '...' PUBLICATION pub WITH (origin =
>> none, copy_data = on);
>>
>> For now we log a warning if the publisher has subscribed to the same
>> table from some other publisher.
>> However, in case of publication with publish_via_partition_root option,
>> we will not raise such warinigs
>> because SQL command in check_publications_origin() checks only directly
>> published tables.
> Yes, I agree that we are checking only the directly published tables
> which is why there is no warning in this case. I'm working on a fix to
> change the check_publications_origin to check accordingly.
seems promising. I would like to see this patch
>> For example:
>>
>> CREATE TABLE t(id int) PARTITION BY RANGE(id);
>> CREATE TABLE part1 PARTITION OF t FOR VALUES FROM (0) TO (5);
>> CREATE TABLE part2 PARTITION OF t FOR VALUES FROM (5) TO (10);
>> -- subscribe to part2
>> CREATE SUBSCRIPTION sub_part2 CONNECTION '...' PUBLICATION pub_part2;
>> CREATE PUBLICATION pub_t FOR TABLE t;
>> CREATE PUBLICATION pub_t_via_root FOR TABLE t WITH
>> (publish_via_partition_root);
>>
>> and now this command will raise a warning:
>> CREATE SUBSCRIPTION sub1 CONNECTION '...' PUBLICATION pub_t WITH (origin
>> = none, copy_data = on);
>>
>> but not this:
>> CREATE SUBSCRIPTION sub1 CONNECTION '...' PUBLICATION pub_t_via_root
>> WITH (origin = none, copy_data = on);
>>
>> We also do not take into account cases of foreign partitions:
>> CREATE TABLE t(id int) PARTITION BY RANGE(id);
>> CREATE TABLE part1 PARTITION OF t FOR VALUES FROM (0) TO (5);
>> CREATE FOREIGN TABLE part2 PARTITION OF t FOR VALUES FROM (5) TO (10)
>> SERVER fdw_server;
>> CREATE PUBLICATION pub_t FOR TABLE t;
>>
>> Maybe we should raise WARNING (or even ERROR) in such cases?
> Currently we do not support replication of foreign tables. This is
> mentioned in logical replication restriction sections at [1].
Yes of course, but we must raise an ERROR in such cases
>> I would also note that the (origin = none) will work as expected, but in
>> case of (origin = any)
>> it will lead to inappropriate behavior - we will perform an initial sync
>> of "t", but we unable to
>> replicate further updates for "part2".
> I noticed the same behavior with both origins as none and any. i.e
> initial sync is ok and then replication of foreign table part2 will
> not work which is because of the above restriction that I mentioned.
> Just to be sure that I'm not checking a different scenario, could you
> share the test for this case.

That's right, but i think we must tell user about inappropriate usage.

check_publication_add_relation() checks only publication creation for 
foreign tables directly, but not partitioned tables structure.

My test case just shows that we can try to replicate partitioned table 
with foreign partitions, but I think we should disallow such cases.


I would also like to show an interesting subscription creation scenario 
that I found:

1. subscriber: calls check_publications_origin()

2. publisher: executes the create/attach foreign partition command

3. the subscriber is sure he checked the origin and performing COPY t TO 
STDOUT

i.e. between the check and the start of copying the publication has changed

the problem is that check_publications_origin() and COPY t TO STDOUT are 
performed in different transactions


>
> [1] - https://www.postgresql.org/docs/devel/logical-replication-restrictions.html
>
> Regards,
> Vignesh

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

    vignesh C <vignesh21@gmail.com> — 2025-01-18T05:01:27Z

    On Fri, 17 Jan 2025 at 21:30, vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 17 Jan 2025 at 14:00, Sergey Tatarintsev
> <s.tatarintsev@postgrespro.ru> wrote:
> >
> > Hi, hackers!
> >
> > I am looking at subscription creation command:
> >
> > CREATE SUBSCRIPTION sub CONNECTION '...' PUBLICATION pub WITH (origin =
> > none, copy_data = on);
> >
> > For now we log a warning if the publisher has subscribed to the same
> > table from some other publisher.
> > However, in case of publication with publish_via_partition_root option,
> > we will not raise such warinigs
> > because SQL command in check_publications_origin() checks only directly
> > published tables.
>
> Yes, I agree that we are checking only the directly published tables
> which is why there is no warning in this case. I'm working on a fix to
> change the check_publications_origin to check accordingly.

Attached patch has the fix for this issue which includes the partition
tables also for the publication now and throws a warning
appropriately.

Regards,
Vignesh

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

    Sergey Tatarintsev <s.tatarintsev@postgrespro.ru> — 2025-01-18T08:59:17Z

    18.01.2025 12:01, vignesh C пишет:
> On Fri, 17 Jan 2025 at 21:30, vignesh C<vignesh21@gmail.com> wrote:
>> On Fri, 17 Jan 2025 at 14:00, Sergey Tatarintsev
>> <s.tatarintsev@postgrespro.ru> wrote:
>>> Hi, hackers!
>>>
>>> I am looking at subscription creation command:
>>>
>>> CREATE SUBSCRIPTION sub CONNECTION '...' PUBLICATION pub WITH (origin =
>>> none, copy_data = on);
>>>
>>> For now we log a warning if the publisher has subscribed to the same
>>> table from some other publisher.
>>> However, in case of publication with publish_via_partition_root option,
>>> we will not raise such warinigs
>>> because SQL command in check_publications_origin() checks only directly
>>> published tables.
>> Yes, I agree that we are checking only the directly published tables
>> which is why there is no warning in this case. I'm working on a fix to
>> change the check_publications_origin to check accordingly.
> Attached patch has the fix for this issue which includes the partition
> tables also for the publication now and throws a warning
> appropriately.
>
> Regards,
> Vignesh

Thanks for patch!

I think we must take into account whole inheritance tree of partitioned 
table.

For example:

node_A:
CREATE TABLE t(id int);
CREATE PUBLICATION pub_b FOR TABLE t;

node_A:
CREATE TABLE t(id int) PARTITION BY RANGE(id);
CREATE TABLE part PARTITION OF t FOR VALUES FROM (0) TO (10) PARTITION 
BY RANGE(id);
CREATE TABLE subpart PARTITION OF part FOR VALUES FROM (0) TO (5);
CREATE SUBSCRIPTION sub_c CONNECTION '$node_B_connstr' PUBLICATION pub_b;
CREATE PUBLICATION pub_t FOR TABLE t WITH (publish_via_partition_root);
CREATE PUBLICATION pub_part FOR TABLE part WITH 
(publish_via_partition_root);

node_C:
-- this command will raise a warning CREATE SUBSCRIPTION sub_t 
CONNECTION '$node_A_connstr' PUBLICATION pub_t WITH (origin = none, 
copy_data = on);
DROP SUBSCRIPTION IF EXISTS sub_t;
-- here we got silence, but "part" is in tree of upper level replicated 
table
CREATE SUBSCRIPTION sub_part CONNECTION '$node_A_connstr' PUBLICATION 
pub_part WITH (origin = none, copy_data = on);
DROP SUBSCRIPTION IF EXISTS sub_part;

I think that for each partition/partitioned table in the publication we 
can use something like

select relid from pg_partition_tree('part'::regclass)
union
select relid from pg_partition_ancestors('part'::regclass);

In this case  we don't care about  publish_via_partition_root option, 
because we already check all inheritance tree, and there is no need to 
change pg_class

What are you thinking about it?

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

    Amit Kapila <amit.kapila16@gmail.com> — 2025-01-20T12:01:22Z

    On Sat, Jan 18, 2025 at 10:31 AM vignesh C <vignesh21@gmail.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.

*
@@ -1147,10 +1151,12 @@ pg_get_publication_tables(PG_FUNCTION_ARGS)
     *schemarelids;

  relids = GetPublicationRelations(pub_elem->oid,
+ allparttables ? PUBLICATION_PART_ALL :
  pub_elem->pubviaroot ?
  PUBLICATION_PART_ROOT :
  PUBLICATION_PART_LEAF);
  schemarelids = GetAllSchemaPublicationRelations(pub_elem->oid,
+ allparttables ? PUBLICATION_PART_ALL :
  pub_elem->pubviaroot ?
  PUBLICATION_PART_ROOT :
  PUBLICATION_PART_LEAF);

Don't we need to add similar handling FOR ALL TABLES case? If not, why?

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.

[1] - https://www.postgresql.org/docs/devel/sql-createsubscription.html#SQL-CREATESUBSCRIPTION-NOTES

-- 
With Regards,
Amit Kapila.

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

    vignesh C <vignesh21@gmail.com> — 2025-01-20T12:44:40Z

    On Sat, 18 Jan 2025 at 14:29, Sergey Tatarintsev
<s.tatarintsev@postgrespro.ru> wrote:
>
> I think we must take into account whole inheritance tree of partitioned table.
>
> For example:
>
> node_A:
> CREATE TABLE t(id int);
> CREATE PUBLICATION pub_b FOR TABLE t;
>
> node_A:
> CREATE TABLE t(id int) PARTITION BY RANGE(id);
> CREATE TABLE part PARTITION OF t FOR VALUES FROM (0) TO (10) PARTITION BY RANGE(id);
> CREATE TABLE subpart PARTITION OF part FOR VALUES FROM (0) TO (5);
> CREATE SUBSCRIPTION sub_c CONNECTION '$node_B_connstr' PUBLICATION pub_b;
> CREATE PUBLICATION pub_t FOR TABLE t WITH (publish_via_partition_root);
> CREATE PUBLICATION pub_part FOR TABLE part WITH (publish_via_partition_root);
>
> node_C:
> -- this command will raise a warning CREATE SUBSCRIPTION sub_t CONNECTION '$node_A_connstr' PUBLICATION pub_t WITH (origin = none, copy_data = on);
> DROP SUBSCRIPTION IF EXISTS sub_t;
> -- here we got silence, but "part" is in tree of upper level replicated table
> CREATE SUBSCRIPTION sub_part CONNECTION '$node_A_connstr' PUBLICATION pub_part WITH (origin = none, copy_data = on);
> DROP SUBSCRIPTION IF EXISTS sub_part;
>
> I think that for each partition/partitioned table in the publication we can use something like
>
> select relid from pg_partition_tree('part'::regclass)
> union
> select relid from pg_partition_ancestors('part'::regclass);
>
> In this case  we don't care about  publish_via_partition_root option, because we already check all inheritance tree, and there is no need to change pg_class
>
> What are you thinking about it?

Yes, we should include the ancestors of the table to handle the
scenario you mentioned. The attached patch has the changes which
includes the ancestors also while getting the tables for the table
publication and schema publication. And in case of all tables
publication, get all the tables.
Thoughts?

Regards,
Vignesh

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

    vignesh C <vignesh21@gmail.com> — 2025-01-20T17:30:45Z

    On Mon, 20 Jan 2025 at 17:31, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Jan 18, 2025 at 10:31 AM vignesh C <vignesh21@gmail.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.

Modified this too

> *
> @@ -1147,10 +1151,12 @@ pg_get_publication_tables(PG_FUNCTION_ARGS)
>      *schemarelids;
>
>   relids = GetPublicationRelations(pub_elem->oid,
> + allparttables ? PUBLICATION_PART_ALL :
>   pub_elem->pubviaroot ?
>   PUBLICATION_PART_ROOT :
>   PUBLICATION_PART_LEAF);
>   schemarelids = GetAllSchemaPublicationRelations(pub_elem->oid,
> + allparttables ? PUBLICATION_PART_ALL :
>   pub_elem->pubviaroot ?
>   PUBLICATION_PART_ROOT :
>   PUBLICATION_PART_LEAF);
>
> Don't we need to add similar handling FOR ALL TABLES case? If not, why?

Yes, it is required. Modified

> 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.

Regards,
Vignesh

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

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2025-01-22T03:30:15Z

    On Tuesday, January 21, 2025 1:31 AM vignesh C <vignesh21@gmail.com> wrote:

Hi,

> 
> On Mon, 20 Jan 2025 at 17:31, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sat, Jan 18, 2025 at 10:31 AM vignesh C <vignesh21@gmail.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.

Best Regards,
Hou zj

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

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-01-22T15:41:55Z

    On Wed, 22 Jan 2025 at 09:00, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Tuesday, January 21, 2025 1:31 AM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> >
> > On Mon, 20 Jan 2025 at 17:31, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Sat, Jan 18, 2025 at 10:31 AM vignesh C <vignesh21@gmail.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

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

    Sergey Tatarintsev <s.tatarintsev@postgrespro.ru> — 2025-01-22T16:13:57Z

    22.01.2025 18:41, Shlok Kyal пишет:
> On Wed, 22 Jan 2025 at 09:00, Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
>> On Tuesday, January 21, 2025 1:31 AM vignesh C <vignesh21@gmail.com> wrote:
>>
>> Hi,
>>
>>> On Mon, 20 Jan 2025 at 17:31, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>>> On Sat, Jan 18, 2025 at 10:31 AM vignesh C <vignesh21@gmail.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?

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

    Amit Kapila <amit.kapila16@gmail.com> — 2025-01-23T02:46:40Z

    On Wed, Jan 22, 2025 at 9:44 PM Sergey Tatarintsev
<s.tatarintsev@postgrespro.ru> wrote:
>
> 22.01.2025 18:41, Shlok Kyal пишет:
>
> Also we still don't care about foreign partitions  (as I wrote earlier
> we should raise an ERROR for such publications).
>

I think dealing with this separately from the origin vs. partitioned
table issue is better.

-- 
With Regards,
Amit Kapila.

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

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-01-23T07:05:46Z

    On Wed, 22 Jan 2025 at 09:00, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Tuesday, January 21, 2025 1:31 AM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> >
> > On Mon, 20 Jan 2025 at 17:31, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Sat, Jan 18, 2025 at 10:31 AM vignesh C <vignesh21@gmail.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 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.

Thanks and Regards,
Shlok Kyal

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

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-01-23T08:42:40Z

    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:
> >
> > On Tuesday, January 21, 2025 1:31 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Hi,
> >
> > >
> > > On Mon, 20 Jan 2025 at 17:31, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Sat, Jan 18, 2025 at 10:31 AM vignesh C <vignesh21@gmail.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 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 and Regards,
Shlok Kyal

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

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2025-01-23T12:24:50Z

    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.

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

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

    Sergey Tatarintsev <s.tatarintsev@postgrespro.ru> — 2025-01-23T13:03:12Z

    23.01.2025 15:24, Zhijie Hou (Fujitsu) пишет:
> 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.
>
> 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

Hi!

That's right, separate schema was used to test  "CREATE PUBLICATION FOR 
TABLES IN SCHEMA".

My first patch with test cases contains 3 scenarios:

CREATE PUBLICATION pub_a FOR TABLE ts.t;

CREATE PUBLICATION pub_a_via_root FOR TABLE ts.t WITH 
(publish_via_partition_root);

CREATE PUBLICATION pub_a_schema FOR TABLES IN SCHEMA ts WITH 
(publish_via_partition_root);

(but not scenario where the table's ancestors are subscribed)

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

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-01-24T04:22:41Z

    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

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

    Sergey Tatarintsev <s.tatarintsev@postgrespro.ru> — 2025-01-24T12:13:21Z

    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?

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

    Amit Kapila <amit.kapila16@gmail.com> — 2025-01-27T08:57:40Z

    On Fri, Jan 24, 2025 at 5:43 PM Sergey Tatarintsev
<s.tatarintsev@postgrespro.ru> wrote:
>
> 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 )
>

We don't want to make the code complex for this, especially in
back-branches as that can introduce more bugs. We already document
that "... it is the user's responsibility to make the necessary checks
to ensure the copied data origins are really as wanted or not.". We
should try to give this WARNING in possible scenarios without
complicating the code too much. Anyway, even after this WARNING it is
possible that the data is not copied from the publisher and there is
no danger of copying the wrong origin. So, I suggest leaving the above
case as it is.

-- 
With Regards,
Amit Kapila.

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

    Sergey Tatarintsev <s.tatarintsev@postgrespro.ru> — 2025-01-29T06:26:08Z

    23.01.2025 09:46, Amit Kapila пишет:
> On Wed, Jan 22, 2025 at 9:44 PM Sergey Tatarintsev
> <s.tatarintsev@postgrespro.ru> wrote:
>> 22.01.2025 18:41, Shlok Kyal пишет:
>>
>> Also we still don't care about foreign partitions  (as I wrote earlier
>> we should raise an ERROR for such publications).
>>
> I think dealing with this separately from the origin vs. partitioned
> table issue is better.
>
ok! let it be so
I made a patch for such cases. You can see it here:

https://www.postgresql.org/message-id/c78766fa-4eff-4805-ad9c-868f02954ad4%40postgrespro.ru

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

    vignesh C <vignesh21@gmail.com> — 2025-01-29T10:28:21Z

    On Fri, 24 Jan 2025 at 09:52, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> I have added the test in the latest patch.

Few comments:
1) Let's rearrange this query slightly so that the "PT.pubname IN
(&lt;pub-names&gt;)" appears at the end, the reason being that it will
be easy to copy/paste and edit it to include the publications names if
it is at the end:
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -534,13 +534,15 @@ CREATE SUBSCRIPTION <replaceable
class="parameter">subscription_name</replaceabl
 <programlisting>
 # substitute &lt;pub-names&gt; below with your publication name(s) to
be queried
 SELECT DISTINCT PT.schemaname, PT.tablename
-FROM pg_publication_tables PT,
+FROM pg_publication_tables PT
+     JOIN pg_class C ON (C.relname = PT.tablename)
+     JOIN pg_namespace N ON (N.nspname = PT.schemaname),
      pg_subscription_rel PS
-     JOIN pg_class C ON (C.oid = PS.srrelid)
-     JOIN pg_namespace N ON (N.oid = C.relnamespace)
-WHERE N.nspname = PT.schemaname AND
-      C.relname = PT.tablename AND
-      PT.pubname IN (&lt;pub-names&gt;);
+WHERE C.relnamespace = N.oid AND
+      PT.pubname IN (&lt;pub-names&gt;) AND
+      (PS.srrelid = C.oid OR
+      C.oid IN (SELECT relid FROM pg_partition_ancestors(PS.srrelid) UNION
+                SELECT relid FROM pg_partition_tree(PS.srrelid)));

2) The same should be handled in the PG17 version patch too.

3) Currently the setup is done like:
node_B(table tab_part2 - publication pub_b_a) replicating to
node_A(sub_a_b subscription)
node_A(table tab_main - publication pub_a_c) replicating to node_C(sub_a_c)

+###############################################################################
+# Specifying origin = NONE and copy_data = on must raise WARNING if
we subscribe
+# to a partitioned table and this table contains any remotely originated data.
+###############################################################################
+
+# create a partition table on node A
+$node_A->safe_psql(
+       'postgres', qq(
+CREATE TABLE tab_main(a int) PARTITION BY RANGE(a);
+CREATE TABLE tab_part1 PARTITION OF tab_main FOR VALUES FROM (0) TO (5);
+CREATE TABLE tab_part2(a int) PARTITION BY RANGE(a);
+CREATE TABLE tab_part2_1 PARTITION OF tab_part2 FOR VALUES FROM (5) TO (10);
+ALTER TABLE tab_main ATTACH PARTITION tab_part2 FOR VALUES FROM (5) to (10);
+));
+
+# create a table on node B which will act as a source for a partition on node A
+$node_B->safe_psql(
+       'postgres', qq(

Can we change this like below to make review easier:
node_A(table tab_part2 - publication pub_b_a) replicating to
node_B(sub_a_b subscription)
node_B(table tab_main - publication pub_a_c) replicating to node_C(sub_a_c)

Also add something similar like above to the comment.

Regards,
Vignesh

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

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-01-29T12:19:18Z

    On Wed, 29 Jan 2025 at 15:58, vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 24 Jan 2025 at 09:52, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > I have added the test in the latest patch.
>
> Few comments:
> 1) Let's rearrange this query slightly so that the "PT.pubname IN
> (&lt;pub-names&gt;)" appears at the end, the reason being that it will
> be easy to copy/paste and edit it to include the publications names if
> it is at the end:
> +++ b/doc/src/sgml/ref/create_subscription.sgml
> @@ -534,13 +534,15 @@ CREATE SUBSCRIPTION <replaceable
> class="parameter">subscription_name</replaceabl
>  <programlisting>
>  # substitute &lt;pub-names&gt; below with your publication name(s) to
> be queried
>  SELECT DISTINCT PT.schemaname, PT.tablename
> -FROM pg_publication_tables PT,
> +FROM pg_publication_tables PT
> +     JOIN pg_class C ON (C.relname = PT.tablename)
> +     JOIN pg_namespace N ON (N.nspname = PT.schemaname),
>       pg_subscription_rel PS
> -     JOIN pg_class C ON (C.oid = PS.srrelid)
> -     JOIN pg_namespace N ON (N.oid = C.relnamespace)
> -WHERE N.nspname = PT.schemaname AND
> -      C.relname = PT.tablename AND
> -      PT.pubname IN (&lt;pub-names&gt;);
> +WHERE C.relnamespace = N.oid AND
> +      PT.pubname IN (&lt;pub-names&gt;) AND
> +      (PS.srrelid = C.oid OR
> +      C.oid IN (SELECT relid FROM pg_partition_ancestors(PS.srrelid) UNION
> +                SELECT relid FROM pg_partition_tree(PS.srrelid)));
>
> 2) The same should be handled in the PG17 version patch too.
>
> 3) Currently the setup is done like:
> node_B(table tab_part2 - publication pub_b_a) replicating to
> node_A(sub_a_b subscription)
> node_A(table tab_main - publication pub_a_c) replicating to node_C(sub_a_c)
>
> +###############################################################################
> +# Specifying origin = NONE and copy_data = on must raise WARNING if
> we subscribe
> +# to a partitioned table and this table contains any remotely originated data.
> +###############################################################################
> +
> +# create a partition table on node A
> +$node_A->safe_psql(
> +       'postgres', qq(
> +CREATE TABLE tab_main(a int) PARTITION BY RANGE(a);
> +CREATE TABLE tab_part1 PARTITION OF tab_main FOR VALUES FROM (0) TO (5);
> +CREATE TABLE tab_part2(a int) PARTITION BY RANGE(a);
> +CREATE TABLE tab_part2_1 PARTITION OF tab_part2 FOR VALUES FROM (5) TO (10);
> +ALTER TABLE tab_main ATTACH PARTITION tab_part2 FOR VALUES FROM (5) to (10);
> +));
> +
> +# create a table on node B which will act as a source for a partition on node A
> +$node_B->safe_psql(
> +       'postgres', qq(
>
> Can we change this like below to make review easier:
> node_A(table tab_part2 - publication pub_b_a) replicating to
> node_B(sub_a_b subscription)
> node_B(table tab_main - publication pub_a_c) replicating to node_C(sub_a_c)
>
> Also add something similar like above to the comment.
>

I have addressed the comments. Here is an updated patch.


Thanks and Regards,
Shlok Kyal

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

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2025-02-20T11:02:45Z

    On Wednesday, January 29, 2025 8:19 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> 
> I have addressed the comments. Here is an updated patch.

Thanks for updating the patch. The patches look mostly OK to me, I only have
one minor comments in 0002.

1.

+CREATE PUBLICATION pub_b_c_2 FOR TABLE tab_part2_1;
+));
+
+($result, $stdout, $stderr) = $node_C->psql(
+	'postgres', "
+	CREATE SUBSCRIPTION sub_b_c CONNECTION '$node_B_connstr' PUBLICATION pub_b_c WITH (origin = none, copy_data = on);
+");

The naming style of new publications and subscriptions doesn't seem consistent
with existing ones in 030_origin.

Best Regards,
Hou zj

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

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-02-21T06:45:36Z

    On Thu, 20 Feb 2025 at 16:32, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Wednesday, January 29, 2025 8:19 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > I have addressed the comments. Here is an updated patch.
>
> Thanks for updating the patch. The patches look mostly OK to me, I only have
> one minor comments in 0002.
>
> 1.
>
> +CREATE PUBLICATION pub_b_c_2 FOR TABLE tab_part2_1;
> +));
> +
> +($result, $stdout, $stderr) = $node_C->psql(
> +       'postgres', "
> +       CREATE SUBSCRIPTION sub_b_c CONNECTION '$node_B_connstr' PUBLICATION pub_b_c WITH (origin = none, copy_data = on);
> +");
>
> The naming style of new publications and subscriptions doesn't seem consistent
> with existing ones in 030_origin.
>

I have addressed the comments and attached the updated v9 patch.
I have also combined the 0001 and 0002 patch and updated the commit
message as per off list discussion.

Thanks and Regards
Shlok Kyal