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

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