| From: | SATYANARAYANA NARLAPURAM <satyanarlapuram(at)gmail(dot)com> |
|---|---|
| To: | Euler Taveira <euler(at)eulerto(dot)com> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: [Patch] Omit virtual generated columns from test_decoding output |
| Date: | 2026-05-05 05:16:26 |
| Message-ID: | CAHg+QDdPuDskxeSvL1Y+pSkEHmycBv+tn5Rpp4ra8+NZj_se+w@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
On Mon, May 4, 2026 at 8:09 PM Euler Taveira <euler(at)eulerto(dot)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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Amit Kapila | 2026-05-05 05:25:44 | Re: Proposal: Conflict log history table for Logical Replication |
| Previous Message | Amit Kapila | 2026-05-05 04:57:47 | Re: Include schema-qualified names in publication error messages. |