Re: support virtual generated column not null constraint

Hi

I encountered an issue when trying to add a virtual column to an existing
table using the ALTER command. The operation fails even though the existing
data ensures that the virtual column's value will never be NULL. However,
if I define the same virtual column while creating the table and then
insert the same data, it works as expected.

For example
This scenario fails
1. CREATE TABLE person (
id INT GENERATED BY DEFAULT AS IDENTITY,
first_name VARCHAR(50) NOT NULL,
last_name VARCHAR(50) NOT NULL
);

2. INSERT INTO person (first_name, last_name)
VALUES ('first', 'last');

3. ALTER TABLE person
ADD COLUMN full_name VARCHAR(100) GENERATED ALWAYS AS (first_name || ' ' ||
last_name) VIRTUAL;

The above ALTER command works if I use STORED instead. Also if I define
virtual generated column while creating table it works with same data.

1. CREATE TABLE person (
id INT GENERATED BY DEFAULT AS IDENTITY,
first_name VARCHAR(50) NOT NULL,
last_name VARCHAR(50) NOT NULL,
full_name VARCHAR(100) NOT NULL GENERATED ALWAYS AS (first_name || ' '
|| last_name) VIRTUAL
);

2. INSERT INTO person(first_name, last_name)

VALUES ('first', 'last');

On Thu, Mar 6, 2025 at 7:15 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:

> On Wed, Feb 26, 2025 at 3:01 PM ego alter <xunengzhou(at)gmail(dot)com> wrote:
> >
> > Hi, I’ve had a chance to review the patch. As I am still getting
> familiar with executor part, questions and feedbacks could be relatively
> trivial.
> >
> > There are two minor issues i want to discuss:
> > 1. The way of caching constraint-checking expr for virtual generated not
> null
> > The existing array for caching constraint-checking expr is
> > /* array of constraint-checking expr states */
> > ExprState **ri_ConstraintExprs;
> >
> > the proposed changes for virtual generated not null in patch
> > + /* array of virtual generated not null constraint-checking expr
> states */
> > + ExprState **ri_VirGeneratedConstraintExprs;
> > /*
> > Could you explain the rationale behind adding this new field instead of
> reusing ri_ConstraintExprs? The comment suggests it’s used specifically for
> not null constraint-checking, but could it be extended to handle other
> constraints in the future as well? I assume one benefit is that it
> separates the handling of normal constraints from virtual ones, but I'd
> like to appreciate more context on the decision.
> >
>
> it's a cache mechanism, just like ResultRelInfo.ri_ConstraintExprs
> you can see comments in ExecRelCheck
> /*
> * If first time through for this result relation, build expression
> * nodetrees for rel's constraint expressions. Keep them in the
> per-query
> * memory context so they'll survive throughout the query.
> */
>
> for example:
> create table t(a int, check (a > 3));
> insert into t values (4),(3);
> we need to call ExecRelCheck for values 4 and 3.
> The second time ExecRelCheck called for values 3, if
> ri_ConstraintExprs is not null then
> we didn't need to call ExecPrepareExpr again for the same check
> constraint expression.
>
> + ExprState **ri_VirGeneratedConstraintExprs;
> is specifically only for virtual generated columns not null constraint
> evaluation.
> Anyway, I renamed it to ri_GeneratedNotNullExprs.
>
> If you want to extend cache for other constraints in the future,
> you can add it to struct ResultRelInfo.
> Currently struct ResultRelInfo, we have ri_GeneratedExprsU,
> ri_GeneratedExprsI for generated expressions;
> ri_ConstraintExprs for check constraints.
>
>
> > 2. The naming of variable gen_virtualnn.
> > Gen_virtualnn looks confusing at first glance. Checkconstr seems to be
> more straightforward.
> >
> thanks. I changed it to exprstate.
>
> new patch attached.
> 0001 not null for virtual generated columns, some refactoring happened
> within ExecConstraints
> to avoid duplicated error handling code.
>
> 0002 and 0003 is the domain for virtual generated columns. domain can
> have not-null constraints,
> this is built on top of 0001, so I guess posting here for review should be
> fine?
>
> currently it works as intended. but add a virtual generated column
> with domain_with_constraints
> requires table rewrite. but some cases table scan should just be enough.
> some case we do need table rewrite.
> for example:
> create domain d1 as int check(value > random(min=>11::int, max=>12));
> create domain d2 as int check(value > 12);
> create table t(a int);
> insert into t select g from generate_series(1, 10) g;
> alter table t add column b d1 generated always as (a+11) virtual; --we
> do need table rewrite in phase 3.
> alter table t add column c d2 generated always as (a + 12) virtual;
> --we can only do table scan in phase 3.
>