Subject: Re: Data is copied twice when specifying both child and parent table in publication

From: Amit Kapila <[email-masked]>
Date: Thu, 16 Mar 2023 17:55:22 +0530

On Wed, Feb 8, 2023 at 9:21 AM [email-masked]
<[email-masked]> wrote:
>
> I think this failure is caused by the recently commit (b7ae039) in the current
> HEAD. Rebased the patch set and attach them.
>

+ if (server_version >= 160000)
+ {
+ appendStringInfo(&cmd, "SELECT DISTINCT N.nspname, C.relname,\n"
+ "              ( SELECT array_agg(a.attname ORDER BY a.attnum)\n"
+ "                FROM pg_attribute a\n"
+ "                WHERE a.attrelid = GPT.relid AND a.attnum > 0 AND\n"
+ "                      NOT a.attisdropped AND\n"
+ "                      (a.attnum = ANY(GPT.attrs) OR GPT.attrs IS NULL)\n"
+ "              ) AS attnames\n"
+ " FROM pg_class C\n"
+ "   JOIN pg_namespace N ON N.oid = C.relnamespace\n"
+ "   JOIN ( SELECT (pg_get_publication_tables(VARIADIC
array_agg(pubname::text))).*\n"
+ "          FROM pg_publication\n"
+ "          WHERE pubname IN ( %s )) as GPT\n"
+ "       ON GPT.relid = C.oid\n",
+ pub_names.data);

The function pg_get_publication_tables()  has already handled dropped
columns, so we don't need it here in this query. Also, the part to
build attnames should be the same as it is in view
pg_publication_tables. Can we directly try to pass the list of
pubnames to the function pg_get_publication_tables() instead of
joining it with pg_publication?

Can we keep the changes in the else part (fix when publisher < 16) the
same as HEAD and move the proposed change to a separate patch?
Basically, for the HEAD patch, let's just try to fix this when
publisher >=16. I am slightly worried that as this is a corner case
bug and we didn't see any user complaints for this, so introducing a
complex fix for back branches may not be required or at least we can
discuss that separately.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 17 Mar 2023 06:28:00 +0000

On Thu, Mar 16, 2023 at 20:25 PM Amit Kapila <[email-masked]> wrote:

Thanks for your comments.

> pg_publication_tables.

Agree. Changed.

> Can we directly try to pass the list of

Changed.
I think the aim of joining it with pg_publication before is to exclude
non-existing publications. Otherwise, we would get an error because of the call
to function GetPublicationByName (with 'missing_ok = false') in function
pg_get_publication_tables. So, I changed "missing_ok" to true. If anyone doesn't
like this change, I'll reconsider this in the next version.

> complex fix for back branches may not be required or at least we can

Split the patch as suggested.

Attach the new patch set.

Regards,
Wang Wei

---

From: Amit Kapila <[email-masked]>
Date: Fri, 17 Mar 2023 17:37:18 +0530

On Fri, Mar 17, 2023 at 11:58 AM [email-masked]
<[email-masked]> wrote:
> non-existing publications.
>

Okay, A comment for that would have made it clear.

> Otherwise, we would get an error because of the call

I am not sure about changing missing_ok behavior. Did you check it for
any other similar usage in other functions?

