Re: Show expression of virtual columns in error messages

Hi,

I received an offlist review from Andrew and he found a behaviour
regression in COPY. With v7, COPY FROM no longer enforces a CHECK
constraint on a virtual generated column, while INSERT still does:

CREATE TABLE cpy (a int, b int GENERATED ALWAYS AS (a * 2) VIRTUAL
CHECK (b < 10));
INSERT INTO cpy VALUES (100); -- fails, as it should
COPY cpy (a) FROM PROGRAM 'printf "100\n"';

On master the COPY fails with the check constraint violation, but with
v7 it succeeds and the table contains (100, 200). The reason is that v7
removed the expand_generated_columns_in_expr() call from ExecRelCheck()
and relied on ExecComputeVirtualGenerated() having filled the slot
before, but CopyFrom() never calls it, so the check expression sees
NULL for the virtual column and passes. He also noticed that a dump and
restore of the regression database fails with v7 on "gtest20c", which
has the same cause: the whole-row check (gtest20c IS NOT NULL) sees the
virtual column as NULL during COPY and rejects valid rows.

While fixing this I found some other issues with the v7 approach of
storing the computed values directly in the input slot. Depending on
whether the slot was already materialized, the computed values could end
up written to disk, and on UPDATE, v7 only computed the virtual columns
that depend on the updated columns, so a whole-row check failed on
UPDATE t SET c = 2 for a perfectly valid row, since the other virtual
columns were NULL in the slot.

The attached v8 changes the approach. The virtual generated columns are
now computed inside ExecConstraints() into a separate slot, and the
constraints are checked against that slot. The input slot is never
modified, so nothing reaches the disk, and since every caller goes
through ExecConstraints() this covers INSERT, UPDATE, ON CONFLICT,
MERGE, COPY FROM and logical replication apply. Following Tom's
suggestion, the values are computed even if there are no constraints, so
a row whose virtual column values cannot be computed is rejected on
write.

ExecBuildSlotValueDescription() shows "virtual" again by default, since
some callers (e.g. logical replication conflicts) build the description
from tuples read from disk, where the virtual columns are NULL. Only the
check and not-null violation messages, which use the computed slot, show
the actual values.

I've notice some consequences of computing the virtual columns on every
write that I think are worth sharing:

1. ALTER TABLE ... ADD COLUMN ... VIRTUAL and ALTER COLUMN ... SET
EXPRESSION don't evaluate the new expression against existing rows, so
a table can still have rows whose virtual column values cannot be
computed:

CREATE TABLE t1 (a int, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
INSERT INTO t1 VALUES (1000000000);
ALTER TABLE t1 ALTER COLUMN b SET EXPRESSION AS (a * 10);

pg_dump works fine, but restoring the dump fails with "integer out of
range" on the COPY and the table is restored empty. On master the
restore works and the error only happens on SELECT. The same applies to
the initial table synchronization of logical replication, as if the
subscriber table has a virtual column that cannot be computed for some
publisher row, the table sync fails and keeps retrying.

2. On logical replication apply, if a virtual column that only exists
on the subscriber cannot be computed, the apply worker now errors out,
even if there is no constraint on the table.

3. All virtual columns are computed at the start of ExecConstraints(),
so an error evaluating a generation expression now takes precedence
over a not-null violation on a regular column, a check violation, a
unique violation or a "no partition found" error. For example, a row
with a NULL in a NOT NULL column and an overflowing virtual column now
reports "integer out of range".

4. With INSERT ... ON CONFLICT, the error is raised even if the row
conflicts and would never be stored, as already happens with check
constraints.

Most of these consequences stem from virtual generated columns being
computed on the write path, which risks breaking existing workflows.
While I think that this is a valuable feature, I'm wondering if it's
still worth pursuing in this form. Thoughts?

--
Matheus Alcantara
EDB: https://www.enterprisedb.com