Shouldn't duplicate addition to publication be a no-op?
I wonder if trying to add a relation to a publication that it is already a
part should be considered a no-op, instead of causing an error (which
happens in the ALTER PUBLICATION ADD TABLES case).
create table bar (a int);
create publication mypub for table bar;
alter publication mypub add table bar;
ERROR: relation "bar" is already member of publication "mypub"
2nd command should be a no-op, IMHO.
Consider the following (I know we're discussing inheritance elsewhere as
well):
create table foo (a int);
create table baz () inherits (foo);
alter table bar inherit foo;
alter table mypub add table foo;
ERROR: relation "bar" is already member of publication "mypub"
There is no way to add foo and children other than bar to mypub without
doing so one-by-one.
If my proposal to make that a no-op sounds desirable, attached patch
implements that.
Thanks,
Amit
Attachments:
0001-Fix-how-duplicate-addition-to-a-publication-is-preve.patchtext/x-diff; name=0001-Fix-how-duplicate-addition-to-a-publication-is-preve.patchDownload
From 810b7bd26458b625e4f9badb0a032effc954d060 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Thu, 13 Apr 2017 19:00:22 +0900
Subject: [PATCH] Fix how duplicate addition to a publication is prevented
Treat such an attempt as a no-op instead of causing an error.
---
src/backend/catalog/pg_publication.c | 18 ++++--------------
src/backend/commands/publicationcmds.c | 14 +++++++-------
src/include/catalog/pg_publication.h | 3 +--
src/test/regress/expected/publication.out | 1 -
4 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 9330e2380a..5af9af0f6e 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -101,8 +101,7 @@ is_publishable_class(Oid relid, Form_pg_class reltuple)
* Insert new publication / relation mapping.
*/
ObjectAddress
-publication_add_relation(Oid pubid, Relation targetrel,
- bool if_not_exists)
+publication_add_relation(Oid pubid, Relation targetrel)
{
Relation rel;
HeapTuple tup;
@@ -110,29 +109,20 @@ publication_add_relation(Oid pubid, Relation targetrel,
bool nulls[Natts_pg_publication_rel];
Oid relid = RelationGetRelid(targetrel);
Oid prrelid;
- Publication *pub = GetPublication(pubid);
ObjectAddress myself,
referenced;
rel = heap_open(PublicationRelRelationId, RowExclusiveLock);
/*
- * Check for duplicates. Note that this does not really prevent
- * duplicates, it's here just to provide nicer error message in common
- * case. The real protection is the unique key on the catalog.
+ * If target relation is already a part of the publication, nothing to
+ * to do here.
*/
if (SearchSysCacheExists2(PUBLICATIONRELMAP, ObjectIdGetDatum(relid),
ObjectIdGetDatum(pubid)))
{
heap_close(rel, RowExclusiveLock);
-
- if (if_not_exists)
- return InvalidObjectAddress;
-
- ereport(ERROR,
- (errcode(ERRCODE_DUPLICATE_OBJECT),
- errmsg("relation \"%s\" is already member of publication \"%s\"",
- RelationGetRelationName(targetrel), pub->name)));
+ return InvalidObjectAddress;
}
check_publication_add_relation(targetrel);
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 541da7ee12..100d2ab9b5 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -52,7 +52,7 @@
static List *OpenTableList(List *tables);
static void CloseTableList(List *rels);
-static void PublicationAddTables(Oid pubid, List *rels, bool if_not_exists,
+static void PublicationAddTables(Oid pubid, List *rels,
AlterPublicationStmt *stmt);
static void PublicationDropTables(Oid pubid, List *rels, bool missing_ok);
@@ -232,7 +232,7 @@ CreatePublication(CreatePublicationStmt *stmt)
Assert(list_length(stmt->tables) > 0);
rels = OpenTableList(stmt->tables);
- PublicationAddTables(puboid, rels, true, NULL);
+ PublicationAddTables(puboid, rels, NULL);
CloseTableList(rels);
}
@@ -357,7 +357,7 @@ AlterPublicationTables(AlterPublicationStmt *stmt, Relation rel,
rels = OpenTableList(stmt->tables);
if (stmt->tableAction == DEFELEM_ADD)
- PublicationAddTables(pubid, rels, false, stmt);
+ PublicationAddTables(pubid, rels, stmt);
else if (stmt->tableAction == DEFELEM_DROP)
PublicationDropTables(pubid, rels, false);
else /* DEFELEM_SET */
@@ -399,7 +399,7 @@ AlterPublicationTables(AlterPublicationStmt *stmt, Relation rel,
* Don't bother calculating the difference for adding, we'll catch
* and skip existing ones when doing catalog update.
*/
- PublicationAddTables(pubid, rels, true, stmt);
+ PublicationAddTables(pubid, rels, stmt);
CloseTableList(delrels);
}
@@ -595,7 +595,7 @@ CloseTableList(List *rels)
* Add listed tables to the publication.
*/
static void
-PublicationAddTables(Oid pubid, List *rels, bool if_not_exists,
+PublicationAddTables(Oid pubid, List *rels,
AlterPublicationStmt *stmt)
{
ListCell *lc;
@@ -612,8 +612,8 @@ PublicationAddTables(Oid pubid, List *rels, bool if_not_exists,
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
RelationGetRelationName(rel));
- obj = publication_add_relation(pubid, rel, if_not_exists);
- if (stmt)
+ obj = publication_add_relation(pubid, rel);
+ if (obj.objectId != InvalidOid)
{
EventTriggerCollectSimpleCommand(obj, InvalidObjectAddress,
(Node *) stmt);
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index f3c4f3932b..07407f6e94 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -93,8 +93,7 @@ extern List *GetPublicationRelations(Oid pubid);
extern List *GetAllTablesPublications(void);
extern List *GetAllTablesPublicationRelations(void);
-extern ObjectAddress publication_add_relation(Oid pubid, Relation targetrel,
- bool if_not_exists);
+extern ObjectAddress publication_add_relation(Oid pubid, Relation targetrel);
extern Oid get_publication_oid(const char *pubname, bool missing_ok);
extern char *get_publication_name(Oid pubid);
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 0964718a60..7a84cc71a7 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -78,7 +78,6 @@ DETAIL: Only tables can be added to publications.
CREATE PUBLICATION testpub_fortbl FOR TABLE testpub_tbl1, pub_test.testpub_nopk;
-- fail - already added
ALTER PUBLICATION testpub_fortbl ADD TABLE testpub_tbl1;
-ERROR: relation "testpub_tbl1" is already member of publication "testpub_fortbl"
-- fail - already added
CREATE PUBLICATION testpub_fortbl FOR TABLE testpub_tbl1;
ERROR: publication "testpub_fortbl" already exists
--
2.11.0
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
I wonder if trying to add a relation to a publication that it is already a
part should be considered a no-op, instead of causing an error (which
happens in the ALTER PUBLICATION ADD TABLES case).
On what grounds?
The equivalent case for inheritance is an error:
regression=# create table foo (a int);
CREATE TABLE
regression=# create table bar () inherits (foo);
CREATE TABLE
regression=# alter table bar inherit foo;
ERROR: relation "foo" would be inherited from more than once
(Your example purporting to show the contrary contains a typo.)
If there's a reason why this case should act differently from that
precedent, you haven't shown it.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Apr 13, 2017 at 9:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
I wonder if trying to add a relation to a publication that it is already a
part should be considered a no-op, instead of causing an error (which
happens in the ALTER PUBLICATION ADD TABLES case).On what grounds?
The equivalent case for inheritance is an error:
regression=# create table foo (a int);
CREATE TABLE
regression=# create table bar () inherits (foo);
CREATE TABLE
regression=# alter table bar inherit foo;
ERROR: relation "foo" would be inherited from more than once
Hmm, yes. Making it a no-op might be surprising to some.
(Your example purporting to show the contrary contains a typo.)
Oops, I had meant: alter publication mypub add table foo;
If there's a reason why this case should act differently from that
precedent, you haven't shown it.
Maybe we won't solve it by doing what I proposed, but if there is a
database like this:
create table foo (a int);
create table bar () inherits(foo);
create publication mypub for table foo;
Dumping and restoring it into another database is not without errors,
because of the order in which things are dumped:
$ createdb test
$ pg_dump -s | psql -e test
<snip>
CREATE PUBLICATION mypub WITH (PUBLISH INSERT, PUBLISH UPDATE, PUBLISH DELETE);
ALTER PUBLICATION mypub ADD TABLE bar;
ALTER PUBLICATION mypub ADD TABLE foo;
ERROR: relation "bar" is already member of publication "mypub"
But perhaps that's a pg_dump issue, not this. I haven't closely
looked. Or perhaps something that will be resolved in the nearby
"Logical replication and inheritance" thread.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 4/13/17 06:23, Amit Langote wrote:
create table bar (a int);
create publication mypub for table bar;
alter publication mypub add table bar;
ERROR: relation "bar" is already member of publication "mypub"2nd command should be a no-op, IMHO.
We generally require a IF NOT EXISTS in those situations.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/04/15 8:53, Peter Eisentraut wrote:
On 4/13/17 06:23, Amit Langote wrote:
create table bar (a int);
create publication mypub for table bar;
alter publication mypub add table bar;
ERROR: relation "bar" is already member of publication "mypub"2nd command should be a no-op, IMHO.
We generally require a IF NOT EXISTS in those situations.
Hmm, okay. So I guess the grammar support will be added later.
By the way, Petr said in the other thread that it could be made a no-op
(presumably without requiring IF NOT EXISTS) on the grounds that
membership of table in publication is "soft object" or "property" rather
than real object.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Apr 16, 2017 at 11:58 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
By the way, Petr said in the other thread that it could be made a no-op
(presumably without requiring IF NOT EXISTS) on the grounds that
membership of table in publication is "soft object" or "property" rather
than real object.
I don't find that argument terribly convincing.
The nearest parallel that we have for this is probably:
ALTER EXTENSION name ADD member_object;
ALTER EXTENSION name DROP member_object;
I would guess this ought to work similarly.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/04/17 14:46, Robert Haas wrote:
On Sun, Apr 16, 2017 at 11:58 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:By the way, Petr said in the other thread that it could be made a no-op
(presumably without requiring IF NOT EXISTS) on the grounds that
membership of table in publication is "soft object" or "property" rather
than real object.I don't find that argument terribly convincing.
The nearest parallel that we have for this is probably:
ALTER EXTENSION name ADD member_object;
ALTER EXTENSION name DROP member_object;I would guess this ought to work similarly.
Hmm, it does make sense to mock this behavior.
create extension dummy;
create table foo ();
alter extension dummy add table foo;
alter extension dummy add table foo;
ERROR: table foo is already a member of extension "dummy"
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers