Re: Pgoutput not capturing the generated columns

Hi Shlok, here are my review comments for v16-0003.

======
src/backend/replication/logical/proto.c


On Mon, Jul 8, 2024 at 10:04 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Mon, 8 Jul 2024 at 13:20, Peter Smith <smithpb2250@gmail.com> wrote:
> >
> >
> > 2. logicalrep_write_tuple and logicalrep_write_attrs
> >
> > I thought all the code fragments like this:
> >
> > + if (att->attgenerated && att->attgenerated != ATTRIBUTE_GENERATED_STORED)
> > + continue;
> > +
> >
> > don't need to be in the code anymore, because of the BitMapSet (BMS)
> > processing done to make the "column list" for publication where
> > disallowed generated cols should already be excluded from the BMS,
> > right?
> >
> > So shouldn't all these be detected by the following statement:
> > if (!column_in_column_list(att->attnum, columns))
> >   continue;
> The current BMS logic do not handle the Virtual Generated Columns.
> There can be cases where we do not want a virtual generated column but
> it would be present in BMS.
> To address this I have added the above logic. I have added this logic
> similar to the checks of 'attr->attisdropped'.
>

Hmm. I thought the BMS idea of patch 0001 is to discover what columns
should be replicated up-front. If they should not be replicated (e.g.
virtual generated columns cannot be) then they should never be in the
BMS.

So what you said ("There can be cases where we do not want a virtual
generated column but it would be present in BMS") should not be
happening. If that is happening then it sounds more like a bug in the
new BMS logic of pgoutput_column_list_init() function. In other words,
if what you say is true, then it seems like the current extra
conditions you have in patch 0004 are just a band-aid to cover a
problem of the BMS logic of patch 0001. Am I mistaken?

> > ======
> > src/backend/replication/pgoutput/pgoutput.c
> >
> > 4. send_relation_and_attrs
> >
> > (this is a similar comment for #2 above)
> >
> > IIUC of the advantages of the BitMapSet (BMS) idea in patch 0001 to
> > process the generated columns up-front means there is no need to check
> > them again in code like this.
> >
> > They should be discovered anyway in the subsequent check:
> > /* Skip this attribute if it's not present in the column list */
> > if (columns != NULL && !bms_is_member(att->attnum, columns))
> >   continue;
> Same explanation as above.

As above.

======
src/test/subscription/t/011_generated.pl

I'm not sure if you needed to say "STORED" generated cols for the
subscriber-side columns but anyway, whatever is done needs to be done
consistently. FYI, below you did *not* say STORED for subscriber-side
generated cols, but in other comments for subscriber-side generated
columns, you did say STORED.

# tab3:
# publisher-side tab3 has STORED generated col 'b' but
# subscriber-side tab3 has DIFFERENT COMPUTATION generated col 'b'.

~

# tab4:
# publisher-side tab4 has STORED generated cols 'b' and 'c' but
# subscriber-side tab4 has non-generated col 'b', and generated-col 'c'
# where columns on publisher/subscriber are in a different order

======
Kind Regards,
Peter Smith.
Fujitsu Australia