+ foreach(lc, pub_elem_tables)
+ {
+ published_rel *table_info = (published_rel *) malloc(sizeof(published_rel));

Is there a reason to use malloc instead of palloc?

-- 
With Regards,
Amit Kapila.

---

From: Jacob Champion <[email-masked]>
Date: Fri, 17 Mar 2023 16:36:53 -0700

> Attach the new patch set.

Hi,

I ran into this problem while hacking on [1], so thank you for tackling
it! I have no strong opinions on the implementation itself; I just want
to register a concern that the tests have not kept up with the
implementation complexity.

For example, the corner case mentioned in 0003, with multiple
publications having conflicting pubviaroot settings, isn't tested as far
as I can see. (I checked manually, and it appears to work as intended.)
And the related pub_lower_level test currently only covers the case
where multiple publications have pubviaroot=true, so the following test
comment is now misleading:

> # for tab4, we publish changes through the "middle" partitioned table
> $node_publisher->safe_psql('postgres',
> 	"CREATE PUBLICATION pub_lower_level FOR TABLE tab4_1 WITH (publish_via_partition_root = true)"
> );

...since the changes are now in fact published via the tab4 root after
this patchset is applied.

> pg_get_publication_tables.

In the same vein, I don't think that case is covered anywhere.

There are a bunch of moving parts and hidden subtleties here, and I fell
into a few traps when I was working on my patch, so it'd be nice to have
additional coverage. I'm happy to contribute effort in that area if it's
helpful.

Thanks!
--Jacob

[1] https://postgr.es/m/dc57f088-039b-7a71-8f4c-082ef106246e%40timescale.com

---

From: Amit Kapila <[email-masked]>
Date: Sat, 18 Mar 2023 10:14:56 +0530

> In the same vein, I don't think that case is covered anywhere.

We can have a test case to cover this scenario.

> additional coverage. I'm happy to contribute effort in that area if it's

I think it depends on what tests you have in mind. I suggest you can
propose a patch to cover tests for this are in a separate thread. We
can then evaluate those separately.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 20 Mar 2023 00:53:56 +0000

> Okay, A comment for that would have made it clear.

Make sense. Added the comment atop the query.

> any other similar usage in other functions?

After reviewing the pg_get_* functions in the 'pg_proc.dat' file, I think most
of them ignore incorrect input, such as the function pg_get_indexdef. However,
some functions, such as pg_get_serial_sequence and pg_get_object_address, will
report an error. So, I think it's better to discuss this in a separate thread.
Reverted this modification. And I will start a new separate thread for this
later.

> Is there a reason to use malloc instead of palloc?

No. I think we need to use palloc here.
Changed.

Regards,
Wang Wei

---

From: Peter Smith <[email-masked]>
Date: Mon, 20 Mar 2023 18:32:15 +1100

Here are some review comments for v17-0001.

======
src/backend/catalog/pg_publication.c

1. filter_partitions

-static List *
-filter_partitions(List *relids)
+static void
+filter_partitions(List *table_infos)
 {
- List    *result = NIL;
  ListCell   *lc;
- ListCell   *lc2;

- foreach(lc, relids)
+ foreach(lc, table_infos)
  {
- bool skip = false;
- List    *ancestors = NIL;
- Oid relid = lfirst_oid(lc);
+ bool skip = false;
+ List    *ancestors = NIL;
+ ListCell    *lc2;
+ published_rel    *table_info = (published_rel *) lfirst(lc);

- if (get_rel_relispartition(relid))
- ancestors = get_partition_ancestors(relid);
+ if (get_rel_relispartition(table_info->relid))
+ ancestors = get_partition_ancestors(table_info->relid);

  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);
  }
-
- return result;
 }

It seems the 'skip' and 'ancestors' and 'lc2' vars are not needed
except when "if (get_rel_relispartition(table_info->relid))" is true,
so won't it be better to restructure the code to put everything inside
that condition. Then you will save a few unnecessary tests of
foreach(lc2, ancestors) and (skip).

For example,

static void
filter_partitions(List *table_infos)
{
ListCell   *lc;

foreach(lc, table_infos)
{
published_rel    *table_info = (published_rel *) lfirst(lc);

if (get_rel_relispartition(table_info->relid))
{
bool skip = false;
List *ancestors = get_partition_ancestors(table_info->relid);
ListCell *lc2;

foreach(lc2, ancestors)
{
Oid ancestor = lfirst_oid(lc2);
ListCell   *lc3;
/* Check if the parent table exists in the published table list. */
foreach(lc3, table_infos)
{
Oid relid = ((published_rel *) lfirst(lc3))->relid;

if (relid == ancestor)
{
skip = true;
break;
}
}
if (skip)
break;
}

if (skip)
table_infos = foreach_delete_current(table_infos, lc);
}
}
}

~~~

2. pg_get_publication_tables

+ else
+ {
+ List    *relids,
+    *schemarelids;
+
+ relids = GetPublicationRelations(pub_elem->oid,
+ pub_elem->pubviaroot ?
+ PUBLICATION_PART_ROOT :
+ PUBLICATION_PART_LEAF);
+ schemarelids = GetAllSchemaPublicationRelations(pub_elem->oid,
+ pub_elem->pubviaroot ?
+ PUBLICATION_PART_ROOT :
+ PUBLICATION_PART_LEAF);
+ pub_elem_tables = list_concat_unique_oid(relids, schemarelids);
+ }

