On Mon, Oct 28, 2024 at 12:27 PM Peter Smith <smithpb2...@gmail.com> wrote:
>
> On Mon, Oct 28, 2024 at 4:34 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > On Mon, Oct 28, 2024 at 7:43 AM Peter Smith <smithpb2...@gmail.com> wrote:
> > >
> > > Hi, here are my review comments for patch v43-0001.
> > >
> > > ======
> > > src/backend/replication/logical/proto.c
> > >
> > > 2.
> > > +static bool
> > > +should_publish_column(Form_pg_attribute att, Bitmapset *columns)
> > > +{
> > > + if (att->attisdropped)
> > > + return false;
> > > +
> > > + /*
> > > + * Skip publishing generated columns if they are not included in the
> > > + * column list.
> > > + */
> > > + if (att->attgenerated && !columns)
> > > + return false;
> > > +
> > > + if (!column_in_column_list(att->attnum, columns))
> > > + return false;
> > > +
> > > + return true;
> > > +}
> > >
> > > Here, I wanted to suggest that the whole "Skip publishing generated
> > > columns" if-part is unnecessary because the next check
> > > (!column_in_column_list) is going to return false for the same
> > > scenario anyhow.
> > >
> > > But, unfortunately, the "column_in_column_list" function has some
> > > special NULL handling logic in it; this means none of this code is
> > > quite what it seems to be (e.g. the function name
> > > column_in_column_list is somewhat misleading)
> > >
> > > IMO it would be better to change the column_in_column_list signature
> > > -- add another boolean param to say if a NULL column list is allowed
> > > or not. That will remove any subtle behaviour and then you can remove
> > > the "if (att->attgenerated && !columns)" part.
> > >
> >
> > The NULL column list still means all columns, so changing the behavior
> > as you are proposing doesn't make sense and would make the code
> > difficult to understand.
> >
>
> My point was that the function 'column_in_column_list' would return
> true even when there is no publication column list at all, so that
> function name is misleading.
>
> And, because in patch 0001 the generated columns only work when
> specified via a column list it means now there is a difference
> between:
> - NULL (all columns specified in the column list) and
> - NULL (no column list at all).
>
> which seems strange and likely to cause confusion.
>This is no more strange than it was before the 0001 patch. Also, the comment atop the function clarifies the special condition of the function. OTOH, I am fine with pulling the check outside function as you are proposing especially because now it is called from just one place. -- With Regards, Amit Kapila.
