pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
21+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2024-08-15 80ffcb8427 Improve ALTER PUBLICATION validation and error messages
* BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
@ 2024-07-27 09:00 PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: PG Bug reporting form @ 2024-07-27 09:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 18558
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 17beta2
Operating system: Ubuntu 22.04
Description:
The following script:
CREATE TABLE t(a int);
CREATE PUBLICATION p FOR TABLE t(a);
ALTER PUBLICATION p SET TABLE t (a, ctid);
triggers
ERROR: negative bitmapset member not allowed
Whilst:
CREATE PUBLICATION p FOR TABLE t(a, ctid);
ends up with a more informative
ERROR: cannot use system column "ctid" in publication column list
Reproduced on REL_15_STABLE .. master.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
@ 2024-07-29 01:53 ` Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 21+ messages in thread
From: Peter Smith @ 2024-07-29 01:53 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Sat, Jul 27, 2024 at 11:15 PM PG Bug reporting form
<noreply@postgresql.org> wrote:
>
> The following bug has been logged on the website:
>
> Bug reference: 18558
> Logged by: Alexander Lakhin
> Email address: exclusion@gmail.com
> PostgreSQL version: 17beta2
> Operating system: Ubuntu 22.04
> Description:
>
> The following script:
> CREATE TABLE t(a int);
> CREATE PUBLICATION p FOR TABLE t(a);
>
> ALTER PUBLICATION p SET TABLE t (a, ctid);
> triggers
> ERROR: negative bitmapset member not allowed
>
> Whilst:
> CREATE PUBLICATION p FOR TABLE t(a, ctid);
> ends up with a more informative
> ERROR: cannot use system column "ctid" in publication column list
>
> Reproduced on REL_15_STABLE .. master.
>
Thank you for reporting the inconsistent/unfriendly error message.
~~~
The good message::
The good error message comes from publication_translate_columns()
called by publication_add_relation(). This is why the good message
happens for CREATE PUBLICATION. So, you will also find that ALTER
PUBLICATION .. ADD TABLE would give this same good error message:
test_pub=# CREATE TABLE t2(a int);
CREATE TABLE
test_pub=# ALTER PUBLICATION p ADD TABLE t2(a, ctid);
ERROR: cannot use system column "ctid" in publication column list
~~~
The bad message:
OTOH, "ALTER PUBLICATION ... SET TABLE" uses different logic; it first
builds a BitMapSet (BMS) for the old and new column lists to compare
what has changed. No validation is done here before building the new
BMS so it results in the low-level (not user-friendly) error message
caused by the "ctid" column.
~~~
My fix:
I feel the ALTER ... SET and CREATE PUBLICATION the same column list
validation logic. But instead of cut/pasting that validation checking
from publication_translate_columns(), attached is a diff patch that
exposes the publication_translate_columns() so we can just call that
same function.
Result:
Now all these scenarios will produce the same good error, below.
test_pub=# CREATE TABLE t(a int);
CREATE TABLE
test_pub=# CREATE PUBLICATION p FOR TABLE t(a, cid);
2024-07-29 11:30:16.809 AEST [2281] ERROR: column "cid" of relation
"t" does not exist
2024-07-29 11:30:16.809 AEST [2281] STATEMENT: CREATE PUBLICATION p
FOR TABLE t(a, cid);
ERROR: column "cid" of relation "t" does not exist
test_pub=# CREATE PUBLICATION p FOR TABLE t(a);
CREATE PUBLICATION
test_pub=# ALTER PUBLICATION p SET TABLE t(a, ctid);
2024-07-29 11:30:36.579 AEST [2281] ERROR: cannot use system column
"ctid" in publication column list
2024-07-29 11:30:36.579 AEST [2281] STATEMENT: ALTER PUBLICATION p
SET TABLE t(a, ctid);
ERROR: cannot use system column "ctid" in publication column list
~~~
If this is deemed an acceptable fix, then I will improve on it (e.g.
IMO publication_translate_columns can modified to return the BMS), and
I will also add the necessary test cases.
======
Kind Regards,
Peter Smith.
Fujitsu Australia.
Attachments:
v1-0001-fix-new-collist-validation-msg.patch (application/octet-stream, 2.4K) download
From f36c484e8e76879dbe3d51c318d30d9646da4602 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Mon, 29 Jul 2024 11:45:51 +1000
Subject: [PATCH v1] fix-new-collist-validation-msg
---
src/backend/catalog/pg_publication.c | 5 +----
src/backend/commands/publicationcmds.c | 6 ++++++
src/include/catalog/pg_publication.h | 2 ++
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 0602398..1d45572 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -48,9 +48,6 @@ typedef struct
* table. */
} published_rel;
-static void publication_translate_columns(Relation targetrel, List *columns,
- int *natts, AttrNumber **attrs);
-
/*
* Check if relation can be in given publication and throws appropriate
* error if not.
@@ -498,7 +495,7 @@ compare_int16(const void *a, const void *b)
* FirstLowInvalidHeapAttributeNumber; system columns are forbidden so this
* is okay.
*/
-static void
+void
publication_translate_columns(Relation targetrel, List *columns,
int *natts, AttrNumber **attrs)
{
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 6ea7099..16a6766 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1181,8 +1181,14 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
*/
if (newpubrel->columns)
{
+ AttrNumber *attarray = NULL;
+ int natts = 0;
ListCell *lc;
+ /* Do pre-validation of the new columns. */
+ publication_translate_columns(newpubrel->relation, newpubrel->columns,
+ &natts, &attarray);
+
foreach(lc, newpubrel->columns)
{
char *colname = strVal(lfirst(lc));
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 2f1b6ab..596f2cc 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -157,5 +157,7 @@ extern ObjectAddress publication_add_schema(Oid pubid, Oid schemaid,
extern Bitmapset *pub_collist_to_bitmapset(Bitmapset *columns, Datum pubcols,
MemoryContext mcxt);
+extern void publication_translate_columns(Relation targetrel, List *columns,
+ int *natts, AttrNumber **attrs);
#endif /* PG_PUBLICATION_H */
--
1.8.3.1
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
@ 2024-07-29 02:23 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Tom Lane @ 2024-07-29 02:23 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Peter Smith <smithpb2250@gmail.com> writes:
> On Sat, Jul 27, 2024 at 11:15 PM PG Bug reporting form
> <noreply@postgresql.org> wrote:
>> The following script:
>> CREATE TABLE t(a int);
>> CREATE PUBLICATION p FOR TABLE t(a);
>>
>> ALTER PUBLICATION p SET TABLE t (a, ctid);
>> triggers
>> ERROR: negative bitmapset member not allowed
> My fix:
> I feel the ALTER ... SET and CREATE PUBLICATION the same column list
> validation logic. But instead of cut/pasting that validation checking
> from publication_translate_columns(), attached is a diff patch that
> exposes the publication_translate_columns() so we can just call that
> same function.
Agreed on that, but shouldn't this patch also be removing some code
from the ALTER ... SET path? Or is that part of the cleanup you
handwaved about?
> If this is deemed an acceptable fix, then I will improve on it (e.g.
> IMO publication_translate_columns can modified to return the BMS), and
> I will also add the necessary test cases.
Have at it ...
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-07-29 02:45 ` Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 21+ messages in thread
From: Peter Smith @ 2024-07-29 02:45 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, Jul 29, 2024 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Smith <smithpb2250@gmail.com> writes:
> > On Sat, Jul 27, 2024 at 11:15 PM PG Bug reporting form
> > <noreply@postgresql.org> wrote:
> >> The following script:
> >> CREATE TABLE t(a int);
> >> CREATE PUBLICATION p FOR TABLE t(a);
> >>
> >> ALTER PUBLICATION p SET TABLE t (a, ctid);
> >> triggers
> >> ERROR: negative bitmapset member not allowed
>
> > My fix:
> > I feel the ALTER ... SET and CREATE PUBLICATION the same column list
> > validation logic. But instead of cut/pasting that validation checking
> > from publication_translate_columns(), attached is a diff patch that
> > exposes the publication_translate_columns() so we can just call that
> > same function.
>
> Agreed on that, but shouldn't this patch also be removing some code
> from the ALTER ... SET path? Or is that part of the cleanup you
> handwaved about?
My hand waving was about the following code which is building the
'newcolumns' BMS:
foreach(lc, newpubrel->columns)
{
char *colname = strVal(lfirst(lc));
AttrNumber attnum = get_attnum(newrelid, colname);
newcolumns = bms_add_member(newcolumns, attnum);
}
I was thinking that if publication_translate_columns() is modified to
return the BMS, which it is building internally anyway, then we avoid
processing the column list 2x. Then the above ALTER SET code can be
removed. Is that the same code ("shouldn't this patch also be removing
some code") you were referring to?
>
> > If this is deemed an acceptable fix, then I will improve on it (e.g.
> > IMO publication_translate_columns can modified to return the BMS), and
> > I will also add the necessary test cases.
>
> Have at it ...
Thanks!
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
@ 2024-07-29 03:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Tom Lane @ 2024-07-29 03:01 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Peter Smith <smithpb2250@gmail.com> writes:
> On Mon, Jul 29, 2024 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Agreed on that, but shouldn't this patch also be removing some code
>> from the ALTER ... SET path? Or is that part of the cleanup you
>> handwaved about?
> I was thinking that if publication_translate_columns() is modified to
> return the BMS, which it is building internally anyway, then we avoid
> processing the column list 2x. Then the above ALTER SET code can be
> removed. Is that the same code ("shouldn't this patch also be removing
> some code") you were referring to?
If publication_translate_columns is already building that exact same
BMS, then yeah we're on the same page. I hadn't checked the code to
see.
If you'd have to add code to publication_translate_columns, then maybe
it'd be better to make AlterPublicationTables build the BMS from what
publication_translate_columns returns presently.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-07-29 05:18 ` Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Peter Smith @ 2024-07-29 05:18 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, Jul 29, 2024 at 1:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Smith <smithpb2250@gmail.com> writes:
> > On Mon, Jul 29, 2024 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Agreed on that, but shouldn't this patch also be removing some code
> >> from the ALTER ... SET path? Or is that part of the cleanup you
> >> handwaved about?
>
> > I was thinking that if publication_translate_columns() is modified to
> > return the BMS, which it is building internally anyway, then we avoid
> > processing the column list 2x. Then the above ALTER SET code can be
> > removed. Is that the same code ("shouldn't this patch also be removing
> > some code") you were referring to?
>
> If publication_translate_columns is already building that exact same
> BMS, then yeah we're on the same page. I hadn't checked the code to
> see.
>
> If you'd have to add code to publication_translate_columns, then maybe
> it'd be better to make AlterPublicationTables build the BMS from what
> publication_translate_columns returns presently.
>
I chose the 2nd strategy and added a test case. In passing, made
better use of the existing 'newrelid' variable, otherwise that
variable would become unused.
Please see attached fix v2
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v2-0001-fix-new-collist-validation-msg.patch (application/octet-stream, 4.6K) download
From 4cde2fa96adfd40d942e6261b34b8c958ffb9e58 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Mon, 29 Jul 2024 15:12:25 +1000
Subject: [PATCH v2] fix-new-collist-validation-msg
---
src/backend/catalog/pg_publication.c | 5 +----
src/backend/commands/publicationcmds.c | 17 +++++++++--------
src/include/catalog/pg_publication.h | 2 ++
src/test/regress/expected/publication.out | 2 ++
src/test/regress/sql/publication.sql | 1 +
5 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 0602398..1d45572 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -48,9 +48,6 @@ typedef struct
* table. */
} published_rel;
-static void publication_translate_columns(Relation targetrel, List *columns,
- int *natts, AttrNumber **attrs);
-
/*
* Check if relation can be in given publication and throws appropriate
* error if not.
@@ -498,7 +495,7 @@ compare_int16(const void *a, const void *b)
* FirstLowInvalidHeapAttributeNumber; system columns are forbidden so this
* is okay.
*/
-static void
+void
publication_translate_columns(Relation targetrel, List *columns,
int *natts, AttrNumber **attrs)
{
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 6ea7099..e293dda 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1181,15 +1181,16 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
*/
if (newpubrel->columns)
{
- ListCell *lc;
+ AttrNumber *attarray = NULL;
+ int natts = 0;
- foreach(lc, newpubrel->columns)
- {
- char *colname = strVal(lfirst(lc));
- AttrNumber attnum = get_attnum(newrelid, colname);
+ /* First, validate the new column list. */
+ publication_translate_columns(newpubrel->relation, newpubrel->columns,
+ &natts, &attarray);
- newcolumns = bms_add_member(newcolumns, attnum);
- }
+ newcolumns = pub_collist_to_bitmapset(NULL,
+ PointerGetDatum(buildint2vector(attarray, natts)),
+ NULL);
}
/*
@@ -1199,7 +1200,7 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
* expressions also match. Same for the column list. Drop the
* rest.
*/
- if (RelationGetRelid(newpubrel->relation) == oldrelid)
+ if (newrelid == oldrelid)
{
if (equal(oldrelwhereclause, newpubrel->whereClause) &&
bms_equal(oldcolumns, newcolumns))
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 2f1b6ab..596f2cc 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -157,5 +157,7 @@ extern ObjectAddress publication_add_schema(Oid pubid, Oid schemaid,
extern Bitmapset *pub_collist_to_bitmapset(Bitmapset *columns, Datum pubcols,
MemoryContext mcxt);
+extern void publication_translate_columns(Relation targetrel, List *columns,
+ int *natts, AttrNumber **attrs);
#endif /* PG_PUBLICATION_H */
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 30b6371..04a7795 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -693,6 +693,8 @@ ERROR: cannot use generated column "d" in publication column list
-- error: system attributes "ctid" not allowed in column list
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, ctid);
ERROR: cannot use system column "ctid" in publication column list
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl1 (id, ctid);
+ERROR: cannot use system column "ctid" in publication column list
-- ok
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, c);
ALTER TABLE testpub_tbl5 DROP COLUMN c; -- no dice
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 479d4f3..c379901 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -417,6 +417,7 @@ ALTER PUBLICATION testpub_fortable DROP TABLE testpub_tbl5;
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, d);
-- error: system attributes "ctid" not allowed in column list
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, ctid);
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl1 (id, ctid);
-- ok
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, c);
ALTER TABLE testpub_tbl5 DROP COLUMN c; -- no dice
--
1.8.3.1
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
@ 2024-07-29 06:14 ` David Rowley <dgrowleyml@gmail.com>
2024-07-29 08:10 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 14:06 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-08-02 02:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
0 siblings, 3 replies; 21+ messages in thread
From: David Rowley @ 2024-07-29 06:14 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, 29 Jul 2024 at 17:19, Peter Smith <smithpb2250@gmail.com> wrote:
> Please see attached fix v2
It's likely also worth fixing the incorrect header comment for
publication_translate_columns() while fixing this. "and a Bitmapset
with them;" seems to be incorrect and not all that well phrased.
On the whole, I think the API of these functions could be improved as
it's made the fix for this bug more convoluted than it needs to be.
It would be much nicer if publication_translate_columns() returned a
Bitmapset *. That would reduce the code size by a dozen or so lines
as you could get rid of the qsort() and the compare_int16 function.
Converting a bitmapset to a vector will lead to a naturally sorted
vector. Doing this would mean having to invent a function that takes a
Bitmapset to do the job of buildint2vector, which is effectively, the
inverse of pub_collist_to_bitmapset(). You could probably also strip
about 7 lines out of pub_collist_to_bitmapset() by just doing
Bitmapset *result = columns; It probably won't change the compiled
code, however.
I'm on the fence if this should be done as part of this bug fix. The
reason I think it might is that you're changing
publication_translate_columns() to be non-static, and if the above API
change gets done later, that's then changing the API of an existing
external function. The reason against is that it's more risky to do in
the back branches as it's changing more code. What do others think?
Is it worth adding an ALTER PUBLICATION test with a duplicate column too?
David
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
@ 2024-07-29 08:10 ` Peter Smith <smithpb2250@gmail.com>
2024-07-29 14:08 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 21+ messages in thread
From: Peter Smith @ 2024-07-29 08:10 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, Jul 29, 2024 at 4:14 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Mon, 29 Jul 2024 at 17:19, Peter Smith <smithpb2250@gmail.com> wrote:
> > Please see attached fix v2
>
> It's likely also worth fixing the incorrect header comment for
> publication_translate_columns() while fixing this. "and a Bitmapset
> with them;" seems to be incorrect and not all that well phrased.
>
Yeah, I noticed that appeared misleading.
> On the whole, I think the API of these functions could be improved as
> it's made the fix for this bug more convoluted than it needs to be.
I agree. It is better than it was, but is still jumping through some
hoops a bit to get the bitmap.
> It would be much nicer if publication_translate_columns() returned a
> Bitmapset *. That would reduce the code size by a dozen or so lines
> as you could get rid of the qsort() and the compare_int16 function.
> Converting a bitmapset to a vector will lead to a naturally sorted
> vector. Doing this would mean having to invent a function that takes a
> Bitmapset to do the job of buildint2vector, which is effectively, the
> inverse of pub_collist_to_bitmapset(). You could probably also strip
> about 7 lines out of pub_collist_to_bitmapset() by just doing
> Bitmapset *result = columns; It probably won't change the compiled
> code, however.
>
> I'm on the fence if this should be done as part of this bug fix. The
> reason I think it might is that you're changing
> publication_translate_columns() to be non-static, and if the above API
> change gets done later, that's then changing the API of an existing
> external function. The reason against is that it's more risky to do in
> the back branches as it's changing more code. What do others think?
>
I'll wait until tomorrow in case there are other opinions as to
whether that belongs in this patch or elsewhere..
TBH, I was unsure if this error bugfix patch qualified to have
backpatches or not. Although it is a "bug" it was not impacting
anybody because it is only substituting one error msg for another
nicer error msg.
> Is it worth adding an ALTER PUBLICATION test with a duplicate column too?
+1 to do that.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-07-29 08:10 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
@ 2024-07-29 14:08 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 21+ messages in thread
From: Tom Lane @ 2024-07-29 14:08 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Peter Smith <smithpb2250@gmail.com> writes:
> TBH, I was unsure if this error bugfix patch qualified to have
> backpatches or not. Although it is a "bug" it was not impacting
> anybody because it is only substituting one error msg for another
> nicer error msg.
True, we don't really have to back-patch. I'm inclined to do so,
but might think differently once we see how big the final patch is.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
@ 2024-07-29 14:06 ` Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 0 replies; 21+ messages in thread
From: Tom Lane @ 2024-07-29 14:06 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Peter Smith <smithpb2250@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
David Rowley <dgrowleyml@gmail.com> writes:
> On the whole, I think the API of these functions could be improved as
> it's made the fix for this bug more convoluted than it needs to be.
+1
> I'm on the fence if this should be done as part of this bug fix. The
> reason I think it might is that you're changing
> publication_translate_columns() to be non-static, and if the above API
> change gets done later, that's then changing the API of an existing
> external function. The reason against is that it's more risky to do in
> the back branches as it's changing more code. What do others think?
If we are going to export a formerly-static function, we should make
its API as clean as we can. I doubt that that adds meaningful risk,
and it avoids people possibly getting bit by cross-branch differences.
> Is it worth adding an ALTER PUBLICATION test with a duplicate column too?
Probably, just to prove that the non-duplicative representation does
what we want.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
@ 2024-08-02 02:51 ` Peter Smith <smithpb2250@gmail.com>
2024-08-02 14:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2 siblings, 1 reply; 21+ messages in thread
From: Peter Smith @ 2024-08-02 02:51 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Sorry for my delay getting back to this thread.
Here is an updated patch v3 which has the following changes.
1. API.
The publication_translate_columns function now optionally returns the
Bitmapset* (that it was building anyway). The function comment was
also changed.
The patch is not quite the radical change suggested above [1]. I found
the currently returned 'attarray' is used in multiple places (in
publication_add_relation) so it seemed less invasive to leave those
other publication_translate_columns parameters as-is.
2. Tests.
Patch is now also testing for duplicates in a publication column list
======
[1] https://www.postgresql.org/message-id/CAApHDvo2i1j_iCFcURx5q7jYe70qk4Ca7J%2B8Dt9_jSMOdooAOA%40mail.g...
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v3-0001-fix-new-collist-validation-msg.patch (application/octet-stream, 6.9K) download
From 0f3e3786a7619650e817b6e212d865361b2b06c9 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Fri, 2 Aug 2024 12:42:17 +1000
Subject: [PATCH v3] fix new collist validation msg
---
src/backend/catalog/pg_publication.c | 26 +++++++++++++++-----------
src/backend/commands/publicationcmds.c | 20 +++++++++-----------
src/include/catalog/pg_publication.h | 3 +++
src/test/regress/expected/publication.out | 7 +++++++
src/test/regress/sql/publication.sql | 4 ++++
5 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 0602398..bec11ee 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -48,9 +48,6 @@ typedef struct
* table. */
} published_rel;
-static void publication_translate_columns(Relation targetrel, List *columns,
- int *natts, AttrNumber **attrs);
-
/*
* Check if relation can be in given publication and throws appropriate
* error if not.
@@ -401,7 +398,7 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
* Also build an array of attnums, for storing in the catalog.
*/
publication_translate_columns(pri->relation, pri->columns,
- &natts, &attarray);
+ NULL, &natts, &attarray);
/* Form a tuple. */
memset(values, 0, sizeof(values));
@@ -488,18 +485,21 @@ compare_int16(const void *a, const void *b)
}
/*
- * Translate a list of column names to an array of attribute numbers
- * and a Bitmapset with them; verify that each attribute is appropriate
- * to have in a publication column list (no system or generated attributes,
- * no duplicates). Additional checks with replica identity are done later;
- * see pub_collist_contains_invalid_column.
+ * Translate a list of column names to an array of attribute numbers.
+ * The equivalent Bitmapset is also returned when bms_attrs is specified.
+ *
+ * Verify that each attribute is appropriate to have in a
+ * publication column list (no system or generated attributes, no duplicates).
+ * Additional checks with replica identity are done later; see
+ * pub_collist_contains_invalid_column.
*
* Note that the attribute numbers are *not* offset by
* FirstLowInvalidHeapAttributeNumber; system columns are forbidden so this
* is okay.
*/
-static void
+void
publication_translate_columns(Relation targetrel, List *columns,
+ Bitmapset **bms_attrs,
int *natts, AttrNumber **attrs)
{
AttrNumber *attarray = NULL;
@@ -508,6 +508,9 @@ publication_translate_columns(Relation targetrel, List *columns,
int n = 0;
TupleDesc tupdesc = RelationGetDescr(targetrel);
+ if (bms_attrs)
+ *bms_attrs = set;
+
/* Bail out when no column list defined. */
if (!columns)
return;
@@ -556,7 +559,8 @@ publication_translate_columns(Relation targetrel, List *columns,
*natts = n;
*attrs = attarray;
- bms_free(set);
+ if (!bms_attrs)
+ bms_free(set);
}
/*
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 6ea7099..e7e6977 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1176,20 +1176,18 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
newrelid = RelationGetRelid(newpubrel->relation);
/*
- * If the new publication has column list, transform it to a
- * bitmap too.
+ * If the new publication has column list, validate it
+ * before transforming it to a bitmap too.
*/
if (newpubrel->columns)
{
- ListCell *lc;
+ AttrNumber *attarray = NULL;
+ int natts = 0;
- foreach(lc, newpubrel->columns)
- {
- char *colname = strVal(lfirst(lc));
- AttrNumber attnum = get_attnum(newrelid, colname);
-
- newcolumns = bms_add_member(newcolumns, attnum);
- }
+ publication_translate_columns(newpubrel->relation,
+ newpubrel->columns,
+ &newcolumns,
+ &natts, &attarray);
}
/*
@@ -1199,7 +1197,7 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
* expressions also match. Same for the column list. Drop the
* rest.
*/
- if (RelationGetRelid(newpubrel->relation) == oldrelid)
+ if (newrelid == oldrelid)
{
if (equal(oldrelwhereclause, newpubrel->whereClause) &&
bms_equal(oldcolumns, newcolumns))
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 2f1b6ab..5f99d9d 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -157,5 +157,8 @@ extern ObjectAddress publication_add_schema(Oid pubid, Oid schemaid,
extern Bitmapset *pub_collist_to_bitmapset(Bitmapset *columns, Datum pubcols,
MemoryContext mcxt);
+extern void publication_translate_columns(Relation targetrel, List *columns,
+ Bitmapset **bms_attrs,
+ int *natts, AttrNumber **attrs);
#endif /* PG_PUBLICATION_H */
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 30b6371..660245e 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -693,6 +693,13 @@ ERROR: cannot use generated column "d" in publication column list
-- error: system attributes "ctid" not allowed in column list
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, ctid);
ERROR: cannot use system column "ctid" in publication column list
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl1 (id, ctid);
+ERROR: cannot use system column "ctid" in publication column list
+-- error: duplicates not allowed in column list
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, a);
+ERROR: duplicate column "a" in publication column list
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl5 (a, a);
+ERROR: duplicate column "a" in publication column list
-- ok
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, c);
ALTER TABLE testpub_tbl5 DROP COLUMN c; -- no dice
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 479d4f3..f68a5b5 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -417,6 +417,10 @@ ALTER PUBLICATION testpub_fortable DROP TABLE testpub_tbl5;
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, d);
-- error: system attributes "ctid" not allowed in column list
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, ctid);
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl1 (id, ctid);
+-- error: duplicates not allowed in column list
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, a);
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl5 (a, a);
-- ok
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, c);
ALTER TABLE testpub_tbl5 DROP COLUMN c; -- no dice
--
1.8.3.1
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-02 02:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
@ 2024-08-02 14:14 ` David Rowley <dgrowleyml@gmail.com>
2024-08-05 07:43 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: David Rowley @ 2024-08-02 14:14 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, 2 Aug 2024 at 14:52, Peter Smith <smithpb2250@gmail.com> wrote:
> 1. API.
> The publication_translate_columns function now optionally returns the
> Bitmapset* (that it was building anyway). The function comment was
> also changed.
>
> The patch is not quite the radical change suggested above [1]. I found
> the currently returned 'attarray' is used in multiple places (in
> publication_add_relation) so it seemed less invasive to leave those
> other publication_translate_columns parameters as-is.
Looking at this, I only just noticed that fixing the bug is quite
simple. We could just do something like:
- newcolumns =
bms_add_member(newcolumns, attnum);
+ if (attnum >= 0)
+ newcolumns =
bms_add_member(newcolumns, attnum);
as later in AlterPublicationTables() the PublicationAddTables()
function is called and the column list is validated anyway. So if
there are system attributes in the columns list, they'll still be
found and rejected.
For the other refactoring stuff. I know you said you didn't do it
because of the other usages of that array, but those could be fixed
with a bms_next_member() loop. I did that and ended up with:
$ git diff --stat -- src/backend/catalog/pg_publication.c
src/backend/catalog/pg_publication.c | 105 ++++++++++++++---------------------
1 file changed, 41 insertions(+), 64 deletions(-)
It saves about 2 dozen lines. However, I'm not as happy with the idea
as I thought I'd be. I think the refactor feels a bit more pointless
if the bug was fixed with the attnum >= 0 fix.
I've attached patches for the record.
David
Attachments:
AlterPublicationTables_fix.patch.txt (text/plain, 744B) download
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 6ea709988e..97f614dde1 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1188,7 +1188,13 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
char *colname = strVal(lfirst(lc));
AttrNumber attnum = get_attnum(newrelid, colname);
- newcolumns = bms_add_member(newcolumns, attnum);
+ /*
+ * Ignore any unknown columns or system columns, we'll
+ * validate the columns list later during the call to
+ * PublicationAddTables.
+ */
+ if (attnum >= 0)
+ newcolumns = bms_add_member(newcolumns, attnum);
}
}
publication_refactor.patch.txt (text/plain, 9.2K) download
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 0602398a54..6625d92653 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -48,9 +48,8 @@ typedef struct
* table. */
} published_rel;
-static void publication_translate_columns(Relation targetrel, List *columns,
- int *natts, AttrNumber **attrs);
-
+static Bitmapset *publication_validate_column_list(Relation targetrel,
+ List *columns);
/*
* Check if relation can be in given publication and throws appropriate
* error if not.
@@ -351,6 +350,26 @@ GetTopMostAncestorInPublication(Oid puboid, List *ancestors, int *ancestor_level
return topmost_relid;
}
+int2vector *
+attnumstoint2vector(Bitmapset *set)
+{
+ int2vector *result;
+ int n = bms_num_members(set);
+ int i = -1;
+ int j = 0;
+
+ result = buildint2vector(NULL, n);
+
+ while ((i = bms_next_member(set, i)) >= 0)
+ {
+ Assert(i <= PG_INT16_MAX);
+
+ result->values[j++] = (int16) i;
+ }
+
+ return result;
+}
+
/*
* Insert new publication / relation mapping.
*/
@@ -365,12 +384,12 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
Relation targetrel = pri->relation;
Oid relid = RelationGetRelid(targetrel);
Oid pubreloid;
+ Bitmapset *columns;
Publication *pub = GetPublication(pubid);
- AttrNumber *attarray = NULL;
- int natts = 0;
ObjectAddress myself,
referenced;
List *relids = NIL;
+ int i;
rel = table_open(PublicationRelRelationId, RowExclusiveLock);
@@ -395,13 +414,8 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
check_publication_add_relation(targetrel);
- /*
- * Translate column names to attnums and make sure the column list
- * contains only allowed elements (no system or generated columns etc.).
- * Also build an array of attnums, for storing in the catalog.
- */
- publication_translate_columns(pri->relation, pri->columns,
- &natts, &attarray);
+ /* validate and translate column names into a Bitmapset of attnums */
+ columns = publication_validate_column_list(pri->relation, pri->columns);
/* Form a tuple. */
memset(values, 0, sizeof(values));
@@ -423,7 +437,7 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
/* Add column list, if available */
if (pri->columns)
- values[Anum_pg_publication_rel_prattrs - 1] = PointerGetDatum(buildint2vector(attarray, natts));
+ values[Anum_pg_publication_rel_prattrs - 1] = PointerGetDatum(attnumstoint2vector(columns));
else
nulls[Anum_pg_publication_rel_prattrs - 1] = true;
@@ -451,9 +465,10 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
false);
/* Add dependency on the columns, if any are listed */
- for (int i = 0; i < natts; i++)
+ i = -1;
+ while ((i = bms_next_member(columns, i)) >= 0)
{
- ObjectAddressSubSet(referenced, RelationRelationId, relid, attarray[i]);
+ ObjectAddressSubSet(referenced, RelationRelationId, relid, i);
recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
}
@@ -476,47 +491,23 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
return myself;
}
-/* qsort comparator for attnums */
-static int
-compare_int16(const void *a, const void *b)
-{
- int av = *(const int16 *) a;
- int bv = *(const int16 *) b;
-
- /* this can't overflow if int is wider than int16 */
- return (av - bv);
-}
-
/*
- * Translate a list of column names to an array of attribute numbers
- * and a Bitmapset with them; verify that each attribute is appropriate
- * to have in a publication column list (no system or generated attributes,
- * no duplicates). Additional checks with replica identity are done later;
- * see pub_collist_contains_invalid_column.
+ * publication_validate_column_list
+ * Process and validate the 'columns' list and ensure the columns are all
+ * valid to use for a publication. Checks for and raises an ERROR for
+ any; unknown columns, system columns, duplicate columns or generated
+ columns.
*
- * Note that the attribute numbers are *not* offset by
- * FirstLowInvalidHeapAttributeNumber; system columns are forbidden so this
- * is okay.
+ * Looks up each column's attnum and returns a 0-based Bitmapset of the
+ * corresponding attnums.
*/
-static void
-publication_translate_columns(Relation targetrel, List *columns,
- int *natts, AttrNumber **attrs)
+static Bitmapset *
+publication_validate_column_list(Relation targetrel, List *columns)
{
- AttrNumber *attarray = NULL;
Bitmapset *set = NULL;
ListCell *lc;
- int n = 0;
TupleDesc tupdesc = RelationGetDescr(targetrel);
- /* Bail out when no column list defined. */
- if (!columns)
- return;
-
- /*
- * Translate list of columns to attnums. We prohibit system attributes and
- * make sure there are no duplicate columns.
- */
- attarray = palloc(sizeof(AttrNumber) * list_length(columns));
foreach(lc, columns)
{
char *colname = strVal(lfirst(lc));
@@ -547,16 +538,9 @@ publication_translate_columns(Relation targetrel, List *columns,
colname));
set = bms_add_member(set, attnum);
- attarray[n++] = attnum;
}
- /* Be tidy, so that the catalog representation is always sorted */
- qsort(attarray, n, sizeof(AttrNumber), compare_int16);
-
- *natts = n;
- *attrs = attarray;
-
- bms_free(set);
+ return set;
}
/*
@@ -569,19 +553,12 @@ publication_translate_columns(Relation targetrel, List *columns,
Bitmapset *
pub_collist_to_bitmapset(Bitmapset *columns, Datum pubcols, MemoryContext mcxt)
{
- Bitmapset *result = NULL;
+ Bitmapset *result = columns;
ArrayType *arr;
int nelems;
int16 *elems;
MemoryContext oldcxt = NULL;
- /*
- * If an existing bitmap was provided, use it. Otherwise just use NULL and
- * build a new bitmap.
- */
- if (columns)
- result = columns;
-
arr = DatumGetArrayTypeP(pubcols);
nelems = ARR_DIMS(arr)[0];
elems = (int16 *) ARR_DATA_PTR(arr);
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 6ea709988e..97f614dde1 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1188,7 +1188,13 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
char *colname = strVal(lfirst(lc));
AttrNumber attnum = get_attnum(newrelid, colname);
- newcolumns = bms_add_member(newcolumns, attnum);
+ /*
+ * Ignore any unknown columns or system columns, we'll
+ * validate the columns list later during the call to
+ * PublicationAddTables.
+ */
+ if (attnum >= 0)
+ newcolumns = bms_add_member(newcolumns, attnum);
}
}
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index d124bfe55c..a5e18ce396 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2266,7 +2266,7 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
*
* Note that attrs are always stored in sorted order so we don't need
* to worry if different publications have specified them in a
- * different order. See publication_translate_columns.
+ * different order. See publication_validate_column_list.
*/
appendStringInfo(&cmd, "SELECT DISTINCT n.nspname, c.relname, gpt.attrs\n"
" FROM pg_class c\n"
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 30b6371134..660245ed0c 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -693,6 +693,13 @@ ERROR: cannot use generated column "d" in publication column list
-- error: system attributes "ctid" not allowed in column list
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, ctid);
ERROR: cannot use system column "ctid" in publication column list
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl1 (id, ctid);
+ERROR: cannot use system column "ctid" in publication column list
+-- error: duplicates not allowed in column list
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, a);
+ERROR: duplicate column "a" in publication column list
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl5 (a, a);
+ERROR: duplicate column "a" in publication column list
-- ok
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, c);
ALTER TABLE testpub_tbl5 DROP COLUMN c; -- no dice
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 479d4f3264..f68a5b5986 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -417,6 +417,10 @@ ALTER PUBLICATION testpub_fortable DROP TABLE testpub_tbl5;
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, d);
-- error: system attributes "ctid" not allowed in column list
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, ctid);
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl1 (id, ctid);
+-- error: duplicates not allowed in column list
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, a);
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl5 (a, a);
-- ok
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, c);
ALTER TABLE testpub_tbl5 DROP COLUMN c; -- no dice
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-02 02:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-02 14:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
@ 2024-08-05 07:43 ` Peter Smith <smithpb2250@gmail.com>
2024-08-12 07:44 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Peter Smith @ 2024-08-05 07:43 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Sat, Aug 3, 2024 at 12:14 AM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Fri, 2 Aug 2024 at 14:52, Peter Smith <smithpb2250@gmail.com> wrote:
> > 1. API.
> > The publication_translate_columns function now optionally returns the
> > Bitmapset* (that it was building anyway). The function comment was
> > also changed.
> >
> > The patch is not quite the radical change suggested above [1]. I found
> > the currently returned 'attarray' is used in multiple places (in
> > publication_add_relation) so it seemed less invasive to leave those
> > other publication_translate_columns parameters as-is.
>
> Looking at this, I only just noticed that fixing the bug is quite
> simple. We could just do something like:
>
> - newcolumns =
> bms_add_member(newcolumns, attnum);
> + if (attnum >= 0)
> + newcolumns =
> bms_add_member(newcolumns, attnum);
>
> as later in AlterPublicationTables() the PublicationAddTables()
> function is called and the column list is validated anyway. So if
> there are system attributes in the columns list, they'll still be
> found and rejected.
>
... which sounds good. But did you try it?
That AlterPublicationTables_fix.patch.txt patch only prevents the
"negative bitmapset member not allowed" error, but the validation for
system- or generated- columns (which is currently done only by
publication_translate_columns function) is not getting executed.
AFAICT since AlterPublicationTables does not now detect there is any
difference between 'oldcolumns' and 'newcolumns' the
PublicationDropTables decides not to drop the table, then
PublicationAddTables also returns early because the table already
exists.
The result is the bad columns that we specified using ALTER
PUBLICATION ... SET TABLE get silently overlooked.
For example,
test_pub=#
test_pub=# CREATE TABLE t1(a int, b int);
CREATE TABLE
test_pub=# CREATE TABLE t2(a int, b int);
CREATE TABLE
test_pub=# CREATE PUBLICATION pub1 FOR TABLE t1(a);
CREATE PUBLICATION
test_pub=# ALTER PUBLICATION pub1 ADD TABLE t2(a, ctid);
2024-08-05 16:12:34.002 AEST [6048] ERROR: cannot use system column
"ctid" in publication column list
2024-08-05 16:12:34.002 AEST [6048] STATEMENT: ALTER PUBLICATION pub1
ADD TABLE t2(a, ctid);
ERROR: cannot use system column "ctid" in publication column list
test_pub=# ALTER PUBLICATION pub1 ADD TABLE t2(a, a);
2024-08-05 16:13:06.834 AEST [6048] ERROR: duplicate column "a" in
publication column list
2024-08-05 16:13:06.834 AEST [6048] STATEMENT: ALTER PUBLICATION pub1
ADD TABLE t2(a, a);
ERROR: duplicate column "a" in publication column list
The following SET TABLE commands should fail with those same messages,
but your AlterPublicationTables_fix.patch.txt doesn't yield any errors
at all.
test_pub=# ALTER PUBLICATION pub1 SET TABLE t1(a, ctid);
ALTER PUBLICATION
test_pub=# ALTER PUBLICATION pub1 SET TABLE t1(a, a);
ALTER PUBLICATION
test_pub=#
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-02 02:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-02 14:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-05 07:43 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
@ 2024-08-12 07:44 ` David Rowley <dgrowleyml@gmail.com>
2024-08-14 04:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: David Rowley @ 2024-08-12 07:44 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, 5 Aug 2024 at 19:43, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Sat, Aug 3, 2024 at 12:14 AM David Rowley <dgrowleyml@gmail.com> wrote:
> > Looking at this, I only just noticed that fixing the bug is quite
> > simple. We could just do something like:
> >
> > - newcolumns =
> > bms_add_member(newcolumns, attnum);
> > + if (attnum >= 0)
> > + newcolumns =
> > bms_add_member(newcolumns, attnum);
> >
> > as later in AlterPublicationTables() the PublicationAddTables()
> > function is called and the column list is validated anyway. So if
> > there are system attributes in the columns list, they'll still be
> > found and rejected.
> >
>
> ... which sounds good. But did you try it?
Seemingly not well enough. What I was trying to get around was the
double validation when modifying the column list or WHERE clause of an
existing table within a publication. I had assumed that because
PublicationAddTables() is still called that the validation is done
there. What actually happens is PublicationAddTables() is called with
"if_not_exists" == true and we short-circuit out (without validating
the columns) when we see that the table already exists in the
publication.
Maybe it's not worth going to the trouble of rearranging the code so
the validation is only done once. It is fairly cheap. get_attnum() is
a hash table lookup and the algorithm is just O(n) for the number of
attributes. It's probably not going to be a big deal on the
performance front.
Here's the patch updated to do the validation inside AlterPublicationTables().
David
Attachments:
v2-0001-Improve-ALTER-PUBLICATION-validation-and-error-me.patch (application/octet-stream, 11.3K) download
From 856f53c33dc03aeeb690c87411ee5b9520f2de47 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Sun, 4 Aug 2024 10:17:04 +1200
Subject: [PATCH v2] Improve ALTER PUBLICATION validation and error messages
---
src/backend/catalog/pg_publication.c | 110 +++++++++-------------
src/backend/commands/publicationcmds.c | 22 ++---
src/backend/commands/subscriptioncmds.c | 2 +-
src/include/catalog/pg_publication.h | 2 +
src/test/regress/expected/publication.out | 7 ++
src/test/regress/sql/publication.sql | 4 +
6 files changed, 67 insertions(+), 80 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 0602398a54..0e54d6dcbc 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -48,9 +48,6 @@ typedef struct
* table. */
} published_rel;
-static void publication_translate_columns(Relation targetrel, List *columns,
- int *natts, AttrNumber **attrs);
-
/*
* Check if relation can be in given publication and throws appropriate
* error if not.
@@ -351,6 +348,33 @@ GetTopMostAncestorInPublication(Oid puboid, List *ancestors, int *ancestor_level
return topmost_relid;
}
+/*
+ * attnumstoint2vector
+ * Convert a Bitmapset of AttrNumbers into an int2 array.
+ *
+ * AttrNumber numbers are 0-based, i.e., not offset by
+ * FirstLowInvalidHeapAttributeNumber.
+ */
+static int2vector *
+attnumstoint2vector(Bitmapset *attrs)
+{
+ int2vector *result;
+ int n = bms_num_members(attrs);
+ int i = -1;
+ int j = 0;
+
+ result = buildint2vector(NULL, n);
+
+ while ((i = bms_next_member(attrs, i)) >= 0)
+ {
+ Assert(i <= PG_INT16_MAX);
+
+ result->values[j++] = (int16) i;
+ }
+
+ return result;
+}
+
/*
* Insert new publication / relation mapping.
*/
@@ -365,12 +389,12 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
Relation targetrel = pri->relation;
Oid relid = RelationGetRelid(targetrel);
Oid pubreloid;
+ Bitmapset *columns;
Publication *pub = GetPublication(pubid);
- AttrNumber *attarray = NULL;
- int natts = 0;
ObjectAddress myself,
referenced;
List *relids = NIL;
+ int i;
rel = table_open(PublicationRelRelationId, RowExclusiveLock);
@@ -395,13 +419,8 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
check_publication_add_relation(targetrel);
- /*
- * Translate column names to attnums and make sure the column list
- * contains only allowed elements (no system or generated columns etc.).
- * Also build an array of attnums, for storing in the catalog.
- */
- publication_translate_columns(pri->relation, pri->columns,
- &natts, &attarray);
+ /* validate and translate column names into a Bitmapset of attnums */
+ columns = publication_validate_column_list(pri->relation, pri->columns);
/* Form a tuple. */
memset(values, 0, sizeof(values));
@@ -423,7 +442,7 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
/* Add column list, if available */
if (pri->columns)
- values[Anum_pg_publication_rel_prattrs - 1] = PointerGetDatum(buildint2vector(attarray, natts));
+ values[Anum_pg_publication_rel_prattrs - 1] = PointerGetDatum(attnumstoint2vector(columns));
else
nulls[Anum_pg_publication_rel_prattrs - 1] = true;
@@ -451,9 +470,10 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
false);
/* Add dependency on the columns, if any are listed */
- for (int i = 0; i < natts; i++)
+ i = -1;
+ while ((i = bms_next_member(columns, i)) >= 0)
{
- ObjectAddressSubSet(referenced, RelationRelationId, relid, attarray[i]);
+ ObjectAddressSubSet(referenced, RelationRelationId, relid, i);
recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
}
@@ -476,47 +496,23 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
return myself;
}
-/* qsort comparator for attnums */
-static int
-compare_int16(const void *a, const void *b)
-{
- int av = *(const int16 *) a;
- int bv = *(const int16 *) b;
-
- /* this can't overflow if int is wider than int16 */
- return (av - bv);
-}
-
/*
- * Translate a list of column names to an array of attribute numbers
- * and a Bitmapset with them; verify that each attribute is appropriate
- * to have in a publication column list (no system or generated attributes,
- * no duplicates). Additional checks with replica identity are done later;
- * see pub_collist_contains_invalid_column.
+ * publication_validate_column_list
+ * Process and validate the 'columns' list and ensure the columns are all
+ * valid to use for a publication. Checks for and raises an ERROR for
+ * any; unknown columns, system columns, duplicate columns or generated
+ * columns.
*
- * Note that the attribute numbers are *not* offset by
- * FirstLowInvalidHeapAttributeNumber; system columns are forbidden so this
- * is okay.
+ * Looks up each column's attnum and returns a 0-based Bitmapset of the
+ * corresponding attnums.
*/
-static void
-publication_translate_columns(Relation targetrel, List *columns,
- int *natts, AttrNumber **attrs)
+Bitmapset *
+publication_validate_column_list(Relation targetrel, List *columns)
{
- AttrNumber *attarray = NULL;
Bitmapset *set = NULL;
ListCell *lc;
- int n = 0;
TupleDesc tupdesc = RelationGetDescr(targetrel);
- /* Bail out when no column list defined. */
- if (!columns)
- return;
-
- /*
- * Translate list of columns to attnums. We prohibit system attributes and
- * make sure there are no duplicate columns.
- */
- attarray = palloc(sizeof(AttrNumber) * list_length(columns));
foreach(lc, columns)
{
char *colname = strVal(lfirst(lc));
@@ -547,16 +543,9 @@ publication_translate_columns(Relation targetrel, List *columns,
colname));
set = bms_add_member(set, attnum);
- attarray[n++] = attnum;
}
- /* Be tidy, so that the catalog representation is always sorted */
- qsort(attarray, n, sizeof(AttrNumber), compare_int16);
-
- *natts = n;
- *attrs = attarray;
-
- bms_free(set);
+ return set;
}
/*
@@ -569,19 +558,12 @@ publication_translate_columns(Relation targetrel, List *columns,
Bitmapset *
pub_collist_to_bitmapset(Bitmapset *columns, Datum pubcols, MemoryContext mcxt)
{
- Bitmapset *result = NULL;
+ Bitmapset *result = columns;
ArrayType *arr;
int nelems;
int16 *elems;
MemoryContext oldcxt = NULL;
- /*
- * If an existing bitmap was provided, use it. Otherwise just use NULL and
- * build a new bitmap.
- */
- if (columns)
- result = columns;
-
arr = DatumGetArrayTypeP(pubcols);
nelems = ARR_DIMS(arr)[0];
elems = (int16 *) ARR_DATA_PTR(arr);
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 6ea709988e..905f84cc69 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1176,21 +1176,13 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
newrelid = RelationGetRelid(newpubrel->relation);
/*
- * If the new publication has column list, transform it to a
- * bitmap too.
+ * Validate the column list. If the column list or WHERE
+ * clause changes, then the validation done here will be
+ * duplicated inside PublicationAddTables(). The validation
+ * is cheap enough that that seems harmless.
*/
- if (newpubrel->columns)
- {
- ListCell *lc;
-
- foreach(lc, newpubrel->columns)
- {
- char *colname = strVal(lfirst(lc));
- AttrNumber attnum = get_attnum(newrelid, colname);
-
- newcolumns = bms_add_member(newcolumns, attnum);
- }
- }
+ newcolumns = publication_validate_column_list(newpubrel->relation,
+ newpubrel->columns);
/*
* Check if any of the new set of relations matches with the
@@ -1199,7 +1191,7 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
* expressions also match. Same for the column list. Drop the
* rest.
*/
- if (RelationGetRelid(newpubrel->relation) == oldrelid)
+ if (newrelid == oldrelid)
{
if (equal(oldrelwhereclause, newpubrel->whereClause) &&
bms_equal(oldcolumns, newcolumns))
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index d124bfe55c..a5e18ce396 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2266,7 +2266,7 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
*
* Note that attrs are always stored in sorted order so we don't need
* to worry if different publications have specified them in a
- * different order. See publication_translate_columns.
+ * different order. See publication_validate_column_list.
*/
appendStringInfo(&cmd, "SELECT DISTINCT n.nspname, c.relname, gpt.attrs\n"
" FROM pg_class c\n"
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 2f1b6abbfa..8e9193b2fb 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -152,6 +152,8 @@ extern bool is_publishable_relation(Relation rel);
extern bool is_schema_publication(Oid pubid);
extern ObjectAddress publication_add_relation(Oid pubid, PublicationRelInfo *pri,
bool if_not_exists);
+extern Bitmapset *publication_validate_column_list(Relation targetrel,
+ List *columns);
extern ObjectAddress publication_add_schema(Oid pubid, Oid schemaid,
bool if_not_exists);
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 30b6371134..660245ed0c 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -693,6 +693,13 @@ ERROR: cannot use generated column "d" in publication column list
-- error: system attributes "ctid" not allowed in column list
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, ctid);
ERROR: cannot use system column "ctid" in publication column list
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl1 (id, ctid);
+ERROR: cannot use system column "ctid" in publication column list
+-- error: duplicates not allowed in column list
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, a);
+ERROR: duplicate column "a" in publication column list
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl5 (a, a);
+ERROR: duplicate column "a" in publication column list
-- ok
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, c);
ALTER TABLE testpub_tbl5 DROP COLUMN c; -- no dice
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 479d4f3264..f68a5b5986 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -417,6 +417,10 @@ ALTER PUBLICATION testpub_fortable DROP TABLE testpub_tbl5;
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, d);
-- error: system attributes "ctid" not allowed in column list
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, ctid);
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl1 (id, ctid);
+-- error: duplicates not allowed in column list
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, a);
+ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl5 (a, a);
-- ok
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, c);
ALTER TABLE testpub_tbl5 DROP COLUMN c; -- no dice
--
2.34.1
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-02 02:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-02 14:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-05 07:43 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-12 07:44 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
@ 2024-08-14 04:51 ` Peter Smith <smithpb2250@gmail.com>
2024-08-14 22:28 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 21+ messages in thread
From: Peter Smith @ 2024-08-14 04:51 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, Aug 12, 2024 at 5:45 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
...
>
> Here's the patch updated to do the validation inside AlterPublicationTables().
>
Hi David,
I think we've come full circle -- your fix is now pretty much the same
as my v3 patch [1].
The main difference is you have taken the opportunity to tidy some of
the surrounding functions at the same time as making the fix.
Your patch looks good to me.
~~~
publication_add_relation:
nit - variable /Bitmapset *columns;/Bitmapset *attnums;/
nit - comment /validate and translate.../Validate and translate.../
publication_validate_column_list:
nit - consider renaming this function to 'pub_collist_validate' (just
to be more like the existing 'pub_collist_to_bitmapset')
======
[1] https://www.postgresql.org/message-id/CAHut%2BPtp6Ztk-Dst_WDmQpAw%2BZy9EbP_8QSNk5NkDEjP%3D8xK-A%40ma...
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-02 02:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-02 14:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-05 07:43 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-12 07:44 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-14 04:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
@ 2024-08-14 22:28 ` David Rowley <dgrowleyml@gmail.com>
2024-08-14 22:34 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-08-14 22:54 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
0 siblings, 2 replies; 21+ messages in thread
From: David Rowley @ 2024-08-14 22:28 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, 14 Aug 2024 at 16:52, Peter Smith <smithpb2250@gmail.com> wrote:
> On Mon, Aug 12, 2024 at 5:45 PM David Rowley <dgrowleyml@gmail.com> wrote:
> > Here's the patch updated to do the validation inside AlterPublicationTables().
>
> I think we've come full circle -- your fix is now pretty much the same
> as my v3 patch [1].
Thanks for having a look at the patch.
I don't agree that this is full circle. My motivation for the
additional changes is the API cleanliness of the function you're
making extern. I mentioned this in my 2nd paragraph in [1]. I didn't
like that you wanted to make the external function populate an array
and a Bitmapset with the same information. I think just having the
Bitmapset is fine and the array adds needless overhead and confusion.
As for this being backpatched or not. For me, about 51% of me thinks
this is fine for a master-only fix.
Does anyone else have any thoughts about back-patching the fix?
David
[1] https://postgr.es/m/CAApHDvo2i1j_iCFcURx5q7jYe70qk4Ca7J+8Dt9_jSMOdooAOA@mail.gmail.com
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-02 02:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-02 14:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-05 07:43 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-12 07:44 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-14 04:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-14 22:28 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
@ 2024-08-14 22:34 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-08-14 22:46 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 21+ messages in thread
From: Tom Lane @ 2024-08-14 22:34 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Peter Smith <smithpb2250@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
David Rowley <dgrowleyml@gmail.com> writes:
> As for this being backpatched or not. For me, about 51% of me thinks
> this is fine for a master-only fix.
> Does anyone else have any thoughts about back-patching the fix?
It does seem mostly cosmetic, since we're just replacing one error
message with a better one.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-02 02:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-02 14:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-05 07:43 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-12 07:44 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-14 04:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-14 22:28 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-14 22:34 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-08-14 22:46 ` David Rowley <dgrowleyml@gmail.com>
2024-08-14 23:12 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 21+ messages in thread
From: David Rowley @ 2024-08-14 22:46 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Peter Smith <smithpb2250@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, 15 Aug 2024 at 10:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > Does anyone else have any thoughts about back-patching the fix?
>
> It does seem mostly cosmetic, since we're just replacing one error
> message with a better one.
It's not 100% clear, but that sounds like a vote for a master-only fix. Correct?
David
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-02 02:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-02 14:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-05 07:43 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-12 07:44 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-14 04:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-14 22:28 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-14 22:34 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-08-14 22:46 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
@ 2024-08-14 23:12 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 21+ messages in thread
From: Tom Lane @ 2024-08-14 23:12 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Peter Smith <smithpb2250@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
David Rowley <dgrowleyml@gmail.com> writes:
> On Thu, 15 Aug 2024 at 10:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It does seem mostly cosmetic, since we're just replacing one error
>> message with a better one.
> It's not 100% clear, but that sounds like a vote for a master-only fix. Correct?
Yes. Sorry if I was unclear.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-02 02:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-02 14:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-05 07:43 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-12 07:44 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-14 04:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-14 22:28 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
@ 2024-08-14 22:54 ` Peter Smith <smithpb2250@gmail.com>
2024-08-15 01:12 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 21+ messages in thread
From: Peter Smith @ 2024-08-14 22:54 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Aug 15, 2024 at 8:28 AM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Wed, 14 Aug 2024 at 16:52, Peter Smith <smithpb2250@gmail.com> wrote:
> > On Mon, Aug 12, 2024 at 5:45 PM David Rowley <dgrowleyml@gmail.com> wrote:
> > > Here's the patch updated to do the validation inside AlterPublicationTables().
> >
> > I think we've come full circle -- your fix is now pretty much the same
> > as my v3 patch [1].
>
> Thanks for having a look at the patch.
>
> I don't agree that this is full circle.
I meant only that the bugfix part is essentially the same -- both
calling the common validating function, from the same place.
> My motivation for the
> additional changes is the API cleanliness of the function you're
> making extern. I mentioned this in my 2nd paragraph in [1]. I didn't
> like that you wanted to make the external function populate an array
> and a Bitmapset with the same information. I think just having the
> Bitmapset is fine and the array adds needless overhead and confusion.
It's not because I wanted to make a strange API; I was only trying to
touch less code by tweaking what was already there instead of
re-writing it.
Your patch is better.
> As for this being backpatched or not. For me, about 51% of me thinks
> this is fine for a master-only fix.
+1
======
Kind Regards,
Peter Smith.
Fujitsu Australia.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-02 02:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-02 14:14 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-05 07:43 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-12 07:44 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-14 04:51 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
2024-08-14 22:28 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column David Rowley <dgrowleyml@gmail.com>
2024-08-14 22:54 ` Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column Peter Smith <smithpb2250@gmail.com>
@ 2024-08-15 01:12 ` David Rowley <dgrowleyml@gmail.com>
0 siblings, 0 replies; 21+ messages in thread
From: David Rowley @ 2024-08-15 01:12 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, 15 Aug 2024 at 10:55, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, Aug 15, 2024 at 8:28 AM David Rowley <dgrowleyml@gmail.com> wrote:
> > As for this being backpatched or not. For me, about 51% of me thinks
> > this is fine for a master-only fix.
>
> +1
Thanks. Pushed to master only.
David
^ permalink raw reply [nested|flat] 21+ messages in thread
end of thread, other threads:[~2024-08-15 01:12 UTC | newest]
Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-07-27 09:00 BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column PG Bug reporting form <noreply@postgresql.org>
2024-07-29 01:53 ` Peter Smith <smithpb2250@gmail.com>
2024-07-29 02:23 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 02:45 ` Peter Smith <smithpb2250@gmail.com>
2024-07-29 03:01 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 05:18 ` Peter Smith <smithpb2250@gmail.com>
2024-07-29 06:14 ` David Rowley <dgrowleyml@gmail.com>
2024-07-29 08:10 ` Peter Smith <smithpb2250@gmail.com>
2024-07-29 14:08 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-07-29 14:06 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-08-02 02:51 ` Peter Smith <smithpb2250@gmail.com>
2024-08-02 14:14 ` David Rowley <dgrowleyml@gmail.com>
2024-08-05 07:43 ` Peter Smith <smithpb2250@gmail.com>
2024-08-12 07:44 ` David Rowley <dgrowleyml@gmail.com>
2024-08-14 04:51 ` Peter Smith <smithpb2250@gmail.com>
2024-08-14 22:28 ` David Rowley <dgrowleyml@gmail.com>
2024-08-14 22:34 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-08-14 22:46 ` David Rowley <dgrowleyml@gmail.com>
2024-08-14 23:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-08-14 22:54 ` Peter Smith <smithpb2250@gmail.com>
2024-08-15 01:12 ` David Rowley <dgrowleyml@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox