Fix tuple deformation with virtual generated NOT NULL columns

While testing "Optimize tuple deformation”, I found a bug:
```
evantest=# create table t (a int not null,
evantest(# g int generated always as (a+1) virtual not null,
evantest(# b int not null);
CREATE TABLE
evantest=# insert into t (a, b) values (10, 20);
INSERT 0 1
evantest=# select a, g, b from t;
 a  | g  | b
----+----+---
 10 | 11 | 0
(1 row)
```

Nice catch! I can reproduce this bug on master. Some comments about the fix:

I find that a virtual generated column is stored as a null in heap tuple, so I think
we should stop setting 'attcacheoff' when we see a virtual generated column in
TupleDescFinalize(), or we will set wrong 'attcacheoff' value. But it seems that
we don't use these wrong value because we can only use 'attcacheoff' up until
the first NULL.

--
Regards,
ChangAo Chen

On Jun 4, 2026, at 17:32, cca5507 <cca5507@qq.com> wrote:

While testing "Optimize tuple deformation”, I found a bug:
```
evantest=# create table t (a int not null,
evantest(# g int generated always as (a+1) virtual not null,
evantest(# b int not null);
CREATE TABLE
evantest=# insert into t (a, b) values (10, 20);
INSERT 0 1
evantest=# select a, g, b from t;
a | g | b
----+----+---
10 | 11 | 0
(1 row)
```

Nice catch! I can reproduce this bug on master. Some comments about the fix:

I find that a virtual generated column is stored as a null in heap tuple, so I think
we should stop setting 'attcacheoff' when we see a virtual generated column in
TupleDescFinalize(), or we will set wrong 'attcacheoff' value. But it seems that
we don't use these wrong value because we can only use 'attcacheoff' up until
the first NULL.

--
Regards,
ChangAo Chen

Hi ChangAo,

Thanks for your review. Please see v2 that addressed your comment.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Thanks for your review. Please see v2 that addressed your comment.

The v2 patch LGTM.

--
Regards,
ChangAo Chen

On Thu, 4 Jun 2026 at 17:57, Chao Li <li.evan.chao@gmail.com> wrote:

While testing "Optimize tuple deformation”, I found a bug:

I think the problem is in finding the first non-guaranteed attribute where virtual generated attributes are not considered:

Thanks for the report and fix. I pushed a slightly adjusted version.
Form_pg_attribute.attgenerated is '\0' for non-generated columns, so
there's no point in checking cattr->attgenerated as well as that.

I also added an Assert() to help catch any other reason that the
guaranteed column gets set incorrectly again in TupleDescFinalize().

David

On Jun 6, 2026, at 12:50, David Rowley <dgrowleyml@gmail.com> wrote:

On Thu, 4 Jun 2026 at 17:57, Chao Li <li.evan.chao@gmail.com> wrote:

While testing "Optimize tuple deformation”, I found a bug:

I think the problem is in finding the first non-guaranteed attribute where virtual generated attributes are not considered:

Thanks for the report and fix. I pushed a slightly adjusted version.

Thanks for pushing.

Form_pg_attribute.attgenerated is '\0' for non-generated columns, so
there's no point in checking cattr->attgenerated as well as that.

I was trying to avoid unconditionally fetching TupleDescAttr(tupdesc, i), since cattr->attgenerated is already available and most columns won't be virtual generated columns. But I was probably overthinking the performance angle here. TupleDescFinalize() runs only when finalizing a tuple descriptor, not during per-tuple deformation. I agree your adjusted version is cleaner.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi,

On 2026-06-06 16:50:29 +1200, David Rowley wrote:

On Thu, 4 Jun 2026 at 17:57, Chao Li <li.evan.chao@gmail.com> wrote:

While testing "Optimize tuple deformation”, I found a bug:

I think the problem is in finding the first non-guaranteed attribute where virtual generated attributes are not considered:

