Re: Wrong result for whole-row IS NOT NULL with dropped columns

Hi,

I also had a look at v1, and I agree with Sho that it looks good. A few
results that may help with the test coverage and the backpatch question.

Tested against master 952bab8b74, Debian bookworm container, ./configure
--enable-cassert --enable-debug --enable-tap-tests.

Applying and building
- v1 applies cleanly with git am, and the build has no compiler warnings.
- make check: 243 tests, PASS.

Behavior on master without the patch
The problem is wider than the virtual generated column case. Every case
below gives a wrong answer on master and the right one with v1. The server
log had no warnings or leaks with the patch.

1. WHERE clause. With the table from the report (a int, b int GENERATED
ALWAYS AS (a * 2) VIRTUAL, dropped column), "SELECT count(*) FROM t WHERE t
IS NOT NULL" returns 0. The plan is "Result / One-Time Filter: false", so
all rows are lost silently. With v1 the filter is "((a IS NOT NULL) AND ((a
* 2) IS NOT NULL))" and the count is 2. I think this is the most serious
case, and a test for it would be good to have.

2. ROW(...)::type, with no generated column: a table (a int, dropme int, b
int) with dropme dropped. ROW(a, b)::ts IS NOT NULL is false for (1, 2). The
same happens with dropped columns at the start and at the end of the table,
and with a generic plan: PREPARE q(int) AS SELECT ROW($1, 2)::ts IS NOT NULL
returns false for q(1) under plan_cache_mode = force_generic_plan. A fully
constant ROW(1, 2)::ts is correct, because it is folded to a constant.

3. Composite types: CREATE TYPE ct AS (x int, y int, z int) followed by
ALTER TYPE ct DROP ATTRIBUTE y behaves the same way as a table.

4. A stored view. This one is a little different:

CREATE TABLE src (x int, y int, z int);
INSERT INTO src VALUES (1, NULL, 3), (1, 2, 3);
CREATE TYPE ct3 AS (x int, y int, z int);
CREATE VIEW v AS SELECT ROW(x, y, z)::ct3 IS NOT NULL AS nn FROM src;
CREATE VIEW v2 AS SELECT ROW(x, y, z)::ct3 AS r FROM src;
ALTER TYPE ct3 DROP ATTRIBUTE y;
SELECT nn FROM v; -- f, t
SELECT r IS NOT NULL FROM (SELECT r FROM v2 OFFSET 0) s; -- t, t

Here the stored RowExpr still has the old Var src.y for the dropped
attribute, not a null Const, so the planner and the executor disagree for
the first row. With v1 both give t, t, and EXPLAIN shows "((src.x IS NOT
NULL) AND (src.z IS NOT NULL))". So checking attisdropped in the tuple
descriptor, as v1 does, is the right approach; looking for a null Const
would not cover this case.

5. Other places where the same folding shows up: a CHECK (t IS NOT NULL)
constraint added NOT VALID rejects a valid row, and a whole-row test on a
partitioned parent with a dropped column returns false while the same test
on the partition (which has no dropped column) returns true.

Cases that are already correct on master, and stay correct with v1: a table
with a single live column, a nested row (ROW(1, ROW(1, 2)::ts) IS NOT NULL),
and a domain over the composite type (the CoerceToDomain is not simplified).

Code
- Changing the early "return makeBoolConst(false, false)" into "refuted =
true; break;" is needed so that the tuple descriptor is released. Looks
right to me.
- The "i < tupdesc->natts" check looks defensive only, since a RowExpr of a
named type has one argument per attribute, dropped ones included. Fine to
keep.
- Tests: the two new tests cover the generated column and the cast. Maybe
add the WHERE case from item 1 (the plan with One-Time Filter: false is
easy to check with EXPLAIN), and the ALTER TYPE ... DROP ATTRIBUTE case.

Backpatch
- v1 does not apply to REL_18_STABLE or REL_17_STABLE. The conflict is in
the third clauses.c hunk, which touches code that exists only on master.
- I think the same fix works on older branches, since the ROW()::type path
from item 2 is not related to virtual generated columns. On 17,
TupleDescAttr() has to be used instead of TupleDescCompactAttr(), and the
generated_virtual test applies only to 18 and later. I prepared that port
for REL_17_STABLE but could not finish building and testing it, so I
cannot confirm yet that it works there.

Thanks for working on this.

Regards,
Joao Rito