Re: support virtual generated column not null constraint

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".