Thanks for the report and fix. I pushed a slightly adjusted version.
Form_pg_attribute.attgenerated is '\0' for non-generated columns, so
there's no point in checking cattr->attgenerated as well as that.

I also added an Assert() to help catch any other reason that the
guaranteed column gets set incorrectly again in TupleDescFinalize().

Seems like a test for some of this would be good too?

Greetings,

Andres Freund

On Jun 8, 2026, at 21:01, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2026-06-06 16:50:29 +1200, David Rowley wrote:

On Thu, 4 Jun 2026 at 17:57, Chao Li <li.evan.chao@gmail.com> wrote:

While testing "Optimize tuple deformation”, I found a bug:

I think the problem is in finding the first non-guaranteed attribute where virtual generated attributes are not considered:

Thanks for the report and fix. I pushed a slightly adjusted version.
Form_pg_attribute.attgenerated is '\0' for non-generated columns, so
there's no point in checking cattr->attgenerated as well as that.

I also added an Assert() to help catch any other reason that the
guaranteed column gets set incorrectly again in TupleDescFinalize().

Seems like a test for some of this would be good too?

Greetings,

Andres Freund

Okay, I tried to add a test matching my repro. With this test in place, I reverted the fix and ran make check; it failed by hitting the Assert David added:
```
TRAP: failed Assert("first_null_attr(tup->t_bits, natts) >= firstNullAttr"), File: "execTuples.c", Line: 1083, PID: 65804
0 postgres 0x0000000104e883b8 ExceptionalCondition + 216
1 postgres 0x00000001048e5ebc slot_deform_heap_tuple + 456
2 postgres 0x00000001048e2a18 tts_buffer_heap_getsomeattrs + 112
3 postgres 0x00000001048c5044 slot_getsomeattrs + 68
4 postgres 0x00000001048b97a4 ExecInterpExpr + 416
5 postgres 0x00000001048b8ea0 ExecInterpExprStillValid + 76
6 postgres 0x000000010492ec68 ExecEvalExprNoReturn + 44
7 postgres 0x000000010492ec28 ExecEvalExprNoReturnSwitchContext + 48
8 postgres 0x000000010492eb20 ExecProject + 72
9 postgres 0x000000010492e680 ExecScanExtended + 288
10 postgres 0x000000010492d7cc ExecSeqScanWithProject + 220
11 postgres 0x00000001048db508 ExecProcNodeFirst + 92
12 postgres 0x00000001048d1eb8 ExecProcNode + 60
```

See the attached patch for details.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Tue, 9 Jun 2026 at 14:20, Chao Li <li.evan.chao@gmail.com> wrote:

On Jun 8, 2026, at 21:01, Andres Freund <andres@anarazel.de> wrote:
Seems like a test for some of this would be good too?

