On Tue, Aug 18, 2026 at 10:44 AM Bharath Rupireddy
<[email protected]> wrote:
>
> Hi,
>
> On Tue, Aug 18, 2026 at 5:52 AM Euler Taveira <[email protected]> wrote:
> >
> > I took another look at it.
>
> Thanks for reviewing.
>
> > +-- Virtual generated columns are always stored as null in the tuple, so 
> > they
> > +-- are not printed at all; a printed null would not be distinguishable 
> > from a
> > +-- column that really contains a null. Stored generated columns are printed
> > +-- as usual.
> >
> > It seems repetitive to say the same explanation in the commit message
> > and at the top of this test. It is sufficient to follow the same pattern
> > from the tests in this file.
>
> Yes, reworded it to just this: -- check that virtual generated columns
> are not printed
>
> > Do you really need to test the 3 commands (I, U, D) here? I'm asking
> > because all of them use the same function (tuple_to_stringinfo) behind
> > the scenes. If so, I suggest that you use a single transaction instead
> > of 3 separate transactions.
>
> Sounds good. I'm all in favor of having fewer in tests, so trimmed it
> down to just inserting 1 row.
>
> > +               /*
> > +                * Virtual generated columns are always stored as null in 
> > the tuple,
> > +                * so don't print them at all; a printed null would not be
> > +                * distinguishable from a column that really contains a 
> > null. pgoutput
> > +                * likewise never publishes virtual generated columns (see
> > +                * logicalrep_should_publish_column()). Stored generated 
> > columns are
> > +                * printed as usual since their values are actually on disk.
> > +                */
> >
> > No need to mention the pgoutput here. It is sufficient (for historical
> > reason) that the commit message says it.
>
> Agreed.
>
> Please find the attached v6 patch.

Thank you for updating the patch!

The v6 patch looks good to me. I'm going to push it to HEAD, barring
any objections.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com


Reply via email to