On Thu, 20 Feb 2025 at 16:32, Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
> On Wednesday, January 29, 2025 8:19 PM Shlok Kyal <shlok.kyal....@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
From 9ecf7c638c41c161da36b8862e26f42449d55641 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
Date: Tue, 21 Jan 2025 16:18:34 +0800
Subject: [PATCH v9] Fix a WARNING for data origin discrepancies.
Previously, a WARNING was issued at the time of defining a subscription
with origin=NONE only when the publisher subscribed to the same table from
other publishers, indicating potential data origination from different
origins. However, the publisher can subscribe to the partition ancestors
or partition children of the table from other publishers, which could also
result in mixed-origin data inclusion. So, give a WARNING in those cases
as well.
Reported-by: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
Author: Hou Zhijie <houzj.fnst@fujitsu.com>
Author: Shlok Kyal <shlok.kyal.oss@gmail.com>
Reviewed-by: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Backpatch-through: 16, where it was introduced
---
doc/src/sgml/ref/create_subscription.sgml | 12 +--
src/backend/commands/subscriptioncmds.c | 15 ++--
src/test/subscription/t/030_origin.pl | 104 ++++++++++++++++++++++
3 files changed, 120 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 6cf7d4f9a1a..57dec28a5df 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -534,12 +534,14 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
<programlisting>
# substitute <pub-names> 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
+WHERE C.relnamespace = N.oid 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))) AND
PT.pubname IN (<pub-names>);
</programlisting></para>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 2d8a71ca1e1..4aec73bcc6b 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2083,11 +2083,12 @@ AlterSubscriptionOwner_oid(Oid subid, Oid newOwnerId)
}
/*
- * Check and log a warning if the publisher has subscribed to the same table
- * from some other publisher. This check is required only if "copy_data = true"
- * and "origin = none" for CREATE SUBSCRIPTION and
- * ALTER SUBSCRIPTION ... REFRESH statements to notify the user that data
- * having origin might have been copied.
+ * Check and log a warning if the publisher has subscribed to the same table,
+ * its partition ancestors (if it's a partition), or its partition children (if
+ * it's a partitioned table), from some other publishers. This check is
+ * required only if "copy_data = true" and "origin = none" for CREATE
+ * SUBSCRIPTION and ALTER SUBSCRIPTION ... REFRESH statements to notify the
+ * user that data having origin might have been copied.
*
* This check need not be performed on the tables that are already added
* because incremental sync for those tables will happen through WAL and the
@@ -2117,7 +2118,9 @@ check_publications_origin(WalReceiverConn *wrconn, List *publications,
"SELECT DISTINCT P.pubname AS pubname\n"
"FROM pg_publication P,\n"
" LATERAL pg_get_publication_tables(P.pubname) GPT\n"
- " JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),\n"
+ " JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid OR"
+ " GPT.relid IN (SELECT relid FROM pg_partition_ancestors(PS.srrelid) UNION"
+ " SELECT relid FROM pg_partition_tree(PS.srrelid))),\n"
" pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)\n"
"WHERE C.oid = GPT.relid AND P.pubname IN (");
GetPublicationsStr(publications, &cmd, true);
diff --git a/src/test/subscription/t/030_origin.pl b/src/test/subscription/t/030_origin.pl
index eb1dcb72715..38c51a725b8 100644
--- a/src/test/subscription/t/030_origin.pl
+++ b/src/test/subscription/t/030_origin.pl
@@ -255,6 +255,110 @@ $node_A->safe_psql('postgres', "DROP TABLE tab_new");
$node_B->safe_psql('postgres', "DROP TABLE tab_new");
$node_A->safe_psql('postgres', "DROP SUBSCRIPTION $subname_AB2");
+###############################################################################
+# 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.
+#
+# node_B
+# __________________________
+# | tab_main | --------------> node_C (tab_main)
+# |__________________________|
+# | tab_part1 | tab_part2 | <-------------- node_A (tab_part2)
+# |____________|_____________|
+# | tab_part2_1 |
+# |_____________|
+#
+# node_B
+# __________________________
+# | tab_main |
+# |__________________________|
+# | tab_part1 | tab_part2 | <-------------- node_A (tab_part2)
+# |____________|_____________|
+# | tab_part2_1 | --------------> node_C (tab_part2_1)
+# |_____________|
+###############################################################################
+
+# create a table on node A which will act as a source for a partition on node B
+$node_A->safe_psql(
+ 'postgres', qq(
+CREATE TABLE tab_part2(a int);
+CREATE PUBLICATION tab_pub_A FOR TABLE tab_part2;
+));
+
+# create a partition table on node B
+$node_B->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 SUBSCRIPTION tab_sub_A_B CONNECTION '$node_A_connstr' PUBLICATION tab_pub_A;
+));
+
+# create a table on node C
+$node_C->safe_psql(
+ 'postgres', qq(
+CREATE TABLE tab_main(a int);
+CREATE TABLE tab_part2_1(a int);
+));
+
+# create a logical replication setup between node B and node C with
+# subscription on node C having origin = NONE and copy_data = on
+$node_B->safe_psql(
+ 'postgres', qq(
+CREATE PUBLICATION tab_pub_B FOR TABLE tab_main WITH (publish_via_partition_root);
+CREATE PUBLICATION tab_pub_B_2 FOR TABLE tab_part2_1;
+));
+
+($result, $stdout, $stderr) = $node_C->psql(
+ 'postgres', "
+ CREATE SUBSCRIPTION tab_sub_B_C CONNECTION '$node_B_connstr' PUBLICATION tab_pub_B WITH (origin = none, copy_data = on);
+");
+
+# A warning must be logged as a partition 'tab_part2' in node B is subscribed to
+# node A so partition 'tab_part2' can have remotely originated data
+like(
+ $stderr,
+ qr/WARNING: ( [A-Z0-9]+:)? subscription "tab_sub_b_c" requested copy_data with origin = NONE but might copy data that had a different origin/,
+ "Create subscription with origin = none and copy_data when the publisher's partition is subscribing from different origin"
+);
+$node_C->safe_psql('postgres', "DROP SUBSCRIPTION tab_sub_B_C");
+
+($result, $stdout, $stderr) = $node_C->psql(
+ 'postgres', "
+ CREATE SUBSCRIPTION tab_sub_B_C CONNECTION '$node_B_connstr' PUBLICATION tab_pub_B_2 WITH (origin = none, copy_data = on);
+");
+
+# A warning must be logged as ancestor of table 'tab_part2_1' in node B is
+# subscribed to node A so table 'tab_part2_1' can have remotely originated
+# data
+like(
+ $stderr,
+ qr/WARNING: ( [A-Z0-9]+:)? subscription "tab_sub_b_c" requested copy_data with origin = NONE but might copy data that had a different origin/,
+ "Create subscription with origin = none and copy_data when the publisher's ancestor is subscribing from different origin"
+);
+
+# clear the operations done by this test
+$node_C->safe_psql(
+ 'postgres', qq(
+DROP SUBSCRIPTION tab_sub_B_C;
+DROP TABLE tab_main;
+DROP TABLE tab_part2_1;
+));
+$node_B->safe_psql(
+ 'postgres', qq(
+DROP SUBSCRIPTION tab_sub_A_B;
+DROP PUBLICATION tab_pub_B;
+DROP PUBLICATION tab_pub_B_2;
+DROP TABLE tab_main;
+));
+$node_A->safe_psql(
+ 'postgres', qq(
+DROP PUBLICATION tab_pub_A;
+DROP TABLE tab_part2;
+));
+
# shutdown
$node_B->stop('fast');
$node_A->stop('fast');
--
2.41.0.windows.3
From b80b2e29b96fbb132991299825d6f0e2093dc66f Mon Sep 17 00:00:00 2001
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Thu, 23 Jan 2025 12:59:19 +0530
Subject: [PATCH v9_PG_17-PG_16] Fix a WARNING for data origin discrepancies.
Previously, a WARNING was issued at the time of defining a subscription
with origin=NONE only when the publisher subscribed to the same table from
other publishers, indicating potential data origination from different
origins. However, the publisher can subscribe to the partition ancestors
or partition children of the table from other publishers, which could also
result in mixed-origin data inclusion. So, give a WARNING in those cases
as well.
Reported-by: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
Author: Hou Zhijie <houzj.fnst@fujitsu.com>
Author: Shlok Kyal <shlok.kyal.oss@gmail.com>
Reviewed-by: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Backpatch-through: 16, where it was introduced
---
doc/src/sgml/ref/create_subscription.sgml | 12 +--
src/backend/commands/subscriptioncmds.c | 15 ++--
src/test/subscription/t/030_origin.pl | 104 ++++++++++++++++++++++
3 files changed, 120 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 740b7d94210..c9c8dd440dc 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -526,12 +526,14 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
<programlisting>
# substitute <pub-names> 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
+WHERE C.relnamespace = N.oid 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))) AND
PT.pubname IN (<pub-names>);
</programlisting></para>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8ecb6e0bb87..9467f58a23d 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2012,11 +2012,12 @@ AlterSubscriptionOwner_oid(Oid subid, Oid newOwnerId)
}
/*
- * Check and log a warning if the publisher has subscribed to the same table
- * from some other publisher. This check is required only if "copy_data = true"
- * and "origin = none" for CREATE SUBSCRIPTION and
- * ALTER SUBSCRIPTION ... REFRESH statements to notify the user that data
- * having origin might have been copied.
+ * Check and log a warning if the publisher has subscribed to the same table,
+ * its partition ancestors (if it's a partition), or its partition children (if
+ * it's a partitioned table), from some other publishers. This check is
+ * required only if "copy_data = true" and "origin = none" for CREATE
+ * SUBSCRIPTION and ALTER SUBSCRIPTION ... REFRESH statements to notify the
+ * user that data having origin might have been copied.
*
* This check need not be performed on the tables that are already added
* because incremental sync for those tables will happen through WAL and the
@@ -2046,7 +2047,9 @@ check_publications_origin(WalReceiverConn *wrconn, List *publications,
"SELECT DISTINCT P.pubname AS pubname\n"
"FROM pg_publication P,\n"
" LATERAL pg_get_publication_tables(P.pubname) GPT\n"
- " JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),\n"
+ " JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid OR"
+ " GPT.relid IN (SELECT relid FROM pg_partition_ancestors(PS.srrelid) UNION"
+ " SELECT relid FROM pg_partition_tree(PS.srrelid))),\n"
" pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)\n"
"WHERE C.oid = GPT.relid AND P.pubname IN (");
get_publications_str(publications, &cmd, true);
diff --git a/src/test/subscription/t/030_origin.pl b/src/test/subscription/t/030_origin.pl
index 056561f0084..e2e5b4db213 100644
--- a/src/test/subscription/t/030_origin.pl
+++ b/src/test/subscription/t/030_origin.pl
@@ -208,6 +208,110 @@ $node_A->safe_psql('postgres', "DROP TABLE tab_new");
$node_B->safe_psql('postgres', "DROP TABLE tab_new");
$node_A->safe_psql('postgres', "DROP SUBSCRIPTION $subname_AB2");
+###############################################################################
+# 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.
+#
+# node_B
+# __________________________
+# | tab_main | --------------> node_C (tab_main)
+# |__________________________|
+# | tab_part1 | tab_part2 | <-------------- node_A (tab_part2)
+# |____________|_____________|
+# | tab_part2_1 |
+# |_____________|
+#
+# node_B
+# __________________________
+# | tab_main |
+# |__________________________|
+# | tab_part1 | tab_part2 | <-------------- node_A (tab_part2)
+# |____________|_____________|
+# | tab_part2_1 | --------------> node_C (tab_part2_1)
+# |_____________|
+###############################################################################
+
+# create a table on node A which will act as a source for a partition on node B
+$node_A->safe_psql(
+ 'postgres', qq(
+CREATE TABLE tab_part2(a int);
+CREATE PUBLICATION tab_pub_A FOR TABLE tab_part2;
+));
+
+# create a partition table on node B
+$node_B->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 SUBSCRIPTION tab_sub_A_B CONNECTION '$node_A_connstr' PUBLICATION tab_pub_A;
+));
+
+# create a table on node C
+$node_C->safe_psql(
+ 'postgres', qq(
+CREATE TABLE tab_main(a int);
+CREATE TABLE tab_part2_1(a int);
+));
+
+# create a logical replication setup between node B and node C with
+# subscription on node C having origin = NONE and copy_data = on
+$node_B->safe_psql(
+ 'postgres', qq(
+CREATE PUBLICATION tab_pub_B FOR TABLE tab_main WITH (publish_via_partition_root);
+CREATE PUBLICATION tab_pub_B_2 FOR TABLE tab_part2_1;
+));
+
+($result, $stdout, $stderr) = $node_C->psql(
+ 'postgres', "
+ CREATE SUBSCRIPTION tab_sub_B_C CONNECTION '$node_B_connstr' PUBLICATION tab_pub_B WITH (origin = none, copy_data = on);
+");
+
+# A warning must be logged as a partition 'tab_part2' in node B is subscribed to
+# node A so partition 'tab_part2' can have remotely originated data
+like(
+ $stderr,
+ qr/WARNING: ( [A-Z0-9]+:)? subscription "tab_sub_b_c" requested copy_data with origin = NONE but might copy data that had a different origin/,
+ "Create subscription with origin = none and copy_data when the publisher's partition is subscribing from different origin"
+);
+$node_C->safe_psql('postgres', "DROP SUBSCRIPTION tab_sub_B_C");
+
+($result, $stdout, $stderr) = $node_C->psql(
+ 'postgres', "
+ CREATE SUBSCRIPTION tab_sub_B_C CONNECTION '$node_B_connstr' PUBLICATION tab_pub_B_2 WITH (origin = none, copy_data = on);
+");
+
+# A warning must be logged as ancestor of table 'tab_part2_1' in node B is
+# subscribed to node A so table 'tab_part2_1' can have remotely originated
+# data
+like(
+ $stderr,
+ qr/WARNING: ( [A-Z0-9]+:)? subscription "tab_sub_b_c" requested copy_data with origin = NONE but might copy data that had a different origin/,
+ "Create subscription with origin = none and copy_data when the publisher's ancestor is subscribing from different origin"
+);
+
+# clear the operations done by this test
+$node_C->safe_psql(
+ 'postgres', qq(
+DROP SUBSCRIPTION tab_sub_B_C;
+DROP TABLE tab_main;
+DROP TABLE tab_part2_1;
+));
+$node_B->safe_psql(
+ 'postgres', qq(
+DROP SUBSCRIPTION tab_sub_A_B;
+DROP PUBLICATION tab_pub_B;
+DROP PUBLICATION tab_pub_B_2;
+DROP TABLE tab_main;
+));
+$node_A->safe_psql(
+ 'postgres', qq(
+DROP PUBLICATION tab_pub_A;
+DROP TABLE tab_part2;
+));
+
# shutdown
$node_B->stop('fast');
$node_A->stop('fast');
--
2.41.0.windows.3
