MIME-Version: 1.0
References: 
 <CACJufxHArQysbDkWFmvK+D1TPHQWWTxWN15cMuUaTYX3xhQXgg@mail.gmail.com>
 <CABPTF7Vhmy-qGjB5XvpO-BXbf9bCR7BO5J548XzD2Bx7xOdkBg@mail.gmail.com>
 <CACJufxGof=Tb6-AP_UdDy8By4Oi_Cgutpnodw4RLW-zNJLK+9g@mail.gmail.com>
In-Reply-To: 
 <CACJufxGof=Tb6-AP_UdDy8By4Oi_Cgutpnodw4RLW-zNJLK+9g@mail.gmail.com>
From: Navneet Kumar <thanit3111@gmail.com>
Date: Fri, 7 Mar 2025 22:17:41 +0530
Message-ID: 
 <CANzA6spEUcRpsbP8Z_8Bwfuy8bVR_fb==ouYD5iLa7-bRbpsSw@mail.gmail.com>
Subject: Re: support virtual generated column not null constraint
To: jian he <jian.universality@gmail.com>
Cc: ego alter <xunengzhou@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000002cdfc5062fc362d7"
Archived-At: 
 <https://www.postgresql.org/message-id/CANzA6spEUcRpsbP8Z_8Bwfuy8bVR_fb%3D%3DouYD5iLa7-bRbpsSw%40mail.gmail.com>
Precedence: bulk

--0000000000002cdfc5062fc362d7
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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=E2=80=AFPM jian he <jian.universality@gmail.com=
> wrote:

> On Wed, Feb 26, 2025 at 3:01=E2=80=AFPM ego alter <xunengzhou@gmail.com> =
wrote:
> >
> > Hi, I=E2=80=99ve had a chance to review the patch.  As I am still getti=
ng
> 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 no=
t
> 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=E2=80=99s used specif=
ically 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 b=
e
> 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=3D>11::int, max=3D>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.
>

--0000000000002cdfc5062fc362d7
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Hi=C2=A0<div><br></div><div>I encountered an issue when tr=
ying to add a virtual column to an existing table using the <code>ALTER</co=
de> command. The operation fails even though the existing data ensures that=
 the virtual column&#39;s value will never be <code>NULL</code>. However, i=
f I define the same virtual column while creating the table and then insert=
 the same data, it works as expected.</div><div><br></div><div>For example<=
