MIME-Version: 1.0
References: 
 <CAHg+QDfTh3UbB-Ed--o2Bd=SBDJoEiG-qp3C0+ETDibF63y=dw@mail.gmail.com>
 <48110911-4707-4945-8d19-6afc9829fd1f@app.fastmail.com>
In-Reply-To: <48110911-4707-4945-8d19-6afc9829fd1f@app.fastmail.com>
From: SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>
Date: Mon, 4 May 2026 22:16:26 -0700
Message-ID: 
 <CAHg+QDdPuDskxeSvL1Y+pSkEHmycBv+tn5Rpp4ra8+NZj_se+w@mail.gmail.com>
Subject: Re: [Patch] Omit virtual generated columns from test_decoding output
To: Euler Taveira <euler@eulerto.com>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="000000000000daabe006510b267c"
Archived-At: 
 <https://www.postgresql.org/message-id/CAHg%2BQDdPuDskxeSvL1Y%2BpSkEHmycBv%2Btn5Rpp4ra8%2BNZj_se%2Bw%40mail.gmail.com>
Precedence: bulk

--000000000000daabe006510b267c
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Mon, May 4, 2026 at 8:09=E2=80=AFPM Euler Taveira <euler@eulerto.com> wr=
ote:

> 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 an=
d
> 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 tes=
t.
> > 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 fo=
r
> this
> change. There are some concerns about the total test execution time. Do y=
ou
> 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

--000000000000daabe006510b267c
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi,</div><br><div class=3D"gmail_quote gm=
ail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Mon, May 4, 2=
026 at 8:09=E2=80=AFPM Euler Taveira &lt;<a href=3D"mailto:euler@eulerto.co=
m">euler@eulerto.com</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quo=
te" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204=
);padding-left:1ex">On Mon, May 4, 2026, at 10:11 PM, SATYANARAYANA NARLAPU=
RAM wrote:<br>
&gt;<br>
&gt; Virtual generated columns are not stored on disk, so heap_getattr() in=
<br>
&gt; tuple_to_stringinfo() always returned NULL for them, producing<br>
&gt; misleading output such as<br>
&gt;<br>
&gt;=C2=A0 =C2=A0table public.t: INSERT: a[integer]:1 b[integer]:10 c[integ=
er]:null<br>
&gt;<br>
&gt; even though the user could observe a non-null value via SELECT.=C2=A0 =
Stored<br>
&gt; generated columns continue to be emitted as before because their value=
s<br>
&gt; do live in the heap tuple.<br>
&gt;<br>
<br>
I wouldn&#39;t say misleading but expected. Logical decoding relies on WAL =
and<br>
virtual generated columns are not stored in the WAL.<br>
<br>
&gt; This matches the pgoutput&#39;s logicalrep_should_publish_column()<br>
&gt; which never publishes virtual generated columns. Added a regression te=
st.<br>
&gt; Please find the patch attached.<br>
&gt;<br>
<br>
There is no guarantee that test_decoding should match the pgoutput. </block=
quote><div><br></div><div>Agreed, not trying to keep them in sync but givin=
g as a reference.</div><div><br></div><div>=C2=A0</div><blockquote class=3D=
"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(2=
04,204,204);padding-left:1ex">I agree that<br>
test_decoding shouldn&#39;t output virtual generated columns. The problem i=
s that it<br>
already does it. I&#39;m afraid that removing it should break existing appl=
ications.<br>
(I heard that some solutions rely on test_decoding for CDC.) Should we chan=
ge it<br>
as you proposed or add an option to put it back to keep the old behavior?<b=
r></blockquote><div><br></div><div>It is emitting null, I am not sure if it=
 is meaningful for the consumers to consume this or</div><div>have taken de=
pendency on this. Adding an extra option isn&#39;t an overkill for this? I =
am open=C2=A0</div><div>to this idea if others feel the same.</div><div><br=
></div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0=
px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I didn&#39;t review your patch but I noticed that there is a new test file =
for this<br>
change. There are some concerns about the total test execution time. Do you=
<br>
really need to include this test? If so, should you combine it with an exis=
ting<br>
test file?</blockquote><div><br></div><div>Fair concern, I moved the tests =
to=C2=A0ddl.sql.=C2=A0 Please find the attached v2 patch.</div><div><br></d=
iv><div>Thanks,</div><div>Satya</div></div></div>

--000000000000daabe006510b267c--