On Thu, Jul 30, 2026 at 9:32 PM Bharath Rupireddy
<[email protected]> wrote:
>
> Hi,
>
> On Thu, May 7, 2026 at 9:31 PM Fujii Masao <[email protected]> wrote:
> >
> > > Fair concern, I moved the tests to ddl.sql. Please find the attached v2
> > > patch.
> >
> > Seems you forgot to attached the patch.
>
> I agree we must fix this for test_decoding. Here's my take.
>
> When a column holding a genuine NULL value gets decoded, it shows up
> as null in the output. But a virtual generated column also shows up as
> null, so the two are hard to tell apart in the test_decoding output
> today.
>
> table public.t: INSERT: a[integer]:1 b[integer]:null c[integer]:null
> d[integer]:100
>
> Here b is a genuine NULL and c is a virtual generated column whose
> value is 10 (for example), but both show up as null.
>
> That gives me two reasons to +1 this patch:
>
> 1/ It's hard to distinguish in the test_decoding output whether a
> column is a genuine NULL or a virtual generated column. For example:
> 2/ The pgoutput already skips virtual generated columns in
> logicalrep_should_publish_column().
>
> I quickly reviewed the v3 patch and it looks good to me. However,
> pgindent was not happy, so I ran it, tweaked the comments and commit
> message a bit, ran the tests, and attached a v4 patch. Please have a
> look.
>
> I prefer to back-patch this through PG18, where virtual generated
> columns were introduced (commit 83ea6c54025).
I agree with the proposed change. It's quite confusing to distinguish
between a genuine NULL value and a NULL value in a virtual generated
column, as Bharath mentioned.
Here are some review comments:
+-- Check that virtual generated columns are omitted from the output (their
+-- values are not stored on disk so heap_getattr() would otherwise emit a
+-- wrong NULL), while stored generated columns are emitted normally.
How about rewriting it to:
+-- 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.
A similar change would be required to the comments in test_decoding.c.
---
+-- table with only virtual generated columns alongside the key
+CREATE TABLE gtest2 (
+ a int PRIMARY KEY,
+ b int GENERATED ALWAYS AS (a + 1) VIRTUAL,
+ c text GENERATED ALWAYS AS ('row-' || a::text) VIRTUAL
+);
+INSERT INTO gtest2 (a) VALUES (10), (20);
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,
NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
I don't think this test is needed as it doesn't improve test coverages.
---
As for backpatching, IIUC it's not a correctness bug nor causes a
server crash or data corruption. Given it might affect the existing
consumers using test_decoding, I think it should be only for HEAD.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
