On Mon, Apr 5, 2021 at 6:41 PM Euler Taveira <eu...@eulerto.com> wrote:
> Here's the v4 patch reabsed on the latest master, please review it further.
>
> /* UNLOGGED and TEMP relations cannot be part of publication. */
> if (!RelationIsPermanent(targetrel))
> - 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.")));
> + }
>
> RelationIsPermanent(), RelationUsesLocalBuffers(), and
> targetrel->rd_rel->relpersistence all refers to relpersistence. Hence, it is
> not necessary to test !RelationIsPermanent().
Done.
> I would slightly rewrite the commit message to something like:
>
> 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.
Thanks for the better wording.
Attaching v5 patch, please have a look.
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
