On Tue, Apr 15, 2025 at 4:10 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > jian he <jian.universal...@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.
