Hi, jian he <jian.universal...@gmail.com> 于2025年3月6日周四 21:44写道：
> On Wed, Feb 26, 2025 at 3:01 PM ego alter <xunengz...@gmail.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. > > Thank you for your explanation! This name seems to be more clear for its usage. > > > 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. >
