Re: Pgoutput not capturing the generated columns - Mailing list pgsql-hackers

On Tue, 9 Jul 2024 at 09:53, Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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?
>
We have created a 0004 patch to use the BMS approach. It will be
addressed in the future 0004 patch.

> > > ======
> > > 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.
>
We have created a 0004 patch to use the BMS approach. It will be
addressed in the future 0004 patch.

> ======
> 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
>
Fixed

Please find the updated patch v18-0003 patch at [1].

[1]: https://www.postgresql.org/message-id/CANhcyEW3LVJpRPScz6VBa%3DZipEMV7b-u76PDEALNcNDFURCYMA%40mail.gmail.com

Thanks and Regards,
Shok Kyal