Thread: wrong comments in rewriteTargetListIU

hi.

in function, rewriteTargetListIU
we have:

for (attrno = 1; attrno <= numattrs; attrno++)
{
            /*
             * Can only insert DEFAULT into generated columns, regardless of
             * any OVERRIDING clauses.
             */
            if (att_tup->attgenerated && !apply_default)
}

I think the above comments are wrong, since there are no
OVERRIDING clauses for the generated column.

Hi,

On Thu, Apr 10, 2025 at 5:58 PM jian he <jian.universality@gmail.com> wrote:
>
> hi.
>
> in function, rewriteTargetListIU
> we have:
>
> for (attrno = 1; attrno <= numattrs; attrno++)
> {
>             /*
>              * Can only insert DEFAULT into generated columns, regardless of
>              * any OVERRIDING clauses.
>              */
>             if (att_tup->attgenerated && !apply_default)
> }
>
> I think the above comments are wrong, since there are no
> OVERRIDING clauses for the generated column.

Hmm, I'm not sure that is quite right.  See 17958972fe3b which added
the comment, especially the test cases that it adds.

--
Thanks, Amit Langote

On Tue, 17 Jun 2025 at 06:27, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 10.04.25 11:46, Amit Langote wrote:
> >
> > On Thu, Apr 10, 2025 at 5:58 PM jian he <jian.universality@gmail.com> wrote:
> >>
> >> in function, rewriteTargetListIU we have:
> >>
> >>              /*
> >>               * Can only insert DEFAULT into generated columns, regardless of
> >>               * any OVERRIDING clauses.
> >>               */
> >>              if (att_tup->attgenerated && !apply_default)
> >>
> >> I think the above comments are wrong, since there are no
> >> OVERRIDING clauses for the generated column.
> >
> > Hmm, I'm not sure that is quite right.  See 17958972fe3b which added
> > the comment, especially the test cases that it adds.
>
> Jian is right, there are no OVERRIDING clauses for generated columns,
> and the shown code also doesn't check for them, so the comment doesn't
> match the code.  I suspect that commit 17958972fe3b slightly confused
> generated and identity columns in this case.  I suggest to remove the
> half-sentence "regardless of ...".
>

Hmm, to me that comment seems quite clear in the context of the
surrounding code. It's making the point that OVERRIDING clauses have
no effect on what can be inserted into generated columns, unlike the
identity column handling code that immediately precedes it.

Perhaps there's an alternate wording that would make that meaning more
explicit, rather than just removing that part of the comment, which
might leave people wondering why that code block isn't checking
"override", when the preceding code blocks do.

Regards,
Dean

On Tue, Jun 17, 2025 at 7:42 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> However, I see your point that it contrasts with the attidentity code
> just above.
>
> Perhaps a way to resolve this would be to rewrite the comment something
> like:
>
>      /*
>       * Although inserting into a GENERATED BY DEFAULT identity column
>       * is allowed, apply the default if OVERRIDING USER VALUE is
>       * specified.
>       */
>      if (att_tup->attidentity == ATTRIBUTE_IDENTITY_BY_DEFAULT &&
>          override == OVERRIDING_USER_VALUE)
>          apply_default = true;
>
>      /*
>       * Can only insert DEFAULT into generated columns.  (The OVERRIDING
>       * clause does not apply to generated columns, so we don't consider
>       * it here.)
>       */
>      if (att_tup->attgenerated && !apply_default)
>      {
>
I think this comment is better than the master comments.

the master comments
            /*
             * Can only insert DEFAULT into generated columns, regardless of
             * any OVERRIDING clauses.
             */
as a non-native English speaker, i interpret this as it implies that
generated columns
can have OVERRIDING clauses in some scenarios.