Fix tuple deformation with virtual generated NOT NULL columns
Hi,
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)
```
Here, b was inserted as 20, but select only returned 0.
I think the problem is in finding the first non-guaranteed attribute where virtual generated attributes are not considered:
```
for (int i = 0; i < tupdesc->natts; i++)
{
CompactAttribute *cattr = TupleDescCompactAttr(tupdesc, i);
/*
* Find the highest attnum which is guaranteed to exist in all tuples
* in the table. We currently only pay attention to byval attributes
* to allow additional optimizations during tuple deformation.
*/
if (firstNonGuaranteedAttr == tupdesc->natts &&
(cattr->attnullability != ATTNULLABLE_VALID || !cattr->attbyval ||
cattr->atthasmissing || cattr->attisdropped || cattr->attlen <= 0))
firstNonGuaranteedAttr = i;
```
To fix this, we should consider virtual generated attributes as non-guaranteed. The tricky part is that cattr->attgenerated is only a boolean and cannot distinguish virtual generated from stored. So we have to further check TupleDescAttr(tupdesc, i)->attgenerated. In the patch, I changed the check as follows:
```
if (firstNonGuaranteedAttr == tupdesc->natts &&
(cattr->attnullability != ATTNULLABLE_VALID || !cattr->attbyval ||
cattr->atthasmissing || cattr->attisdropped || cattr->attlen <= 0 ||
(cattr->attgenerated &&
TupleDescAttr(tupdesc, i)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)))
firstNonGuaranteedAttr = i;
```
This way, we only check TupleDescAttr(tupdesc, i)->attgenerated when needed.
See the attached patch for details. I also added a regression test case to cover this fix. With the fix, select now returns correct values:
```
evantest=# select a, g, b from t;
a | g | b
----+----+----
10 | 11 | 20
(1 row)
```
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v1-0001-Fix-tuple-deformation-with-virtual-generated-NOT-.patchapplication/octet-stream; name=v1-0001-Fix-tuple-deformation-with-virtual-generated-NOT-.patch; x-unix-mode=0644Download+20-3
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/
Attachments:
v2-0001-Fix-tuple-deformation-with-virtual-generated-NOT-.patchapplication/octet-stream; name=v2-0001-Fix-tuple-deformation-with-virtual-generated-NOT-.patch; x-unix-mode=0644Download+22-4
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/