RE: Data is copied twice when specifying both child and parent table in publication
Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
From: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
To: "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>, Amit Kapila <amit.kapila16@gmail.com>, Peter Smith <smithpb2250@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, vignesh C <vignesh21@gmail.com>, "Takamichi Osumi (Fujitsu)" <osumi.takamichi@fujitsu.com>, "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, Amit Langote <amitlangote09@gmail.com>, Peter
Eisentraut <peter.eisentraut@enterprisedb.com>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>, Dilip Kumar <dilipbalaut@gmail.com>, Greg Nancarrow <gregn4422@gmail.com>
Date: 2023-03-22T06:31:50Z
Lists: pgsql-hackers
Dear Wang,
Thank you for updating patch! Following are comments form v19-0001.
01. logical-replication.sgml
I found a following statement in logical-replication.sgml. I think this may cause
mis-reading because it's OK when publishers list partitions and publish_via_root is true.
```
<para>
A subscriber node may have multiple subscriptions if desired. It is
possible to define multiple subscriptions between a single
publisher-subscriber pair, in which case care must be taken to ensure
that the subscribed publication objects don't overlap.
</para>
```
How about adding "If publications are set publish_via_partition_root as true and
they publish partitions that have same partitioned table, only a change to partitioned
table is published from the publisher."or something like that?
02. filter_partitions
IIUC this function can refactor like following to avoid "skip" flag.
How do you think?
```
@@ -209,7 +209,6 @@ filter_partitions(List *table_infos)
foreach(lc, table_infos)
{
- bool skip = false;
List *ancestors = NIL;
ListCell *lc2;
published_rel *table_info = (published_rel *) lfirst(lc);
@@ -224,13 +223,10 @@ filter_partitions(List *table_infos)
/* Is ancestor exists in the published table list? */
if (is_ancestor_member_tableinfos(ancestor, table_infos))
{
- skip = true;
+ table_infos = foreach_delete_current(table_infos, lc);
break;
}
}
-
- if (skip)
- table_infos = foreach_delete_current(table_infos, lc);
}
}
```
03. fetch_table_list
```
+ /* Get the list of tables from the publisher. */
+ if (server_version >= 160000)
```
I think boolean variable can be used to check it like check_columnlist.
How about "use_extended_function" or something?
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Commits
-
Avoid syncing data twice for the 'publish_via_partition_root' option.
- 062a84442424 16.0 landed
-
Fix partition table's REPLICA IDENTITY checking on the subscriber.
- 52d5ea9adb01 14.5 cited
-
Prohibit combining publications with different column lists.
- fd0b9dcebda7 15.0 cited
-
Fix double publish of child table's data.
- 3f06c00cf6dc 13.6 landed
- 614b77d65a38 14.2 landed
- 5e97905a2c76 15.0 landed
-
Support adding partitioned tables to publication
- 17b9e7f9fe23 13.0 cited