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

On Mon, Jul 29, 2024 at 1:01 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Peter Smith <smithpb2250(at)gmail(dot)com> writes:
> > On Mon, Jul 29, 2024 at 12:23 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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