On Thu, May 27, 2021 at 9:02 PM vignesh C <vignes...@gmail.com> wrote: > > Do you say that we replace table_open in publication_add_relation with > > relation_open and have the "\"%s\" is an index" or "\"%s\" is a > > composite type" checks in check_publication_add_relation? If that is > > so, I don't think it's a good idea to have the extra code in > > check_publication_add_relation and I would like it to be the way it is > > currently. > > Before calling check_publication_add_relation, we will call > OpenTableList to get the list of relations. In openTableList we don't > include the errordetail for the failure like you have fixed it in > check_publication_add_relation. When a user tries to add index objects > or composite types, the error will be thrown earlier itself. I didn't > mean to change check_publication_add_relation, I meant to change > table_openrv to relation_openrv in OpenTableList and include error > details in case of failure like the change attached. If you are ok, > please include the change in your patch.
I don't think we need to change that. General intuition is that with CREATE PUBLICATION ... FOR TABLE/FOR ALL TABLES one can specify only tables and if at all an index/composite type is specified, the error messages ""XXXX" is an index"/""XXXX" is a composite type" can imply that they are not supported with CREATE PUBLICATION. There's no need for a detailed error message saying "Index/Composite Type cannot be added to publications.". Whereas foreign/unlogged/temporary/system tables are actually tables, and we don't support them. So a detailed error message here can state that explicitly. I'm not taking the patch, attaching v5 again here to make cfbot happy and for further review. BTW, when we use relation_openrv, we have to use relation_close. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
From c4c4efa96028fb9fe0b88e91c064a35484c6d8f0 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com> Date: Mon, 5 Apr 2021 19:00:24 +0530 Subject: [PATCH v5] Improve publication error messages Adding a foreign table into a publication prints an error saying "foo is not a table". Although, a foreign table is not a regular table, this message could possibly confuse users. Provide a suitable error message according to the object class (table vs foreign table). While at it, separate unlogged/temp table error message into 2 messages. --- .../postgres_fdw/expected/postgres_fdw.out | 6 ++++++ contrib/postgres_fdw/sql/postgres_fdw.sql | 5 +++++ src/backend/catalog/pg_publication.c | 20 ++++++++++++++++--- src/test/regress/expected/publication.out | 16 +++++++++++++++ src/test/regress/sql/publication.sql | 14 +++++++++++++ 5 files changed, 58 insertions(+), 3 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 59e4e27ffb..b14fd8618c 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -9933,3 +9933,9 @@ DROP TABLE base_tbl4; DROP TABLE join_tbl; ALTER SERVER loopback OPTIONS (DROP async_capable); ALTER SERVER loopback2 OPTIONS (DROP async_capable); +-- =================================================================== +-- test error case for create publication on foreign table +-- =================================================================== +CREATE PUBLICATION testpub_foreign_tbl FOR TABLE ft1; --ERROR +ERROR: "ft1" is a foreign table +DETAIL: Foreign tables cannot be added to publications. diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 107d1c0e03..561c9af23a 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -3138,3 +3138,8 @@ DROP TABLE join_tbl; ALTER SERVER loopback OPTIONS (DROP async_capable); ALTER SERVER loopback2 OPTIONS (DROP async_capable); + +-- =================================================================== +-- test error case for create publication on foreign table +-- =================================================================== +CREATE PUBLICATION testpub_foreign_tbl FOR TABLE ft1; --ERROR diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index 86e415af89..592d2d79e1 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -49,6 +49,14 @@ static void check_publication_add_relation(Relation targetrel) { + /* FOREIGN table cannot be part of publication. */ + if (RelationGetForm(targetrel)->relkind == RELKIND_FOREIGN_TABLE) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("\"%s\" is a foreign table", + RelationGetRelationName(targetrel)), + errdetail("Foreign tables cannot be added to publications."))); + /* Must be a regular or partitioned table */ if (RelationGetForm(targetrel)->relkind != RELKIND_RELATION && RelationGetForm(targetrel)->relkind != RELKIND_PARTITIONED_TABLE) @@ -67,12 +75,18 @@ check_publication_add_relation(Relation targetrel) errdetail("System tables cannot be added to publications."))); /* UNLOGGED and TEMP relations cannot be part of publication. */ - if (!RelationIsPermanent(targetrel)) + if (RelationUsesLocalBuffers(targetrel)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("\"%s\" is a temporary table", + RelationGetRelationName(targetrel)), + errdetail("Temporary tables cannot be added to publications."))); + else if (targetrel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("table \"%s\" cannot be replicated", + errmsg("\"%s\" is an unlogged table", RelationGetRelationName(targetrel)), - errdetail("Temporary and unlogged relations cannot be replicated."))); + errdetail("Unlogged tables cannot be added to publications."))); } /* diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out index 63d6ab7a4e..3dd31cfc84 100644 --- a/src/test/regress/expected/publication.out +++ b/src/test/regress/expected/publication.out @@ -160,6 +160,22 @@ DROP PUBLICATION testpub_forparted, testpub_forparted1; CREATE PUBLICATION testpub_fortbl FOR TABLE testpub_view; ERROR: "testpub_view" is not a table DETAIL: Only tables can be added to publications. +CREATE TEMPORARY TABLE testpub_temptbl(a int); +-- fail - temporary table +CREATE PUBLICATION testpub_fortemptbl FOR TABLE testpub_temptbl; +ERROR: "testpub_temptbl" is a temporary table +DETAIL: Temporary tables cannot be added to publications. +DROP TABLE testpub_temptbl; +CREATE UNLOGGED TABLE testpub_unloggedtbl(a int); +-- fail - unlogged table +CREATE PUBLICATION testpub_forunloggedtbl FOR TABLE testpub_unloggedtbl; +ERROR: "testpub_unloggedtbl" is an unlogged table +DETAIL: Unlogged tables cannot be added to publications. +DROP TABLE testpub_unloggedtbl; +-- fail - system table +CREATE PUBLICATION testpub_forsystemtbl FOR TABLE pg_publication; +ERROR: "pg_publication" is a system table +DETAIL: System tables cannot be added to publications. SET client_min_messages = 'ERROR'; CREATE PUBLICATION testpub_fortbl FOR TABLE testpub_tbl1, pub_test.testpub_nopk; RESET client_min_messages; diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql index d844075368..dceb1c3d7f 100644 --- a/src/test/regress/sql/publication.sql +++ b/src/test/regress/sql/publication.sql @@ -95,6 +95,20 @@ DROP PUBLICATION testpub_forparted, testpub_forparted1; -- fail - view CREATE PUBLICATION testpub_fortbl FOR TABLE testpub_view; + +CREATE TEMPORARY TABLE testpub_temptbl(a int); +-- fail - temporary table +CREATE PUBLICATION testpub_fortemptbl FOR TABLE testpub_temptbl; +DROP TABLE testpub_temptbl; + +CREATE UNLOGGED TABLE testpub_unloggedtbl(a int); +-- fail - unlogged table +CREATE PUBLICATION testpub_forunloggedtbl FOR TABLE testpub_unloggedtbl; +DROP TABLE testpub_unloggedtbl; + +-- fail - system table +CREATE PUBLICATION testpub_forsystemtbl FOR TABLE pg_publication; + SET client_min_messages = 'ERROR'; CREATE PUBLICATION testpub_fortbl FOR TABLE testpub_tbl1, pub_test.testpub_nopk; RESET client_min_messages; -- 2.25.1