br>This scenario fails</div><div>1. CREATE TABLE person (<br>=C2=A0 =C2=A0 =
id INT GENERATED BY DEFAULT AS IDENTITY,<br>=C2=A0 =C2=A0 first_name VARCHA=
R(50) NOT NULL,<br>=C2=A0 =C2=A0 last_name VARCHAR(50) NOT NULL<br>);<br><b=
r>2. INSERT INTO person (first_name, last_name) <br>VALUES (&#39;first&#39;=
, &#39;last&#39;);<br><br>3. ALTER TABLE person<br>ADD COLUMN full_name VAR=
CHAR(100) GENERATED ALWAYS AS (first_name || &#39; &#39; || last_name) VIRT=
UAL;<br></div><br>The above ALTER command works if I use STORED instead. Al=
so if I define virtual generated column while creating table it works with =
same data.<br><br>1. CREATE TABLE person (<br>=C2=A0 =C2=A0 id INT GENERATE=
D BY DEFAULT AS IDENTITY,<br>=C2=A0 =C2=A0 first_name VARCHAR(50) NOT NULL,=
<br>=C2=A0 =C2=A0 last_name VARCHAR(50) NOT NULL,<br>=C2=A0 =C2=A0 full_nam=
e VARCHAR(100) NOT NULL GENERATED ALWAYS AS (first_name || &#39;=C2=A0 &#39=
; ||=C2=A0 =C2=A0 =C2=A0 last_name) VIRTUAL<br>);<br><br>2. INSERT INTO per=
son(first_name, last_name)=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 <br>=C2=A0 =C2=A0 VALUES (&#39;first&#39;, &#39;last&#39;);<br><=
br><br><br><br><br></div><br><div class=3D"gmail_quote"><div dir=3D"ltr" cl=
ass=3D"gmail_attr">On Thu, Mar 6, 2025 at 7:15=E2=80=AFPM jian he &lt;<a hr=
ef=3D"mailto:jian.universality@gmail.com" target=3D"_blank">jian.universali=
ty@gmail.com</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" styl=
e=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddin=
g-left:1ex">On Wed, Feb 26, 2025 at 3:01=E2=80=AFPM ego alter &lt;<a href=
=3D"mailto:xunengzhou@gmail.com" target=3D"_blank">xunengzhou@gmail.com</a>=
&gt; wrote:<br>
&gt;<br>
&gt; Hi, I=E2=80=99ve had a chance to review the patch.=C2=A0 As I am still=
 getting familiar with executor part, questions and feedbacks could be rela=
tively trivial.<br>
&gt;<br>
&gt; There are two minor issues i want to discuss:<br>
&gt; 1. The way of caching constraint-checking expr for virtual generated n=
ot null<br>
&gt; The existing array for caching constraint-checking expr is<br>
&gt;=C2=A0 /* array of constraint-checking expr states */<br>
&gt; ExprState **ri_ConstraintExprs;<br>
&gt;<br>
&gt; the proposed changes for virtual generated not null in patch<br>
&gt; +=C2=A0 /* array of virtual generated not null constraint-checking exp=
r states */<br>
&gt; +=C2=A0 ExprState **ri_VirGeneratedConstraintExprs;<br>
&gt; /*<br>
&gt; Could you explain the rationale behind adding this new field instead o=
f reusing ri_ConstraintExprs? The comment suggests it=E2=80=99s used specif=
ically 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 s=
eparates the handling of normal constraints from virtual ones, but I&#39;d =
like to appreciate more context on the decision.<br>
&gt;<br>
<br>
it&#39;s a cache mechanism, just like ResultRelInfo.ri_ConstraintExprs<br>
you can see comments in ExecRelCheck<br>
=C2=A0 =C2=A0 /*<br>
=C2=A0 =C2=A0 =C2=A0* If first time through for this result relation, build=
 expression<br>
=C2=A0 =C2=A0 =C2=A0* nodetrees for rel&#39;s constraint expressions.=C2=A0=
 Keep them in the per-query<br>
=C2=A0 =C2=A0 =C2=A0* memory context so they&#39;ll survive throughout the =
query.<br>
=C2=A0 =C2=A0 =C2=A0*/<br>
<br>
for example:<br>
create table t(a int, check (a &gt; 3));<br>
insert into t values (4),(3);<br>
we need to call ExecRelCheck for values 4 and 3.<br>
The second time ExecRelCheck called for values 3, if<br>
ri_ConstraintExprs is not null then<br>
we didn&#39;t need to call ExecPrepareExpr again for the same check<br>
constraint expression.<br>
<br>
+=C2=A0 ExprState **ri_VirGeneratedConstraintExprs;<br>
is specifically only for virtual generated columns not null constraint<br>
evaluation.<br>
Anyway, I renamed it to ri_GeneratedNotNullExprs.<br>
<br>
If you want to extend cache for other constraints in the future,<br>
you can add it to struct ResultRelInfo.<br>
Currently struct ResultRelInfo, we have ri_GeneratedExprsU,<br>
ri_GeneratedExprsI for generated expressions;<br>
ri_ConstraintExprs for check constraints.<br>
<br>
<br>
&gt; 2. The naming of variable gen_virtualnn.<br>
&gt; Gen_virtualnn looks confusing at first glance. Checkconstr seems to be=
 more straightforward.<br>
&gt;<br>
thanks. I changed it to exprstate.<br>
<br>
new patch attached.<br>
0001 not null for virtual generated columns, some refactoring happened<br>
within ExecConstraints<br>
to avoid duplicated error handling code.<br>
<br>
0002 and 0003 is the domain for virtual generated columns. domain can<br>
have not-null constraints,<br>
this is built on top of 0001, so I guess posting here for review should be =
fine?<br>
<br>
currently it works as intended. but add a virtual generated column<br>
with domain_with_constraints<br>
requires table rewrite. but some cases table scan should just be enough.<br=
>
some case we do need table rewrite.<br>
for example:<br>
create domain d1 as int check(value &gt; random(min=3D&gt;11::int, max=3D&g=
t;12));<br>
create domain d2 as int check(value &gt; 12);<br>
create table t(a int);<br>
insert into t select g from generate_series(1, 10) g;<br>
alter table t add column b d1 generated always as (a+11) virtual; --we<br>
do need table rewrite in phase 3.<br>
alter table t add column c d2 generated always as (a + 12) virtual;<br>
--we can only do table scan in phase 3.<br>
</blockquote></div>

--0000000000002cdfc5062fc362d7--