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

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.

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