Thread

  1. Re: [Patch] Omit virtual generated columns from test_decoding output

    SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-05-05T05:16:26Z

    Hi,
    
    On Mon, May 4, 2026 at 8:09 PM Euler Taveira <euler@eulerto.com> wrote:
    
    > On Mon, May 4, 2026, at 10:11 PM, SATYANARAYANA NARLAPURAM wrote:
    > >
    > > Virtual generated columns are not stored on disk, so heap_getattr() in
    > > tuple_to_stringinfo() always returned NULL for them, producing
    > > misleading output such as
    > >
    > >   table public.t: INSERT: a[integer]:1 b[integer]:10 c[integer]:null
    > >
    > > even though the user could observe a non-null value via SELECT.  Stored
    > > generated columns continue to be emitted as before because their values
    > > do live in the heap tuple.
    > >
    >
    > I wouldn't say misleading but expected. Logical decoding relies on WAL and
    > virtual generated columns are not stored in the WAL.
    >
    > > This matches the pgoutput's logicalrep_should_publish_column()
    > > which never publishes virtual generated columns. Added a regression test.
    > > Please find the patch attached.
    > >
    >
    > There is no guarantee that test_decoding should match the pgoutput.
    
    
    Agreed, not trying to keep them in sync but giving as a reference.
    
    
    
    > I agree that
    > test_decoding shouldn't output virtual generated columns. The problem is
    > that it
    > already does it. I'm afraid that removing it should break existing
    > applications.
    > (I heard that some solutions rely on test_decoding for CDC.) Should we
    > change it
    > as you proposed or add an option to put it back to keep the old behavior?
    >
    
    It is emitting null, I am not sure if it is meaningful for the consumers to
    consume this or
    have taken dependency on this. Adding an extra option isn't an overkill for
    this? I am open
    to this idea if others feel the same.
    
    
    
    > I didn't review your patch but I noticed that there is a new test file for
    > this
    > change. There are some concerns about the total test execution time. Do you
    > really need to include this test? If so, should you combine it with an
    > existing
    > test file?
    
    
    Fair concern, I moved the tests to ddl.sql.  Please find the attached v2
    patch.
    
    Thanks,
    Satya