Обсуждение: 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 10.04.25 11:46, Amit Langote wrote:
> 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.
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 ...".
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 17.06.25 11:31, Dean Rasheed wrote:
> On Tue, 17 Jun 2025 at 06:27, Peter Eisentraut <peter@eisentraut.org> wrote:
>> 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.
I think Jian's point is that the OVERRIDING clause was not created to
affect generated columns, and so mentioning explicitly that it does not
affect generated columns does not seem meaningful. You might as well
mention any other clause there and say it doesn't affect this particular
code.
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)
{
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.
On Tue, 17 Jun 2025 at 13:56, jian he <jian.universality@gmail.com> wrote:
>
> 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.
OK, fair enough. The suggested update looks reasonable to me.
Regards,
Dean
On 17.06.25 20:50, Dean Rasheed wrote:
> On Tue, 17 Jun 2025 at 13:56, jian he <jian.universality@gmail.com> wrote:
>>
>> 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.
>
> OK, fair enough. The suggested update looks reasonable to me.
committed