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

On Tue, Apr 15, 2025 at 4:10 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> jian he <jian(dot)universality(at)gmail(dot)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.