From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Petr Jelinek <petr@2ndquadrant.com>
Cc: Rafia Sabih <rafia.pghackers@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: adding partitioned tables to publications
Date: Tue, 7 Apr 2020 11:01:02 +0200
Message-ID: <64a2c1c7-3887-787b-e43b-ac15112bda82@2ndquadrant.com> (raw)
In-Reply-To: <CA+HiwqE2-6U=XzsBZ9efzPatFDgrgGybBEj7vm-kUQL8DM4BWw@mail.gmail.com>
References: <CA+HiwqH=Y85vRK3mOdjEkqFK+E=ST=eQiHdpj43L=_eJMOOznQ@mail.gmail.com>
<CA+HiwqGVbp0yh-7-FscZiYu1B_CJjaDZRTYkPTjnjpUgSzE6RA@mail.gmail.com>
<b8f257af-3fcd-65b9-088c-0248d82e4e99@2ndquadrant.com>
<CA+HiwqFZbbJzAZgTjqbRarp_xeqyymuNmGg8Q5tDKTZbk3=c2A@mail.gmail.com>
<c0d69728-c3d8-ede8-ad31-941b2bbaff66@2ndquadrant.com>
<CA+HiwqE+6gNgr-BVYeCMT38naRi9cJaanJb7+FCNsX9XgMwHsQ@mail.gmail.com>
<CA+HiwqFjYE5anArxvkjr37AQMd52L-LZtz9Ld2QrLQ3YfcYhTw@mail.gmail.com>
<CA+HiwqE+z2=wLv3a41h7JopcnUZAiFPeaiQyxZeGQ62QV3vsPQ@mail.gmail.com>
<4f999141-97c8-c968-a726-0a1ec3e36e47@2ndquadrant.com>
<ff6007b3-02c2-8296-2896-db99a51f57e2@2ndquadrant.com>
<CA+HiwqGWXeBA2xagkz_YdxycmO38Vah_aviyJf_kC_kjCdL-Xg@mail.gmail.com>
<CA+HiwqGkknVb+ccLVkgw1DoBYPzpuR43L+aT1eOT8dVEtiedtw@mail.gmail.com>
<bb9427bd-9da9-68f6-108e-d1b21aa5a307@2ndquadrant.com>
<CA+HiwqFA6V1QKLnHjyrVUBiDF4+K0U8MvymWWgkv4uWgeUfQrQ@mail.gmail.com>
<CA+HiwqE2-6U=XzsBZ9efzPatFDgrgGybBEj7vm-kUQL8DM4BWw@mail.gmail.com>
On 2020-04-07 08:44, Amit Langote wrote:
> I updated the patch to make the following changes:
>
> * Rewrote the tests to match in style with those committed yesterday
> * Renamed all variables that had pubasroot in it to have pubviaroot
> instead to match the publication parameter
> * Updated pg_publication catalog documentation
Thanks. I have some further questions:
The change in nodeModifyTable.c to add CheckValidResultRel() is unclear.
It doesn't seem to do anything, and it's not clear how it's related to
this patch.
The changes in GetRelationPublications() are confusing to me:
+ if (published_rels)
+ {
+ num = list_length(result);
+ for (i = 0; i < num; i++)
+ *published_rels = lappend_oid(*published_rels, relid);
+ }
This adds relid to the output list "num" times, where num is the number
of publications found. Shouldn't "i" be used in the loop somehow?
Similarly later in the function.
The descriptions of the new fields in RelationSyncEntry don't seem to
match the code accurately, or at least it's confusing.
replicate_as_relid is always filled in with an ancestor, even if
pubviaroot is not set.
I think the pubviaroot field is actually not necessary. We only need
replicate_as_relid.
There is a markup typo in logical-replication.sgml:
<xref linkend=="sql-createpublication"/>
In pg_dump, you missed updating a branch for an older version. See
attached patch.
Also attached a patch to rephrase the psql output a bit to make it not
so long.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:download all (2 files, .tar.gz)0001-fixup-Allow-publishing-partition-changes-via-ancesto.patch (text/plain, 1.3K) download
From 572549aa23d4e3fa2d1abc0733d33f28cb692c40 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 7 Apr 2020 10:56:11 +0200
Subject: [PATCH 1/2] fixup! Allow publishing partition changes via ancestors
---
src/bin/pg_dump/pg_dump.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.cindex 4d03608596..c579227b19 100644--- a/src/bin/pg_dump/pg_dump.c+++ b/src/bin/pg_dump/pg_dump.c@@ -3891,14 +3891,14 @@ getPublications(Archive *fout)
appendPQExpBuffer(query,
"SELECT p.tableoid, p.oid, p.pubname, "
"(%s p.pubowner) AS rolname, "
- "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete, p.pubtruncate, false as pubviaroot "+ "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete, p.pubtruncate, false AS pubviaroot "
"FROM pg_publication p",
username_subquery);
else
appendPQExpBuffer(query,
"SELECT p.tableoid, p.oid, p.pubname, "
"(%s p.pubowner) AS rolname, "
- "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete, false AS pubtruncate "+ "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete, false AS pubtruncate, false AS pubviaroot "
"FROM pg_publication p",
username_subquery);
--
2.26.0
0002-fixup-Allow-publishing-partition-changes-via-ancesto.patch (text/plain, 11.8K) download
From 3e12e0ff53e4d1aa5b78b6b7fa181e79ca280ef0 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 7 Apr 2020 10:58:55 +0200
Subject: [PATCH 2/2] fixup! Allow publishing partition changes via ancestors
---
src/bin/psql/describe.c | 4 +-
src/test/regress/expected/publication.out | 72 +++++++++++------------
2 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.cindex c731ed6322..f05e914b4d 100644--- a/src/bin/psql/describe.c+++ b/src/bin/psql/describe.c@@ -5741,7 +5741,7 @@ listPublications(const char *pattern)
if (pset.sversion >= 130000)
appendPQExpBuffer(&buf,
",\n pubviaroot AS \"%s\"",
- gettext_noop("Publishes Using Root Schema"));+ gettext_noop("Via root"));
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_publication\n");
@@ -5874,7 +5874,7 @@ describePublications(const char *pattern)
if (has_pubtruncate)
printTableAddHeader(&cont, gettext_noop("Truncates"), true, align);
if (has_pubviaroot)
- printTableAddHeader(&cont, gettext_noop("Publishes Using Root Schema"), true, align);+ printTableAddHeader(&cont, gettext_noop("Via root"), true, align);
printTableAddCell(&cont, PQgetvalue(res, i, 2), false, false);
printTableAddCell(&cont, PQgetvalue(res, i, 3), false, false);
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.outindex 5b4e73d91b..63d6ab7a4e 100644--- a/src/test/regress/expected/publication.out+++ b/src/test/regress/expected/publication.out@@ -28,18 +28,18 @@ ERROR: unrecognized "publish" value: "cluster"
CREATE PUBLICATION testpub_xxx WITH (publish_via_partition_root = 'true', publish_via_partition_root = '0');
ERROR: conflicting or redundant options
\dRp
- List of publications- Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | Publishes Using Root Schema ---------------------+--------------------------+------------+---------+---------+---------+-----------+-----------------------------+ List of publications+ Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root +--------------------+--------------------------+------------+---------+---------+---------+-----------+----------
testpib_ins_trunct | regress_publication_user | f | t | f | f | f | f
testpub_default | regress_publication_user | f | f | t | f | f | f
(2 rows)
ALTER PUBLICATION testpub_default SET (publish = 'insert, update, delete');
\dRp
- List of publications- Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | Publishes Using Root Schema ---------------------+--------------------------+------------+---------+---------+---------+-----------+-----------------------------+ List of publications+ Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root +--------------------+--------------------------+------------+---------+---------+---------+-----------+----------
testpib_ins_trunct | regress_publication_user | f | t | f | f | f | f
testpub_default | regress_publication_user | f | t | t | t | f | f
(2 rows)
@@ -85,9 +85,9 @@ Publications:
"testpub_foralltables"
\dRp+ testpub_foralltables
- Publication testpub_foralltables- Owner | All tables | Inserts | Updates | Deletes | Truncates | Publishes Using Root Schema ---------------------------+------------+---------+---------+---------+-----------+-----------------------------+ Publication testpub_foralltables+ Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root +--------------------------+------------+---------+---------+---------+-----------+----------
regress_publication_user | t | t | t | f | f | f
(1 row)
@@ -100,18 +100,18 @@ CREATE PUBLICATION testpub3 FOR TABLE testpub_tbl3;
CREATE PUBLICATION testpub4 FOR TABLE ONLY testpub_tbl3;
RESET client_min_messages;
\dRp+ testpub3
- Publication testpub3- Owner | All tables | Inserts | Updates | Deletes | Truncates | Publishes Using Root Schema ---------------------------+------------+---------+---------+---------+-----------+-----------------------------+ Publication testpub3+ Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root +--------------------------+------------+---------+---------+---------+-----------+----------
regress_publication_user | f | t | t | t | t | f
Tables:
"public.testpub_tbl3"
"public.testpub_tbl3a"
\dRp+ testpub4
- Publication testpub4- Owner | All tables | Inserts | Updates | Deletes | Truncates | Publishes Using Root Schema ---------------------------+------------+---------+---------+---------+-----------+-----------------------------+ Publication testpub4+ Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root +--------------------------+------------+---------+---------+---------+-----------+----------
regress_publication_user | f | t | t | t | t | f
Tables:
"public.testpub_tbl3"
@@ -131,9 +131,9 @@ ALTER TABLE testpub_parted ATTACH PARTITION testpub_parted1 FOR VALUES IN (1);
-- only parent is listed as being in publication, not the partition
ALTER PUBLICATION testpub_forparted ADD TABLE testpub_parted;
\dRp+ testpub_forparted
- Publication testpub_forparted- Owner | All tables | Inserts | Updates | Deletes | Truncates | Publishes Using Root Schema ---------------------------+------------+---------+---------+---------+-----------+-----------------------------+ Publication testpub_forparted+ Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root +--------------------------+------------+---------+---------+---------+-----------+----------
regress_publication_user | f | t | t | t | t | f
Tables:
"public.testpub_parted"
@@ -147,9 +147,9 @@ ALTER TABLE testpub_parted DETACH PARTITION testpub_parted1;
UPDATE testpub_parted1 SET a = 1;
ALTER PUBLICATION testpub_forparted SET (publish_via_partition_root = true);
\dRp+ testpub_forparted
- Publication testpub_forparted- Owner | All tables | Inserts | Updates | Deletes | Truncates | Publishes Using Root Schema ---------------------------+------------+---------+---------+---------+-----------+-----------------------------+ Publication testpub_forparted+ Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root +--------------------------+------------+---------+---------+---------+-----------+----------
regress_publication_user | f | t | t | t | t | t
Tables:
"public.testpub_parted"
@@ -170,9 +170,9 @@ ERROR: relation "testpub_tbl1" is already member of publication "testpub_fortbl
CREATE PUBLICATION testpub_fortbl FOR TABLE testpub_tbl1;
ERROR: publication "testpub_fortbl" already exists
\dRp+ testpub_fortbl
- Publication testpub_fortbl- Owner | All tables | Inserts | Updates | Deletes | Truncates | Publishes Using Root Schema ---------------------------+------------+---------+---------+---------+-----------+-----------------------------+ Publication testpub_fortbl+ Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root +--------------------------+------------+---------+---------+---------+-----------+----------
regress_publication_user | f | t | t | t | t | f
Tables:
"pub_test.testpub_nopk"
@@ -211,9 +211,9 @@ Publications:
"testpub_fortbl"
\dRp+ testpub_default
- Publication testpub_default- Owner | All tables | Inserts | Updates | Deletes | Truncates | Publishes Using Root Schema ---------------------------+------------+---------+---------+---------+-----------+-----------------------------+ Publication testpub_default+ Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root +--------------------------+------------+---------+---------+---------+-----------+----------
regress_publication_user | f | t | t | t | f | f
Tables:
"pub_test.testpub_nopk"
@@ -258,9 +258,9 @@ DROP TABLE testpub_parted;
DROP VIEW testpub_view;
DROP TABLE testpub_tbl1;
\dRp+ testpub_default
- Publication testpub_default- Owner | All tables | Inserts | Updates | Deletes | Truncates | Publishes Using Root Schema ---------------------------+------------+---------+---------+---------+-----------+-----------------------------+ Publication testpub_default+ Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root +--------------------------+------------+---------+---------+---------+-----------+----------
regress_publication_user | f | t | t | t | f | f
(1 row)
@@ -271,9 +271,9 @@ ERROR: must be owner of publication testpub_default
RESET ROLE;
ALTER PUBLICATION testpub_default RENAME TO testpub_foo;
\dRp testpub_foo
- List of publications- Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | Publishes Using Root Schema --------------+--------------------------+------------+---------+---------+---------+-----------+-----------------------------+ List of publications+ Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root +-------------+--------------------------+------------+---------+---------+---------+-----------+----------
testpub_foo | regress_publication_user | f | t | t | t | f | f
(1 row)
@@ -281,9 +281,9 @@ ALTER PUBLICATION testpub_default RENAME TO testpub_foo;
ALTER PUBLICATION testpub_foo RENAME TO testpub_default;
ALTER PUBLICATION testpub_default OWNER TO regress_publication_user2;
\dRp testpub_default
- List of publications- Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | Publishes Using Root Schema ------------------+---------------------------+------------+---------+---------+---------+-----------+-----------------------------+ List of publications+ Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root +-----------------+---------------------------+------------+---------+---------+---------+-----------+----------
testpub_default | regress_publication_user2 | f | t | t | t | f | f
(1 row)
--
2.26.0
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: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@2ndquadrant.com, amitlangote09@gmail.com, petr@2ndquadrant.com, rafia.pghackers@gmail.com
Subject: Re: adding partitioned tables to publications
In-Reply-To: <64a2c1c7-3887-787b-e43b-ac15112bda82@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox