On 2022-Apr-12, Amit Kapila wrote:
> On Tue, Apr 12, 2022 at 3:45 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> > We don't have a lock on the relation, so if it gets dropped
> > concurrently, it won't behave sanely. For example, get_rel_name() will
> > return NULL which seems incorrect to me.
Oh, oops ... a trap for the unwary? Anyway, yes, we can disregard the
entry when get_rel_name returns null. Amended patch attached.
> > I am not sure about this as well because you will instead do a RELOID
> > cache lookup even when there is no row filter or column list.
I guess my assumption is that the pg_class cache is typically more
populated than other relcaches, but that's unsubstantiated. I'm not
sure if we have any way to tell which one is the more common case.
Anyway, let's do it the way you already had it.
> It seems to me that we have a similar coding pattern in ExecGrant_Relation().
Not sure what you mean? In that function, when the syscache lookup
returns NULL, an error is raised.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"El número de instalaciones de UNIX se ha elevado a 10,
y se espera que este número aumente" (UPM, 1972)
>From 631a6d04cbe420164833dd4e88a86d0e076fd47d 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 v2] fixup checking for rowfilter/collist on altering
publication
---
src/backend/commands/publicationcmds.c | 91 +++++++++++------------
src/test/regress/expected/publication.out | 8 +-
2 files changed, 49 insertions(+), 50 deletions(-)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 7aacb6b2fe..59fc39e9f2 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -945,60 +945,59 @@ 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 includes partitioned table \"%s\" with a WHERE clause, which is not allowed when \"%s\" is false.",
+ get_rel_name(relid),
+ "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 includes partitioned table \"%s\" with a column list, which is not allowed when \"%s\" is false.",
+ get_rel_name(relid),
+ "publish_via_partition_root")));
}
}
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 8208f9fa0e..a6fd4f6c89 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -588,8 +588,8 @@ 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 includes partitioned table "rf_tbl_abcd_part_pk" with a WHERE clause, which is not allowed when "publish_via_partition_root" is false.
-- 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 +951,8 @@ 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 includes partitioned table "rf_tbl_abcd_part_pk" with a column list, which is not allowed when "publish_via_partition_root" is false.
-- 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
