On Wed, Mar 10, 2021 at 5:48 PM Euler Taveira <eu...@eulerto.com> wrote:
>
> On Wed, Mar 10, 2021, at 2:14 AM, Bharath Rupireddy wrote:
>
> While providing thoughts on [1], I observed that the error messages
> that are emitted while adding foreign, temporary and unlogged tables
> can be improved a bit from the existing [2] to [3]. For instance, the
> existing message when foreign table is tried to add into the
> publication "f1" is not a table" looks odd. Because it says that the
> foreign table is not a table at all.
>
> I wouldn't mix [regular|partitioned|temporary|unlogged] tables with foreign
> tables. Although, they have a pg_class entry in common, foreign tables aren't
> "real" tables (external storage); they even have different DDLs to handle it
> (CREATE TABLE x CREATE FOREIGN TABLE).
>
> postgres=# CREATE PUBLICATION testpub FOR TABLE f1;
> ERROR: "f1" is not a table
> DETAIL: Only tables can be added to publications.
>
> I agree that "f1 is not a table" is a confusing message at first because
> foreign table has "table" as description. Maybe if we apply the negation in
> both messages it would be clear (using the same wording as system tables).
>
> ERROR: "f1" is a foreign table
> DETAIL: Foreign tables cannot be added to publications.
Thanks. Changed the error message and detail to the way we have it for
system tables presently. Attaching v2 patch for further review.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 885a0cc75bb167d2128f1173857a05e0972ddae7 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Thu, 11 Mar 2021 15:03:29 +0530
Subject: [PATCH v2] Improve error message while adding tables to publication
Improve the error messages in check_publication_add_relation
from what we have to a bit more informative and consistent.
---
.../postgres_fdw/expected/postgres_fdw.out | 6 +++++
contrib/postgres_fdw/sql/postgres_fdw.sql | 5 ++++
src/backend/catalog/pg_publication.c | 27 +++++++++++++++----
src/test/regress/expected/publication.out | 16 +++++++++++
src/test/regress/sql/publication.sql | 14 ++++++++++
5 files changed, 63 insertions(+), 5 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 0649b6b81c..a0cc59161e 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9437,3 +9437,9 @@ SELECT tableoid::regclass, * FROM batch_cp_upd_test;
-- Clean up
DROP TABLE batch_table, batch_cp_upd_test CASCADE;
+-- ===================================================================
+-- 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 2b525ea44a..574b7d36df 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2928,3 +2928,8 @@ SELECT tableoid::regclass, * FROM batch_cp_upd_test;
-- Clean up
DROP TABLE batch_table, batch_cp_upd_test CASCADE;
+
+-- ===================================================================
+-- 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 84d2efcfd2..98c77c4b9f 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)
@@ -68,11 +76,20 @@ check_publication_add_relation(Relation targetrel)
/* UNLOGGED and TEMP relations cannot be part of publication. */
if (targetrel->rd_rel->relpersistence != RELPERSISTENCE_PERMANENT)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("table \"%s\" cannot be replicated",
- RelationGetRelationName(targetrel)),
- errdetail("Temporary and unlogged relations cannot be replicated.")));
+ {
+ 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("\"%s\" is an unlogged table",
+ RelationGetRelationName(targetrel)),
+ 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
