On Wed, Feb 26, 2025 at 3:01 PM ego alter <xunengzhou@gmail.com> wrote:
>
> Hi, I’ve had a chance to review the patch. As I am still getting familiar with executor part, questions and
feedbackscould 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
suggestsit’s used specifically for not null constraint-checking, but could it be extended to handle other constraints
inthe future as well? I assume one benefit is that it separates the handling of normal constraints from virtual ones,
butI'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.
