On Thu, Mar 20, 2025 at 12:19 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Other comments:
>
> * The block in ATRewriteTable that creates a resultRelInfo for
> ExecRelCheckGenVirtualNotNull needs an explanation.
>
I tried my best.
here are the comments above the line ``if (notnull_virtual_attrs != NIL)``
/*
* Whether change an existing generation expression or adding a new
* not-null virtual generated column, we need to evaluate whether the
* generation expression is null.
* In ExecConstraints, we use ExecRelCheckGenVirtualNotNull do the job.
* Here, we can also utilize ExecRelCheckGenVirtualNotNull.
* To achieve this, we need create a dummy ResultRelInfo. Ensure that
* the resultRelationIndex of the dummy ResultRelInfo is set to 0.
*/
+ /*
+ * XXX this deserves an explanation. Also, is rInfo a good variable
+ * name?
+ */
there are other places using this variable name: "rInfo", so i use
these names....
ATRewriteTable:
for (i = 0; i < newTupDesc->natts; i++)
{
Form_pg_attribute attr = TupleDescAttr(newTupDesc, i);
if (attr->attnotnull && !attr->attisdropped)
{
if (attr->attgenerated != ATTRIBUTE_GENERATED_VIRTUAL)
notnull_attrs = lappend_int(notnull_attrs, i);
else
notnull_virtual_attrs = lappend_int(notnull_virtual_attrs,
attr->attnum);
}
}
this is kind of ugly? notnull_virtual_attrs is 1 based, notnull_attrs
is 0 based.
I want to change it all to 1 based. see v5-0002
> * I suspect the locations for the new functions were selected with
> the help of a dartboard. ExecRelCheckGenVirtualNotNull() in
> particular looks like it could use a better location. Maybe it's
> better right after ExecConstraints, and ReportNotNullViolationError
> (or whatever we name it) can go after it.
I thought ExecRelCheckGenVirtualNotNull is very similar to ExecRelCheck,
that's why I put it close to ExecRelCheck.
putting it right after ExecConstraints is ok for me.
> * I don't find the name all_virtual_nns particularly appropriate. Maybe
> virtual_notnull_attrs?
>
in ATRewriteTable we already have "notnull_virtual_attrs"
we can rename it to notnull_virtual_attrs
> * There are some funny rules around nulls on rowtypes. I think allowing
> this tuple is wrong (and I left an XXX comment near where the 'argisrow'
> flag is set):
>
> create type twoints as (a int, b int);
> create table foo (a int, b int, c twoints generated always as (row(a,b)::twoints) not null);
> insert into foo values (null, null);
>
> I don't remember exactly what the rules are though so I may be wrong.
>
argisrow should set to false.
i think you mean the special value ``'(,)'``
create table t(a twoints not null);
insert into t select '(,)' returning a is null; --return true;
create table t1(a twoints not null generated always as ('(,)'::twoints));
insert into t1 default values returning a is null; --should return true.
i think you mean this thread:
https://postgr.es/m/173591158454.714.7664064332419606037@wrigleys.postgresql.org
should i put a test into generated_virtual.sql?
+ * We implement this by consing up a NullTest node for each virtual
trivial question.
I googled, and still found any explanation of the word "consing up".