2a.
Maybe 'schema_relids' would be a better name than 'schemareliids'?

~

2b.
By introducing another variable maybe you could remove some of this
duplicated code.

PublicationPartOpt root_or_leaf = pub_elem->pubviaroot ?
PUBLICATION_PART_ROOT : PUBLICATION_PART_LEAF;

~~~

3. pg_get_publication_tables

  /* Show all columns when the column list is not specified. */
- if (nulls[1] == true)
+ if (nulls[2] == true)

Since you are changing this line anyway, you might as well change it
to remove the redundant "== true" part.

SUGGESTION
if (nulls[2])

======
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"?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Mon, 20 Mar 2023 14:23:48 +0530

> PUBLICATION_PART_ROOT : PUBLICATION_PART_LEAF;

IIUC, 2b is an existing code, so I would prefer not to change that as
part of this patch. Similarly, for other comments, unless something is
a very clear improvement and makes difference w.r.t this patch, it
makes sense to change that, otherwise, let's focus on the current
issue.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 20 Mar 2023 15:44:49 +0530

> "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)
  {

- if (!skip)

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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 20 Mar 2023 13:17:52 +0000

Dear Wang,

I have tested about multilevel partitions, and it worked well.
Followings are my comments for v18-0001.

01. pg_get_publication_tables

```
+               ListCell   *lc;
```

This definition can be inside of the "for (i = 0; i < nelems; i++)".

02. pg_get_publication_tables

```
-                        * If the publication publishes partition changes via their
-                        * respective root partitioned tables, we must exclude partitions
-                        * in favor of including the root partitioned tables. Otherwise,
-                        * the function could return both the child and parent tables
-                        * which could cause data of the child table to be
-                        * double-published on the subscriber side.
+                        * Publications support partitioned tables. If
+                        * publish_via_partition_root is false, all changes are replicated
+                        * using leaf partition identity and schema, so we only need those.
+                        * Otherwise, get the partitioned table itself.
```

The comments can be  inside of the "else".

03. pg_get_publication_tables

```
+               pfree(elems);
```

Only elems is pfree()'d here, but how about other variable like pub_elem and pub_elem_tables?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Jacob Champion <[email-masked]>
Date: Mon, 20 Mar 2023 10:52:46 -0700

On Fri, Mar 17, 2023 at 9:45 PM Amit Kapila <[email-masked]> wrote:
> I think it depends on what tests you have in mind.

Just the ones I mentioned, to start with.

> I suggest you can

To confirm -- you want me to start a new thread for tests for this
patchset? (Tests written against HEAD would likely be obsoleted by
this change.)

--Jacob

---

From: Amit Kapila <[email-masked]>
Date: Tue, 21 Mar 2023 11:51:51 +0530

> patchset? (Tests written against HEAD would likely be obsoleted by

If the tests you have in mind are only related to this patch set then
feel free to propose them here if you feel the current ones are not
sufficient. I just want to be cautious that we shouldn't spend too
much time adding additional tests which are related to the base
functionality as we have left with less time for the last CF and I
would like to push the change for HEAD before that.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 21 Mar 2023 07:40:00 +0000

On Mon, Mar 20, 2023 at 18:15 PM Amit Kapila <[email-masked]> wrote:

Changed.

Changed.

Changed.

> tests the behavior changed by this patch.

Reverted this change. And inserted the initial sync data into table tab4 to test
this more clearly.

> It is not clear to me what exactly you want to test here. Please add

Tried to add the following comment to make it clear:
```
+# Subscribe to pub_root_true_1 and pub_root_true_2 at the same time, which
+# means that the initial data will be synced once, and only the column list of
+# the parent table (test_root) in the publication pub_root_true_1 will be used
+# for both table sync and data replication.
 $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;
```

> 5. I think you can merge the 0001 and 0003 patches.

Merged.

> Apart from the above, attached is a patch to change some of the

Thanks for this improvement. I've checked and merged it.

Regards,
Wang Wei

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 21 Mar 2023 07:40:16 +0000

> > Attach the new patch set.

Thanks for your comments and testing.

> this patchset is applied.

Make sense.
Tried to improve this comment like below:
```
If we subscribe only to pub_lower_level, changes for tab4 will be published
through the "middle" partition table. However, since we will be subscribing to
both pub_lower_level and pub_all (see subscription sub2 below), we will publish
changes via the root table (tab4).
```

Regards,
Wang Wei

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 21 Mar 2023 07:41:04 +0000

> table_infos = foreach_delete_current(table_infos, lc);

Refactored this part of code based on other comments.

Changed.

Regards,
Wang Wei

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 21 Mar 2023 07:42:11 +0000

Changed.

> The comments can be  inside of the "else".

Since I think there are related operations in the function
GetAllTablesPublicationRelations, it might be better to write it above the
if-statement.

> Only elems is pfree()'d here, but how about other variable like pub_elem and

Added releases to these two variables.

Regards,
Wang Wei

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 22 Mar 2023 06:31:50 +0000

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

```

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

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 22 Mar 2023 10:09:05 +0000

> table is published from the publisher."or something like that?

I think these seem to be two different scenarios: The scenario mentioned here is
multiple subscriptions at the subscription node, while the scenario we fixed
this time is a single subscription at the subscription node. So, it seems that
these two notes are not strongly related.

> -                       table_infos = foreach_delete_current(table_infos, lc);

I think this approach deletes the cell of the list of the outer loop in the
inner loop. IIUC, we can only use function foreach_delete_current in the current
loop to delete the cell of the current loop.

> How about "use_extended_function" or something?

Since we only need it once, I think it's fine not to add a new variable.

Regards,
Wang Wei

---

From: Jacob Champion <[email-masked]>
Date: Mon, 27 Mar 2023 16:01:59 -0700

On Mon, Mar 20, 2023 at 11:22 PM Amit Kapila <[email-masked]>
wrote:
> sufficient.

I think the new tests added by Wang cover my concerns (thanks!). I share
Peter's comment that we don't seem to have a regression test covering
only the bug description itself -- just ones that combine that case with
row and column restrictions -- but if you're all happy with the existing
approach then I have nothing much to add there.

I was staring at this subquery in fetch_table_list():

> +                        "                WHERE a.attrelid = gpt.relid AND\n"
> +                        "                      a.attnum = ANY(gpt.attrs)\n"
> +                        "              ) AS attnames\n"

On my machine this takes up roughly 90% of the runtime of the query,
which makes for a noticeable delay with a bigger test case (a couple of
FOR ALL TABLES subscriptions on the regression database). And it seems
like we immediately throw all that work away: if I understand correctly,
we only use the third column for its interaction with DISTINCT. Would it
be enough to just replace that whole thing with gpt.attrs?

Thanks,
--Jacob

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 28 Mar 2023 09:59:49 +0000

> approach then I have nothing much to add there.

The scenario of this bug is to subscribe to two publications at the same time,
and these two publications publish parent table and child table respectively.
And option via_root is specified in both publications or only in the publication
of the parent table. At this time, the data on the publisher-side will be copied
twice (the data will be copied to the two tables on the subscribe-side
respectively).
So, I think we have covered this bug itself in 013_partition.pl. We inserted the
initial data into the parent table tab4 on the publisher-side, and checked
whether the sync is completed as we expected (there is data in table tab4, but
there is no data in table tab4_1).

> be enough to just replace that whole thing with gpt.attrs?

Make sense.
Changed as suggested.

Attach the new patch.

Regards,
Wang Wei

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 28 Mar 2023 10:09:27 +0000

> Attach the new patch.

Sorry, I attached the wrong patch.
Here is the correct new version patch which addressed all comments so far.

Regards,
Wang Wei

---

From: Jacob Champion <[email-masked]>
Date: Tue, 28 Mar 2023 10:44:28 -0700

On Tue, Mar 28, 2023 at 2:59 AM [email-masked]
<[email-masked]> wrote:
> of the parent table.

Ah, reading the initial mail again, that makes sense. I came to this
thread with the alternative reproduction in mind (subscribing to one
publication with viaroot=true, and another publication with
viaroot=false) and misread the report accordingly... In the end, I'm
comfortable with the current level of coverage.

> > Would it

LGTM, by inspection. Thanks!

--Jacob