Okay, I tried to add a test matching my repro. With this test in place, I reverted the fix and ran make check; it failed by hitting the Assert David added:
```
TRAP: failed Assert("first_null_attr(tup->t_bits, natts) >= firstNullAttr"), File: "execTuples.c", Line: 1083, PID: 65804

I was on the fence about that test as I felt it was mostly
highlighting that there was a bug once with that particular case, and
didn't feel like it did much to prevent future omissions from
TupleDescFinalize(). I felt the Assert would help highlight future
stuff.

However, I don't object to adding the test if others feel it's
worthwhile, but on looking at the patch, there are a couple of things
that stand out.

1. Nothing has been done for the comment at the top of the file that
says "-- keep these tests aligned with generated_stored.sql". It
looks like there have been quite a few commits already which have
neglected this. Maybe that means we should do away with the comment
rather than try to align the two.
2. I can't quite figure out the pattern in these tests for dropping vs
not dropping the tables at the end of the test. Many tests do DROP
TABLE and a large number of others don't bother. What's meant to be
happening here?

I've added Peter as I think it was his intention to keep these tests
aligned with generated_stored.sql.

David

On Fri, 12 Jun 2026 at 16:26, David Rowley <dgrowleyml@gmail.com> wrote:

However, I don't object to adding the test if others feel it's
worthwhile, but on looking at the patch, there are a couple of things
that stand out.

1. Nothing has been done for the comment at the top of the file that
says "-- keep these tests aligned with generated_stored.sql". It
looks like there have been quite a few commits already which have
neglected this. Maybe that means we should do away with the comment
rather than try to align the two.

I pushed the patch to add the new test after modifying it to also
include the change in the generated_stored test, but with it commented
out. This is the method that 83ea6c540 introduced.

Since a few commits have already put these files out of sync, maybe we
should consider having a single test file that we include with \i
after doing something like:

\set generated_type STORED
\set test_generated_stored true
\set test_generated_virtual false
\i generated_generic.sql

and have generated_generic.sql use \if test_generated_(stored|virtual)
as and when required.

2. I can't quite figure out the pattern in these tests for dropping vs
not dropping the tables at the end of the test. Many tests do DROP
TABLE and a large number of others don't bother. What's meant to be
happening here?

I added the DROP TABLE too, as I didn't see any reason mentioned
anywhere that they should be kept.

David

David Rowley <dgrowleyml@gmail.com> writes:

2. I can't quite figure out the pattern in these tests for dropping vs
not dropping the tables at the end of the test. Many tests do DROP
TABLE and a large number of others don't bother. What's meant to be
happening here?

I added the DROP TABLE too, as I didn't see any reason mentioned
anywhere that they should be kept.

A very rough rule of thumb is that we leave tables around if they
might be interesting for the pg_upgrade tests (which try to upgrade
the ending state of the core regression tests).

regards, tom lane

On Thu, 18 Jun 2026 at 10:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:

set jit = 1;
set jit_above_cost = 0;
set jit_optimize_above_cost = 1000;
CREATE TABLE gtest21c (a int NOT NULL, b int GENERATED ALWAYS AS (a * 2) VIRTUAL NOT NULL, c int NOT NULL);
INSERT INTO gtest21c (a, c) VALUES (10, 42);
table gtest21c;

I conclude that something in the JIT code for tuple formation
is unaware of virtual generated columns.

Just starting to look now, but I suspect that this code in
llvmjit_deform.c needs to be updated now that we have virtual
generated columns.

/*
* If the column is declared NOT NULL then it must be present in every
* tuple, unless there's a "missing" entry that could provide a
* non-NULL value for it. That in turn guarantees that the NULL bitmap
* - if there are any NULLable columns - is at least long enough to
* cover columns up to attnum.
*
* Be paranoid and also check !attisdropped, even though the
* combination of attisdropped && attnotnull combination shouldn't
* exist.
*/
if (att->attnullability == ATTNULLABLE_VALID &&
!att->atthasmissing &&
!att->attisdropped)
guaranteed_column_number = attnum;

David

On Thu, 18 Jun 2026 at 11:06, David Rowley <dgrowleyml@gmail.com> wrote:

Just starting to look now, but I suspect that this code in
llvmjit_deform.c needs to be updated now that we have virtual
generated columns.

I've not fully processed all this code yet. I've still not worked out
why the loop that sets guaranteed_column_number doesn't break when it
finds something non-guaranteed.

Here's a draft patch I was experimenting with. It seems to fix the
issue. I need to spend more time to check it's correct.

David

On Jun 18, 2026, at 07:37, David Rowley <dgrowleyml@gmail.com> wrote:

On Thu, 18 Jun 2026 at 11:06, David Rowley <dgrowleyml@gmail.com> wrote:

Just starting to look now, but I suspect that this code in
llvmjit_deform.c needs to be updated now that we have virtual
generated columns.

I've not fully processed all this code yet. I've still not worked out
why the loop that sets guaranteed_column_number doesn't break when it
finds something non-guaranteed.

Here's a draft patch I was experimenting with. It seems to fix the
issue. I need to spend more time to check it's correct.

David
<fix_jit_deform_for_virtual_generated_cols.patch>

I was not aware of the JIT code before. I think the good thing is that the new test uncovered this JIT bug.

I tested the fix, and it seems to work. While tracing the code, I wondered about this part:
```
-		if (att->attnullability == ATTNULLABLE_VALID &&
-			!att->atthasmissing &&
-			!att->attisdropped)
+		if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
+			break;
+
+		if (catt->attnullability == ATTNULLABLE_VALID &&
+			!catt->atthasmissing &&
+			!catt->attisdropped)
 			guaranteed_column_number = attnum;
