public inbox for [email protected]
help / color / mirror / Atom feedFrom: [email protected] <[email protected]>
To: [email protected] <[email protected]>
Cc: Greg Nancarrow <[email protected]>
Subject: Data is copied twice when specifying both child and parent table in publication
Date: Fri, 15 Oct 2021 11:22:50 +0000
Message-ID: <OS0PR01MB57167F45D481F78CDC5986F794B99@OS0PR01MB5716.jpnprd01.prod.outlook.com> (raw)
Hi,
In another logical replication related thread[1], my colleague Greg found that
if publish_via_partition_root is true, then the child table's data will be
copied twice when adding both child and parent table to the publication.
Example:
-----
Pub:
create table tbl1 (a int) partition by range (a);
create table tbl1_part1 partition of tbl1 for values from (1) to (10);
create table tbl1_part2 partition of tbl1 for values from (10) to (20);
create publication pub for table tbl1, tbl1_part1 with (publish_via_partition_root=on);
insert into tbl1_part1 values(1);
Sub:
create table tbl1 (a int) partition by range (a);
create table tbl1_part1 partition of tbl1 for values from (1) to (10);
create table tbl1_part2 partition of tbl1 for values from (10) to (20);
create subscription sub CONNECTION 'dbname=postgres port=10000' publication pub;
-- data is copied twice
select * from tbl1_part1;
a
---
1
1
-----
The reason is that the subscriber will fetch the table list from publisher
using the following sql[2] and the subscriber will execute table
synchronization for each table in the query results in this case. But
tbl1_part1 is a partition of tbl1, so the data of tbl1_part1 was copied twice.
[2]
select * from pg_publication_tables;
pubname | schemaname | tablename
---------+------------+------------
pub | public | tbl1
pub | public | tbl1_part1
IMO, it looks like a bug and it's more natural to only execute the table
synchronization for the parent table in the above case. Because as the document
said: if publish_via_partition_root is true, "changes in a partitioned table
(or on its partitions) contained in the publication will be published using the
identity and schema of the partitioned table rather than that of the individual
partitions that are actually changed;"
To fix it, I think we should fix function GetPublicationRelations which
generate data for the view pg_publication_tables and make it only show the
parent table if publish_via_partition_root is true. And for other future
feature like schema level publication, we can also follow this to exclude
partitions if their parent is specified by FOR TABLE in the same publication.
Attach a patch to fix it.
Thoughts ?
[1] https://www.postgresql.org/message-id/CAJcOf-eBhDUT2J5zs8Z0qEMiZUdhinX%2BbuGX3GN4V83fPnZV3Q%40mail.g...
Best regards,
Hou zhijie
Attachments:
[application/octet-stream] 0001-fix-double-publish.patch (7.2K, ../OS0PR01MB57167F45D481F78CDC5986F794B99@OS0PR01MB5716.jpnprd01.prod.outlook.com/2-0001-fix-double-publish.patch)
download | inline diff:
From af38de251ac5fb321f19273ec079eac3f08f4f08 Mon Sep 17 00:00:00 2001
From: "houzj.fnst" <[email protected]>
Date: Fri, 15 Oct 2021 18:23:30 +0800
Subject: [PATCH] fix double publish
if publish_via_partition_root is true, then the child table's data will be
copied twice if adding both child and parent table to the publication. The
reason is that the subscriber will fetch the table list from publisher's
pg_publication_tables view to do the table synchronization. But the view always
show both child and parent table which cause the extra synchronization
for the child table.
Fix it by making pg_publication_tables only show parent table if both parent
and child exists in the publication.
---
src/backend/catalog/pg_publication.c | 51 +++++++++++++++++++++--
src/backend/commands/publicationcmds.c | 8 ++--
src/include/catalog/pg_publication.h | 3 +-
src/test/regress/expected/publication.out | 9 ++++
src/test/regress/sql/publication.sql | 4 ++
5 files changed, 67 insertions(+), 8 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 9cd0c82f93..8f039599ee 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -291,11 +291,18 @@ GetRelationPublications(Oid relid)
*
* This should only be used FOR TABLE publications, the FOR ALL TABLES
* should use GetAllTablesPublicationRelations().
+ *
+ * If pubviaroot is true, we must exclude partitions in favor of including the
+ * root partitioned tables.
*/
List *
-GetPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt)
+GetPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt,
+ bool pubviaroot)
{
List *result;
+ List *relids;
+ ListCell *lc;
+ ListCell *lc2;
Relation pubrelsrel;
ScanKeyData scankey;
SysScanDesc scan;
@@ -312,19 +319,54 @@ GetPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt)
scan = systable_beginscan(pubrelsrel, PublicationRelPrrelidPrpubidIndexId,
true, NULL, 1, &scankey);
- result = NIL;
+ result = relids = NIL;
+
while (HeapTupleIsValid(tup = systable_getnext(scan)))
{
Form_pg_publication_rel pubrel;
pubrel = (Form_pg_publication_rel) GETSTRUCT(tup);
- result = GetPubPartitionOptionRelations(result, pub_partopt,
+ relids = GetPubPartitionOptionRelations(relids, pub_partopt,
pubrel->prrelid);
}
systable_endscan(scan);
table_close(pubrelsrel, AccessShareLock);
+ if (!pubviaroot)
+ return relids;
+
+ foreach(lc, relids)
+ {
+ bool skip = false;
+ List *ancestors = NIL;
+ Oid relid = lfirst_oid(lc);
+
+ /*
+ * Filter out the partitions whose parent tables was also specified in
+ * the publication.
+ */
+ if (get_rel_relispartition(relid))
+ ancestors = get_partition_ancestors(relid);
+
+ foreach(lc2, ancestors)
+ {
+ /*
+ * Check if the parent table exists in the published table list or
+ * the schema of the parent table was published in this
+ * publication.
+ */
+ if (list_member_oid(relids, lfirst_oid(lc2)))
+ {
+ skip = true;
+ break;
+ }
+ }
+
+ if (!skip)
+ result = lappend_oid(result, relid);
+ }
+
return result;
}
@@ -560,7 +602,8 @@ pg_get_publication_tables(PG_FUNCTION_ARGS)
tables = GetPublicationRelations(publication->oid,
publication->pubviaroot ?
PUBLICATION_PART_ROOT :
- PUBLICATION_PART_LEAF);
+ PUBLICATION_PART_LEAF,
+ publication->pubviaroot);
funcctx->user_fctx = (void *) tables;
MemoryContextSwitchTo(oldcontext);
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 9c7f91611d..836b6cf421 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -324,7 +324,8 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt,
* trees, not just those explicitly mentioned in the publication.
*/
List *relids = GetPublicationRelations(pubform->oid,
- PUBLICATION_PART_ALL);
+ PUBLICATION_PART_ALL,
+ false);
InvalidatePublicationRels(relids);
}
@@ -387,7 +388,8 @@ AlterPublicationTables(AlterPublicationStmt *stmt, Relation rel,
else /* DEFELEM_SET */
{
List *oldrelids = GetPublicationRelations(pubid,
- PUBLICATION_PART_ROOT);
+ PUBLICATION_PART_ROOT,
+ false);
List *delrels = NIL;
ListCell *oldlc;
@@ -538,7 +540,7 @@ RemovePublicationById(Oid pubid)
if (!HeapTupleIsValid(tup))
elog(ERROR, "cache lookup failed for publication %u", pubid);
- pubform = (Form_pg_publication)GETSTRUCT(tup);
+ pubform = (Form_pg_publication) GETSTRUCT(tup);
/* Invalidate relcache so that publication info is rebuilt. */
if (pubform->puballtables)
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 82f2536c65..721233b226 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -108,7 +108,8 @@ typedef enum PublicationPartOpt
PUBLICATION_PART_ALL,
} PublicationPartOpt;
-extern List *GetPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt);
+extern List *GetPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt,
+ bool pubviaroot);
extern List *GetAllTablesPublications(void);
extern List *GetAllTablesPublicationRelations(bool pubviaroot);
extern List *GetPubPartitionOptionRelations(List *result,
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 82bce9be09..5f5445a985 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -165,6 +165,15 @@ HINT: To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
ALTER PUBLICATION testpub_forparted DROP TABLE testpub_parted;
-- works again, because update is no longer replicated
UPDATE testpub_parted2 SET a = 2;
+-- add both child and parent table to the publication
+ALTER PUBLICATION testpub_forparted ADD TABLE testpub_parted, testpub_parted2;
+-- only show parent table when publish_via_partition_root is set
+select * from pg_publication_tables;
+ pubname | schemaname | tablename
+-------------------+------------+----------------
+ testpub_forparted | public | testpub_parted
+(1 row)
+
DROP TABLE testpub_parted1, testpub_parted2;
DROP PUBLICATION testpub_forparted, testpub_forparted1;
-- Test cache invalidation FOR ALL TABLES publication
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index e5745d575b..d3f2b2fa11 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -97,6 +97,10 @@ UPDATE testpub_parted2 SET a = 2;
ALTER PUBLICATION testpub_forparted DROP TABLE testpub_parted;
-- works again, because update is no longer replicated
UPDATE testpub_parted2 SET a = 2;
+-- add both child and parent table to the publication
+ALTER PUBLICATION testpub_forparted ADD TABLE testpub_parted, testpub_parted2;
+-- only show parent table when publish_via_partition_root is set
+select * from pg_publication_tables;
DROP TABLE testpub_parted1, testpub_parted2;
DROP PUBLICATION testpub_forparted, testpub_forparted1;
--
2.18.4
view thread (91+ 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]
Subject: Re: Data is copied twice when specifying both child and parent table in publication
In-Reply-To: <OS0PR01MB57167F45D481F78CDC5986F794B99@OS0PR01MB5716.jpnprd01.prod.outlook.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