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. On closer inspection, this function 'column_in_column_list; is only called from one place -- the new 'should_publish_column()'. I think the function column_in_column_list should be thrown away and just absorbed into the calling function 'should_publish_column'. Then the misleading function name is eliminated, and the special NULL handling can be commented on properly. ====== Regards, Peter Smith. Fujitsu Australia