```

When computing guaranteed_column_number, I think we can just skip the virtual generated column rather than break. Using the test from Tom’s email:
```
CREATE TABLE gtest21c (a int NOT NULL, b int GENERATED ALWAYS AS (a * 2) VIRTUAL NOT NULL, c int NOT NULL);
```

Here, “c" is a real NOT NULL column, so we can skip “b" and let “c" set guaranteed_column_number. This matters for the later check:
```
/*
* Check if it is guaranteed that all the desired attributes are available
* in the tuple (but still possibly NULL), by dint of either the last
* to-be-deformed column being NOT NULL, or subsequent ones not accessed
* here being NOT NULL. If that's not guaranteed the tuple headers natt's
* has to be checked, and missing attributes potentially have to be
* fetched (using slot_getmissingattrs().
*/
if ((natts - 1) <= guaranteed_column_number)
{
```

If we break at “b", then this check goes to the else branch and invokes the maxatt check plus slot_getmissingattrs(), even though the later NOT NULL “c" proves that the tuple has enough attributes.

Otherwise, the fix looks good to me.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Thu, 18 Jun 2026 at 14:36, Chao Li <li.evan.chao@gmail.com> wrote:

I tested the fix, and it seems to work. While tracing the code, I wondered about this part:
```
-               if (att->attnullability == ATTNULLABLE_VALID &&
-                       !att->atthasmissing &&
-                       !att->attisdropped)
+               if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
+                       break;
+
+               if (catt->attnullability == ATTNULLABLE_VALID &&
+                       !catt->atthasmissing &&
+                       !catt->attisdropped)
guaranteed_column_number = attnum;
```

When computing guaranteed_column_number, I think we can just skip the virtual generated column rather than break. Using the test from Tom’s email:

Yeah, I was confused at first as I'd done a similar optimisation in
the non-JIT deform code, but there "guaranteed" means guaranteed to be
present in the tuple data, whereas with the JIT code it means
guaranteed in the tuple data or its NULL bitmap.

I've attached v2 which includes a test that exercises deforming with
tuples which have various natts counts. I propose to backpatch
1f7dfe8c8 to v18 before applying the attached to master and v18.

David

On Jun 18, 2026, at 13:18, David Rowley <dgrowleyml@gmail.com> wrote:

On Thu, 18 Jun 2026 at 14:36, Chao Li <li.evan.chao@gmail.com> wrote:

I tested the fix, and it seems to work. While tracing the code, I wondered about this part:
```
-               if (att->attnullability == ATTNULLABLE_VALID &&
-                       !att->atthasmissing &&
-                       !att->attisdropped)
+               if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
+                       break;
+
+               if (catt->attnullability == ATTNULLABLE_VALID &&
+                       !catt->atthasmissing &&
+                       !catt->attisdropped)
guaranteed_column_number = attnum;
```

When computing guaranteed_column_number, I think we can just skip the virtual generated column rather than break. Using the test from Tom’s email:

Yeah, I was confused at first as I'd done a similar optimisation in
the non-JIT deform code, but there "guaranteed" means guaranteed to be
present in the tuple data, whereas with the JIT code it means
guaranteed in the tuple data or its NULL bitmap.

I've attached v2 which includes a test that exercises deforming with
tuples which have various natts counts. I propose to backpatch
1f7dfe8c8 to v18 before applying the attached to master and v18.

David
<fix_jit_deform_for_virtual_generated_cols_v2.patch>

This version looks good to me. Only a small comment:

```
+-- try adding a virtual generated column to an existing table with tuples,
+-- then try adding an atthasmissing column before adding a normal nullable
+-- column.
+--CREATE TABLE gtest21d (a int NOT NULL);
+--INSERT INTO gtest21d (a) VALUES(10);
+--ALTER TABLE gtest21d ADD COLUMN b INT GENERATED ALWAYS AS (a * 10) VIRTUAL NOT NULL;
+--SELECT * FROM gtest21c ORDER BY a;
+--INSERT INTO gtest21d (a) VALUES(20);
+--ALTER TABLE gtest21d ADD COLUMN c INT NOT NULL DEFAULT 1234;
+--SELECT * FROM gtest21c ORDER BY a;
+--ALTER TABLE gtest21d ADD COLUMN d INT;
+--INSERT INTO gtest21d (a, c, d) VALUES(30, 12345, 100);
+--SELECT * FROM gtest21c ORDER BY a;
+--DROP TABLE gtest21d;
```

I don’t know why you added these commented SQL statements, I guess you have your reason. The problem is, in the 3 SELECTs, gtest21c should probably be gtest21d.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Thu, 18 Jun 2026 at 17:56, Chao Li <li.evan.chao@gmail.com> wrote:

I don’t know why you added these commented SQL statements, I guess you have your reason. The problem is, in the 3 SELECTs, gtest21c should probably be gtest21d.

This relates to the "-- keep these tests aligned with
generated_stored.sql" at the top of the file.

I mentioned in [1]/messages/by-id/CAApHDvrJBXEhet4=Es_wHBKdv5PCV5OGCaJOSmJexeaFqfmUHA@mail.gmail.com;

I pushed the patch to add the new test after modifying it to also
include the change in the generated_stored test, but with it commented
out. This is the method that 83ea6c540 introduced.

and this is meant to follow the same pattern.

I've attached a version with the commented table names fixed. Thanks
for looking.

David

[1]: /messages/by-id/CAApHDvrJBXEhet4=Es_wHBKdv5PCV5OGCaJOSmJexeaFqfmUHA@mail.gmail.com

On Jun 18, 2026, at 14:27, David Rowley <dgrowleyml@gmail.com> wrote:

On Thu, 18 Jun 2026 at 17:56, Chao Li <li.evan.chao@gmail.com> wrote:

I don’t know why you added these commented SQL statements, I guess you have your reason. The problem is, in the 3 SELECTs, gtest21c should probably be gtest21d.

This relates to the "-- keep these tests aligned with
generated_stored.sql" at the top of the file.

I mentioned in [1];

I pushed the patch to add the new test after modifying it to also
include the change in the generated_stored test, but with it commented
out. This is the method that 83ea6c540 introduced.

and this is meant to follow the same pattern.

Thanks for the explanation.

I've attached a version with the commented table names fixed. Thanks
for looking.

David

[1] /messages/by-id/CAApHDvrJBXEhet4=Es_wHBKdv5PCV5OGCaJOSmJexeaFqfmUHA@mail.gmail.com
<fix_jit_deform_for_virtual_generated_cols_v3.patch>

This version feels all set.

I also tried to build and run tests, all passed.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Thu, 18 Jun 2026 at 18:46, Chao Li <li.evan.chao@gmail.com> wrote:

This version feels all set.

Thanks for looking. I've pushed and backpatched this now.

For the v18 version, I included the tests added by 1f7dfe8c8 in the
same commit. I did it that way so as not to briefly introduce a test
that would fail into REL_18_STABLE.

David

David Rowley <dgrowleyml@gmail.com> writes:

For the v18 version, I included the tests added by 1f7dfe8c8 in the
same commit. I did it that way so as not to briefly introduce a test
that would fail into REL_18_STABLE.

+1, doing it the other way would create a landmine for git-bisect
testing.

regards, tom lane