public inbox for [email protected]
help / color / mirror / Atom feedFrom: Amit Kapila <[email protected]>
To: Peter Smith <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Takamichi Osumi (Fujitsu) <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Amit Langote <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: Greg Nancarrow <[email protected]>
Subject: Re: Data is copied twice when specifying both child and parent table in publication
Date: Mon, 20 Mar 2023 15:44:49 +0530
Message-ID: <CAA4eK1Jpj2Yj5ZWwt4z-qM2fS0c3ZF3_xqAgor-Ziiru00LKHg@mail.gmail.com> (raw)
In-Reply-To: <CAHut+PuNsvO9o9XzeJuSLsAsndgCKVphDPBRqYuOTy2bR28E+g@mail.gmail.com>
References: <OS3PR01MB62751414E4DABF144F5E481C9E529@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<CAHut+PtTN1Udug3x1eZnDdh8Z6PL5VN=xz1b44VbB9T1z_PtcQ@mail.gmail.com>
<OS3PR01MB6275A9B8C65C381C6828DF9D9E549@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<CAHut+PvaSDuZ7PhzZOm4c2u-sbN4DUsZr22qyYQ8dncUpqMpXg@mail.gmail.com>
<OS3PR01MB6275843B2BBE92870F7881C19E299@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<CAHut+Pt+1PNx6VsZ-xKzAU-18HmNXhjCC1TGakKX46Wg7YNT1Q@mail.gmail.com>
<OS3PR01MB6275FB5397C6A647F262A3A69E009@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<CALDaNm3ZzNJ6Y1iX=PBYCcCU+EPm4=9LVXFin5gvQ9XkEzc8UQ@mail.gmail.com>
<OS3PR01MB6275C62FF1D1CDE4FCBF61339E079@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<[email protected]>
<OS3PR01MB62758DFA4FE5BFA7CA0815329ED89@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<CAA4eK1LCzvPLVHHXn+VcGmFHApKtbpaNF1UFed3qE8=GhPzPGw@mail.gmail.com>
<OSZPR01MB62783EDCE9DDC21D35B789209EBD9@OSZPR01MB6278.jpnprd01.prod.outlook.com>
<CAHut+PuNsvO9o9XzeJuSLsAsndgCKVphDPBRqYuOTy2bR28E+g@mail.gmail.com>
On Mon, Mar 20, 2023 at 1:02 PM Peter Smith <[email protected]> wrote:
>
>
> ======
> src/include/catalog/pg_proc.dat
>
> 4.
> +{ oid => '6119',
> + descr => 'get information of the tables in the given publication array',
>
> Should that be worded in a way to make it more clear that the
> "publication array" is really an "array of publication names"?
>
I don't know how important it is to tell that the array is an array of
publication names but the current description can be improved. How
about something like: "get information of the tables that are part of
the specified publications"
Few other comments:
=================
1.
foreach(lc2, ancestors)
{
Oid ancestor = lfirst_oid(lc2);
+ ListCell *lc3;
/* Check if the parent table exists in the published table list. */
- if (list_member_oid(relids, ancestor))
+ foreach(lc3, table_infos)
{
- skip = true;
- break;
+ Oid relid = ((published_rel *) lfirst(lc3))->relid;
+
+ if (relid == ancestor)
+ {
+ skip = true;
+ break;
+ }
}
+
+ if (skip)
+ break;
}
- if (!skip)
- result = lappend_oid(result, relid);
+ if (skip)
+ table_infos = foreach_delete_current(table_infos, lc);
The usage of skip looks a bit ugly to me. Can we move the code for the
inner loop to a separate function (like
is_ancestor_member_tableinfos()) and remove the current cell if it
returns true?
2.
* Filter out the partitions whose parent tables were also specified in
* the publication.
*/
-static List *
-filter_partitions(List *relids)
+static void
+filter_partitions(List *table_infos)
The comment atop filter_partitions is no longer valid. Can we slightly
change it to: "Filter out the partitions whose parent tables are also
present in the list."?
3.
-# Note: We create two separate tables, not a partitioned one, so that we can
-# easily identity through which relation were the changes replicated.
+# Note: We only create one table (tab4) here. We specified
+# publish_via_partition_root = true (see pub_all and pub_lower_level above), so
+# all data will be replicated to that table.
$node_subscriber2->safe_psql('postgres',
"CREATE TABLE tab4 (a int PRIMARY KEY)");
-$node_subscriber2->safe_psql('postgres',
- "CREATE TABLE tab4_1 (a int PRIMARY KEY)");
I am not sure if it is a good idea to remove tab4_1 here. It is
testing something different as mentioned in the comments. Also, I
don't see any data in tab4 for the initial sync, so not sure if this
tests the behavior changed by this patch.
4.
--- a/src/test/subscription/t/031_column_list.pl
+++ b/src/test/subscription/t/031_column_list.pl
@@ -959,7 +959,8 @@ $node_publisher->safe_psql(
CREATE TABLE test_root_1 PARTITION OF test_root FOR VALUES FROM (1) TO (10);
CREATE TABLE test_root_2 PARTITION OF test_root FOR VALUES FROM (10) TO (20);
- CREATE PUBLICATION pub_root_true FOR TABLE test_root (a) WITH
(publish_via_partition_root = true);
+ CREATE PUBLICATION pub_root_true_1 FOR TABLE test_root (a) WITH
(publish_via_partition_root = true);
+ CREATE PUBLICATION pub_root_true_2 FOR TABLE test_root_1 (a, b) WITH
(publish_via_partition_root = true);
-- initial data
INSERT INTO test_root VALUES (1, 2, 3);
@@ -968,7 +969,7 @@ $node_publisher->safe_psql(
$node_subscriber->safe_psql(
'postgres', qq(
- CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION
pub_root_true;
+ CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION
pub_root_true_1, pub_root_true_2;
It is not clear to me what exactly you want to test here. Please add
some comments.
5. I think you can merge the 0001 and 0003 patches.
Apart from the above, attached is a patch to change some of the
comments in the patch.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 82941a0ce7..44fc371d2c 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1949,22 +1949,15 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
initStringInfo(&cmd);
- /*
- * Get namespace, relname and column list (if supported) of the tables
- * belonging to the specified publications.
- *
- * Get the list of tables from the publisher. The partition table whose
- * ancestor is also in this list will be ignored, otherwise the initial
- * data in the partition table would be replicated twice.
- *
- * From version 16, the parameter of the function
- * pg_get_publication_tables can be an array of publications. The
- * partition table whose ancestor is also published in this publication
- * array will be filtered out in this function.
- */
+ /* Get the list of tables from the publisher. */
if (server_version >= 160000)
{
/*
+ * From version 16, we allowed passing multiple publications to the
+ * function pg_get_publication_tables. This helped to filter out the
+ * partition table whose ancestor is also published in this publication
+ * array.
+ *
* Join pg_get_publication_tables with pg_publication to exclude
* non-existing publications.
*/
Attachments:
[text/plain] change_comments_1.patch.txt (1.3K, ../CAA4eK1Jpj2Yj5ZWwt4z-qM2fS0c3ZF3_xqAgor-Ziiru00LKHg@mail.gmail.com/2-change_comments_1.patch.txt)
download | inline diff:
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 82941a0ce7..44fc371d2c 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1949,22 +1949,15 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
initStringInfo(&cmd);
- /*
- * Get namespace, relname and column list (if supported) of the tables
- * belonging to the specified publications.
- *
- * Get the list of tables from the publisher. The partition table whose
- * ancestor is also in this list will be ignored, otherwise the initial
- * data in the partition table would be replicated twice.
- *
- * From version 16, the parameter of the function
- * pg_get_publication_tables can be an array of publications. The
- * partition table whose ancestor is also published in this publication
- * array will be filtered out in this function.
- */
+ /* Get the list of tables from the publisher. */
if (server_version >= 160000)
{
/*
+ * From version 16, we allowed passing multiple publications to the
+ * function pg_get_publication_tables. This helped to filter out the
+ * partition table whose ancestor is also published in this publication
+ * array.
+ *
* Join pg_get_publication_tables with pg_publication to exclude
* non-existing publications.
*/
view thread (23+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Data is copied twice when specifying both child and parent table in publication
In-Reply-To: <CAA4eK1Jpj2Yj5ZWwt4z-qM2fS0c3ZF3_xqAgor-Ziiru00LKHg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox