On 2022-Apr-12, Amit Kapila wrote:
> It still has the same problem. The table can be dropped just before
> this message and the get_rel_name will return NULL and we don't expect
> that.
Ugh, I forgot to change the errmsg() parts to use the new variable,
apologies. Fixed.
> Also, is there a reason that you haven't kept the test case added by Hou-San?
None. I put it back here.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
>From f23be23c27eb9bed7350745233f4660f4c5b326a Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 12 Apr 2022 12:59:50 +0200
Subject: [PATCH v4] fixup checking for rowfilter/collist on altering
publication
---
src/backend/commands/publicationcmds.c | 89 +++++++++++------------
src/test/regress/expected/publication.out | 16 +++-
src/test/regress/sql/publication.sql | 8 ++
3 files changed, 63 insertions(+), 50 deletions(-)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 7aacb6b2fe..d2b9f95f6d 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -945,60 +945,57 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt,
foreach(lc, root_relids)
{
- HeapTuple rftuple;
Oid relid = lfirst_oid(lc);
- bool has_column_list;
- bool has_row_filter;
+ char relkind;
+ char *relname;
+ HeapTuple rftuple;
+ bool has_rowfilter;
+ bool has_collist;
+
+ /* Beware: we don't have lock on the relations */
rftuple = SearchSysCache2(PUBLICATIONRELMAP,
ObjectIdGetDatum(relid),
ObjectIdGetDatum(pubform->oid));
-
- has_row_filter
- = !heap_attisnull(rftuple, Anum_pg_publication_rel_prqual, NULL);
-
- has_column_list
- = !heap_attisnull(rftuple, Anum_pg_publication_rel_prattrs, NULL);
-
- if (HeapTupleIsValid(rftuple) &&
- (has_row_filter || has_column_list))
+ if (!HeapTupleIsValid(rftuple))
+ continue;
+ has_rowfilter = !heap_attisnull(rftuple, Anum_pg_publication_rel_prqual, NULL);
+ has_collist = !heap_attisnull(rftuple, Anum_pg_publication_rel_prattrs, NULL);
+ if (!has_rowfilter && !has_collist)
{
- HeapTuple tuple;
-
- tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
- if (HeapTupleIsValid(tuple))
- {
- Form_pg_class relform = (Form_pg_class) GETSTRUCT(tuple);
-
- if ((relform->relkind == RELKIND_PARTITIONED_TABLE) &&
- has_row_filter)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("cannot set %s for publication \"%s\"",
- "publish_via_partition_root = false",
- stmt->pubname),
- errdetail("The publication contains a WHERE clause for a partitioned table \"%s\" "
- "which is not allowed when %s is false.",
- NameStr(relform->relname),
- "publish_via_partition_root")));
-
- if ((relform->relkind == RELKIND_PARTITIONED_TABLE) &&
- has_column_list)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("cannot set %s for publication \"%s\"",
- "publish_via_partition_root = false",
- stmt->pubname),
- errdetail("The publication contains a column list for a partitioned table \"%s\" "
- "which is not allowed when %s is false.",
- NameStr(relform->relname),
- "publish_via_partition_root")));
-
- ReleaseSysCache(tuple);
- }
-
ReleaseSysCache(rftuple);
+ continue;
}
+
+ relkind = get_rel_relkind(relid);
+ if (relkind != RELKIND_PARTITIONED_TABLE)
+ {
+ ReleaseSysCache(rftuple);
+ continue;
+ }
+ relname = get_rel_name(relid);
+ if (relname == NULL) /* table concurrently dropped */
+ {
+ ReleaseSysCache(rftuple);
+ continue;
+ }
+
+ if (has_rowfilter)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot set parameter \"%s\" to false for publication \"%s\"",
+ "publish_via_partition_root",
+ stmt->pubname),
+ errdetail("The publication contains a WHERE clause for partitioned table \"%s\" which is not allowed when \"%s\" is false.",
+ relname, "publish_via_partition_root")));
+ Assert(has_collist);
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot set parameter \"%s\" to false for publication \"%s\"",
+ "publish_via_partition_root",
+ stmt->pubname),
+ errdetail("The publication contains a column list for partitioned table \"%s\", which is not allowed when \"%s\" is false.",
+ relname, "publish_via_partition_root")));
}
}
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 8208f9fa0e..cc9c8990ae 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -588,8 +588,12 @@ UPDATE rf_tbl_abcd_part_pk SET a = 1;
-- fail - cannot set PUBLISH_VIA_PARTITION_ROOT to false if any row filter is
-- used for partitioned table
ALTER PUBLICATION testpub6 SET (PUBLISH_VIA_PARTITION_ROOT=0);
-ERROR: cannot set publish_via_partition_root = false for publication "testpub6"
-DETAIL: The publication contains a WHERE clause for a partitioned table "rf_tbl_abcd_part_pk" which is not allowed when publish_via_partition_root is false.
+ERROR: cannot set parameter "publish_via_partition_root" to false for publication "testpub6"
+DETAIL: The publication contains a WHERE clause for partitioned table "rf_tbl_abcd_part_pk" which is not allowed when "publish_via_partition_root" is false.
+-- remove partitioned table's row filter
+ALTER PUBLICATION testpub6 SET TABLE rf_tbl_abcd_part_pk;
+-- ok - we don't have row filter for partitioned table.
+ALTER PUBLICATION testpub6 SET (PUBLISH_VIA_PARTITION_ROOT=0);
-- Now change the root filter to use a column "b"
-- (which is not in the replica identity)
ALTER PUBLICATION testpub6 SET TABLE rf_tbl_abcd_part_pk_1 WHERE (b > 99);
@@ -951,8 +955,12 @@ UPDATE rf_tbl_abcd_part_pk SET a = 1;
-- fail - cannot set PUBLISH_VIA_PARTITION_ROOT to false if any column list is
-- used for partitioned table
ALTER PUBLICATION testpub6 SET (PUBLISH_VIA_PARTITION_ROOT=0);
-ERROR: cannot set publish_via_partition_root = false for publication "testpub6"
-DETAIL: The publication contains a column list for a partitioned table "rf_tbl_abcd_part_pk" which is not allowed when publish_via_partition_root is false.
+ERROR: cannot set parameter "publish_via_partition_root" to false for publication "testpub6"
+DETAIL: The publication contains a column list for partitioned table "rf_tbl_abcd_part_pk", which is not allowed when "publish_via_partition_root" is false.
+-- remove partitioned table's column list
+ALTER PUBLICATION testpub6 SET TABLE rf_tbl_abcd_part_pk;
+-- ok - we don't have column list for partitioned table.
+ALTER PUBLICATION testpub6 SET (PUBLISH_VIA_PARTITION_ROOT=0);
-- Now change the root column list to use a column "b"
-- (which is not in the replica identity)
ALTER PUBLICATION testpub6 SET TABLE rf_tbl_abcd_part_pk_1 (b);
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 8539110025..9eb86fd54f 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -352,6 +352,10 @@ UPDATE rf_tbl_abcd_part_pk SET a = 1;
-- fail - cannot set PUBLISH_VIA_PARTITION_ROOT to false if any row filter is
-- used for partitioned table
ALTER PUBLICATION testpub6 SET (PUBLISH_VIA_PARTITION_ROOT=0);
+-- remove partitioned table's row filter
+ALTER PUBLICATION testpub6 SET TABLE rf_tbl_abcd_part_pk;
+-- ok - we don't have row filter for partitioned table.
+ALTER PUBLICATION testpub6 SET (PUBLISH_VIA_PARTITION_ROOT=0);
-- Now change the root filter to use a column "b"
-- (which is not in the replica identity)
ALTER PUBLICATION testpub6 SET TABLE rf_tbl_abcd_part_pk_1 WHERE (b > 99);
@@ -635,6 +639,10 @@ UPDATE rf_tbl_abcd_part_pk SET a = 1;
-- fail - cannot set PUBLISH_VIA_PARTITION_ROOT to false if any column list is
-- used for partitioned table
ALTER PUBLICATION testpub6 SET (PUBLISH_VIA_PARTITION_ROOT=0);
+-- remove partitioned table's column list
+ALTER PUBLICATION testpub6 SET TABLE rf_tbl_abcd_part_pk;
+-- ok - we don't have column list for partitioned table.
+ALTER PUBLICATION testpub6 SET (PUBLISH_VIA_PARTITION_ROOT=0);
-- Now change the root column list to use a column "b"
-- (which is not in the replica identity)
ALTER PUBLICATION testpub6 SET TABLE rf_tbl_abcd_part_pk_1 (b);
--
2.30.2
