Horton

Thread

Commits

  1. Fix failure for generated column with a not-null domain constraint.

  2. Handle default NULL insertion a little better.

  1. bug in stored generated column over domain with constraints.

    jian he <jian.universality@gmail.com> — 2025-02-25T17:09:29Z

    hi.

create domain d1 as int not null;
create domain d2 as int check (value > 1);
create domain d3 as int check (value is not null);

create table t0(b int, a d3 GENERATED ALWAYS as (b + 11) stored);
insert into t0 values (1, default);
ERROR:  value for domain d3 violates check constraint "d3_check"

in the above example, a domain with check constraint not working as intended,
similarly, a domain with not-null constraint will also not work.
now table t0 can not insert any data, because of check constraint violation.
so i think this is a bug, (hope i didn't miss anything).

in ExecBuildProjectionInfo, we compile "values (1, default)"  as
targetlist expression (CONST, COERCETODOMAIN)
Then in ExecResult, ExecProject, ExecInterpExpr we evaluate the
compiled expression;
we failed at ExecEvalConstraintCheck. we are acting like: ``null::d3``.

explain(costs off, verbose) insert into t0 values (1, default);
               QUERY PLAN
----------------------------------------
 Insert on public.t0
   ->  Result
         Output: 1, NULL::integer

the plan is ``NULL::integer``, which will not fail, but  ``NULL::d3`` will fail.
that means, the output plan is right. it's the execution wrong?


the main fix should be in rewriteTargetListIU.
UPDATE don't have this issue, since there is no Result node,
ExecComputeStoredGenerated will do the domain constraint check.

related:
https://git.postgresql.org/cgit/postgresql.git/commit/?id=0da39aa7667b06e16189d318f7850d559d446d52

  2. Re: bug in stored generated column over domain with constraints.

    jian he <jian.universality@gmail.com> — 2025-02-26T03:32:48Z

    hi.

comments refined and minor aesthetic adjustments made.

  3. Re: bug in stored generated column over domain with constraints.

    jian he <jian.universality@gmail.com> — 2025-04-10T08:53:12Z

    hi.

new patch attached.

rewriteTargetListIU, expand_insert_targetlist these two places can
make a null Const TargetEntry for the generated column in an INSERT
operation.

but since this problem only occurs in INSERT, so i placed the logic
within expand_insert_targetlist would be appropriate?

The following are excerpts of the commit message.
--------------------------------
create domain d3 as int check (value is not null);
create table t0(b int, a d3 GENERATED ALWAYS as (b + 11) stored);

insert into t0 values (1, default);
ERROR:  value for domain d3 violates check constraint "d3_check"

explain(costs off, verbose) insert into t0 values (1, default);
              QUERY PLAN
---------------------------------------
 Insert on public.t0
   ->  Result
         Output: 1, NULL::integer

For INSERT operation, for Query->targetList, we should not make a
generated column
over domain with constraint to a CoerceToDomain node, instead, we make it as a
simple null Const over domain's base type.

When a column is a generated column in an INSERT, expand_insert_targetlist
should unconditionally generate a null Const to be inserted. If we are not
doing this way, we might end up wrapping the null Const in a CoerceToDomain
node, which may trigger runtime error earlier if the domain has a NOT NULL
constraint. That's not fine, as generated columns are already handled in
ExecComputeStoredGenerated.
--------------------------------

  4. Re: bug in stored generated column over domain with constraints.

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-14T20:10:54Z

    jian he <jian.universality@gmail.com> writes:
> new patch attached.

I looked this over.  It's kind of astonishing that nobody has reported
this before, because AFAICT it's been broken since we invented
generated columns.

> rewriteTargetListIU, expand_insert_targetlist these two places can
> make a null Const TargetEntry for the generated column in an INSERT
> operation.

rewriteTargetListIU will *not* do that: it explicitly does nothing
for an attgenerated column, except apply a bunch of error checks.
See about line 988 in HEAD.  So it seems sufficient to fix
expand_insert_targetlist as you've done here.  And then we have to
make ExecCheckPlanOutput cope, too.

I think that this patch is technically correct, but I don't like
it much because it pays little attention to keeping
expand_insert_targetlist and ExecCheckPlanOutput readable, and no
attention at all to keeping their logic parallel.  I think we can
make the code cleaner by moving the default case to the end, as
in the attached.

The test cases seemed a bit overdone, especially in comparison
to the adjacent existing tests.  Fine for development maybe,
but I don't see the value in carrying them forever.  On the other
hand, there's a comment at the top of the test script:
	-- keep these tests aligned with generated_virtual.sql
The VIRTUAL case is currently rejecting domains altogether.
But perhaps that won't be true forever, so I think we ought
to follow that advice.

So I end with the attached v4.

			regards, tom lane

  5. Re: bug in stored generated column over domain with constraints.

    jian he <jian.universality@gmail.com> — 2025-04-15T00:37:53Z

    On Tue, Apr 15, 2025 at 4:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> jian he <jian.universality@gmail.com> writes:
> > new patch attached.
>
> I looked this over.  It's kind of astonishing that nobody has reported
> this before, because AFAICT it's been broken since we invented
> generated columns.
>
> > rewriteTargetListIU, expand_insert_targetlist these two places can
> > make a null Const TargetEntry for the generated column in an INSERT
> > operation.
>
> rewriteTargetListIU will *not* do that: it explicitly does nothing
> for an attgenerated column, except apply a bunch of error checks.
> See about line 988 in HEAD.  So it seems sufficient to fix
> expand_insert_targetlist as you've done here.  And then we have to
> make ExecCheckPlanOutput cope, too.
>
> I think that this patch is technically correct, but I don't like
> it much because it pays little attention to keeping
> expand_insert_targetlist and ExecCheckPlanOutput readable, and no
> attention at all to keeping their logic parallel.  I think we can
> make the code cleaner by moving the default case to the end, as
> in the attached.
>

your ExecCheckPlanOutput change makes sense to me.
call getBaseTypeAndTypmod in ExecCheckPlanOutput would be a waste cycle,
given that we will compute the generated column later.


> The test cases seemed a bit overdone, especially in comparison
> to the adjacent existing tests.  Fine for development maybe,
> but I don't see the value in carrying them forever.  On the other
> hand, there's a comment at the top of the test script:
>         -- keep these tests aligned with generated_virtual.sql
> The VIRTUAL case is currently rejecting domains altogether.
> But perhaps that won't be true forever, so I think we ought
> to follow that advice.
>

I submitted a patch for the domain over the virtual generated column,
so didn't add such a test on it.

Thanks for simplifying the tests, overall all looks good.

  6. Re: bug in stored generated column over domain with constraints.

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-15T16:13:49Z

    jian he <jian.universality@gmail.com> writes:
> Thanks for simplifying the tests, overall all looks good.

OK, pushed and back-patched, but only to v14.  I tried to make
a variant that'd work in v13, but it caused ExecCheckPlanOutput
to throw "Query provides a value for a generated column" errors
for UPDATEs.  That's surely related to the major refactoring of
UPDATE targetlists that we did in v14.  Given that we've had
zero field complaints about this problem so far, I think the
risk of breaking something in v13 exceeds the value of fixing
the bug, so I left v13 alone.

			regards, tom lane