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

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