Re: BUG #18558: ALTER PUBLICATION fails with unhelpful error on attempt to use system column

On Sat, Aug 3, 2024 at 12:14 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> On Fri, 2 Aug 2024 at 14:52, Peter Smith <smithpb2250(at)gmail(dot)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