public inbox for [email protected]
help / color / mirror / Atom feedRe: guc: make dereference style consistent in check_backtrace_functions
3+ messages / 2 participants
[nested] [flat]
* Re: guc: make dereference style consistent in check_backtrace_functions
@ 2026-03-02 03:17 zhanghu <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: zhanghu @ 2026-03-02 03:17 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Junwang Zhao <[email protected]>; [email protected]
zhanghu <[email protected]> 于2026年2月27日周五 16:46写道：
>
>
>
> Chao Li <[email protected]> 于2026年2月27日周五 09:34写道：
>>
>>
>>
>> > On Feb 26, 2026, at 20:37, Álvaro Herrera <[email protected]> wrote:
>> >
>> > There is at least one more place in the code where this is done.
>> >
>>
>> I did a search with the command: grep -RInE '\*[[:space:]]*[A-Za-z_][A-Za-z0-9_]*\[0\]' src contrib --include='*.c'
>>
>> Excluding irrelevant results, there are 3 more occurrences:
>>
>> 1 - contrib/basic_archive/basic_archive.c line 105
>> ```
>> if (*newval == NULL || *newval[0] == '\0')
>> return true;
>> ```
>>
>> Here, the code checks *newval first, which implies that the subsequent *newval[0] is unintentional syntax.
>>
>> 2 - src/interfaces/ecpg/pgtypeslib/interval.c line 62
>> ```
>> int
>> DecodeInterval(char **field, int *ftype, int nf, /* int range, */
>> int *dtype, struct /* pg_ */ tm *tm, fsec_t *fsec)
>> {
>> ...
>> if (IntervalStyle == INTSTYLE_SQL_STANDARD && *field[0] == '-')
>> {
>> /* Check for additional explicit signs */
>> bool more_signs = false;
>>
>> for (i = 1; i < nf; i++)
>> {
>> if (*field[i] == '-' || *field[i] == '+')
>> {
>> more_signs = true;
>> break;
>> }
>> }
>> ```
>>
>> 3 - src/backend/utils/adt/datatime.c line 3522
>> ```
>> int
>> DecodeInterval(char **field, int *ftype, int nf, int range,
>> int *dtype, struct pg_itm_in *itm_in)
>> {
>> ...
>> if (IntervalStyle == INTSTYLE_SQL_STANDARD && nf > 0 && *field[0] == '-')
>> {
>> force_negative = true;
>> /* Check for additional explicit signs */
>> for (i = 1; i < nf; i++)
>> {
>> if (*field[i] == '-' || *field[i] == '+')
>> {
>> force_negative = false;
>> break;
>> }
>> }
>> }
>> ```
>>
>> Where 2&3 makes this patch more interesting.
>>
>> Both occurrences are inside functions named DecodeInterval. For non-zero i, the code also performs *field[i]:
>>
>> Given this code has been there for years, I don’t believe it is a bug. I checked the callers of DecodeInterval in both files and found that field is defined as:
>> ```
>> char *field[MAXDATEFIELDS];
>> ```
>>
>> This explains why *field[i] works; it is doing the intended thing by getting the first character of the string at array position i.
>>
>> However, since the precedence between the [] and * operators frequently confuses people, I suggest adding parentheses to make the intention explicit as *(field[i]). Furthermore, I think we should change the function signatures to use the type char *field[] to reflect the actual type the functions expect. If a caller were to pass a true char ** typed field to DecodeInterval, the current logic would result in a bug.
>>
>> See the attached diff for my suggested changes.
>>
>> Best regards,
>> --
>> Chao Li (Evan)
>> HighGo Software Co., Ltd.
>> https://www.highgo.com/
>>
>> Hi,
>>
>> Thank you all for the reviews and detailed feedback.
>>
>> Álvaro, thanks for pointing out that there were additional
>> occurrences elsewhere in the tree. I have updated the original
>> patch to address those cases; the revised version is attached
>> as v2-0001.
>>
>> I also appreciate the review and suggestions from
>> Chao and Junwang.
>>
>> Regarding the additional changes suggested by Chao: they go
>> somewhat beyond the original scope of my original patch.
>> To keep the discussion concrete, I have included Chao’s proposed
>> diff as a separate patch (v2-0002) so it can be reviewed independently.
>>
>> I have reviewed v2-0002 locally, and it looks good to me.
>>
>> Thanks again for the guidance.
>>
>> Regards,
>> Zhang Hu
>>
>>
Hi,
I am planning to add this patch to the current CommitFest, but when
logging in to commitfest.postgresql.org I get the message:
“You have not passed the cool off period yet.”
It seems my account is still within the cool-off period after registration.
Could someone please add this patch to the CommitFest on my behalf?
Thanks.
Best regards,
Zhang Hu
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: guc: make dereference style consistent in check_backtrace_functions
@ 2026-03-04 10:31 zhanghu <[email protected]>
parent: zhanghu <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: zhanghu @ 2026-03-04 10:31 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Junwang Zhao <[email protected]>; [email protected]; Chao Li <[email protected]>
zhanghu <[email protected]> 于2026年3月2日周一 11:17写道：
>
> zhanghu <[email protected]> 于2026年2月27日周五 16:46写道：
> >
> >
> >
> > Chao Li <[email protected]> 于2026年2月27日周五 09:34写道：
> >>
> >>
> >>
> >> > On Feb 26, 2026, at 20:37, Álvaro Herrera <[email protected]>
wrote:
> >> >
> >> > There is at least one more place in the code where this is done.
> >> >
> >>
> >> I did a search with the command: grep -RInE
'\*[[:space:]]*[A-Za-z_][A-Za-z0-9_]*\[0\]' src contrib --include='*.c'
> >>
> >> Excluding irrelevant results, there are 3 more occurrences:
> >>
> >> 1 - contrib/basic_archive/basic_archive.c line 105
> >> ```
> >> if (*newval == NULL || *newval[0] == '\0')
> >> return true;
> >> ```
> >>
> >> Here, the code checks *newval first, which implies that the subsequent
*newval[0] is unintentional syntax.
> >>
> >> 2 - src/interfaces/ecpg/pgtypeslib/interval.c line 62
> >> ```
> >> int
> >> DecodeInterval(char **field, int *ftype, int nf, /* int range,
*/
> >> int *dtype, struct /* pg_ */ tm *tm, fsec_t
*fsec)
> >> {
> >> ...
> >> if (IntervalStyle == INTSTYLE_SQL_STANDARD && *field[0] == '-')
> >> {
> >> /* Check for additional explicit signs */
> >> bool more_signs = false;
> >>
> >> for (i = 1; i < nf; i++)
> >> {
> >> if (*field[i] == '-' || *field[i] == '+')
> >> {
> >> more_signs = true;
> >> break;
> >> }
> >> }
> >> ```
> >>
> >> 3 - src/backend/utils/adt/datatime.c line 3522
> >> ```
> >> int
> >> DecodeInterval(char **field, int *ftype, int nf, int range,
> >> int *dtype, struct pg_itm_in *itm_in)
> >> {
> >> ...
> >> if (IntervalStyle == INTSTYLE_SQL_STANDARD && nf > 0 &&
*field[0] == '-')
> >> {
> >> force_negative = true;
> >> /* Check for additional explicit signs */
> >> for (i = 1; i < nf; i++)
> >> {
> >> if (*field[i] == '-' || *field[i] == '+')
> >> {
> >> force_negative = false;
> >> break;
> >> }
> >> }
> >> }
> >> ```
> >>
> >> Where 2&3 makes this patch more interesting.
> >>
> >> Both occurrences are inside functions named DecodeInterval. For
non-zero i, the code also performs *field[i]:
> >>
> >> Given this code has been there for years, I don’t believe it is a bug.
I checked the callers of DecodeInterval in both files and found that field
is defined as:
> >> ```
> >> char *field[MAXDATEFIELDS];
> >> ```
> >>
> >> This explains why *field[i] works; it is doing the intended thing by
getting the first character of the string at array position i.
> >>
> >> However, since the precedence between the [] and * operators
frequently confuses people, I suggest adding parentheses to make the
intention explicit as *(field[i]). Furthermore, I think we should change
the function signatures to use the type char *field[] to reflect the actual
type the functions expect. If a caller were to pass a true char ** typed
field to DecodeInterval, the current logic would result in a bug.
> >>
> >> See the attached diff for my suggested changes.
> >>
> >> Best regards,
> >> --
> >> Chao Li (Evan)
> >> HighGo Software Co., Ltd.
> >> https://www.highgo.com/
> >>
> >> Hi,
> >>
> >> Thank you all for the reviews and detailed feedback.
> >>
> >> Álvaro, thanks for pointing out that there were additional
> >> occurrences elsewhere in the tree. I have updated the original
> >> patch to address those cases; the revised version is attached
> >> as v2-0001.
> >>
> >> I also appreciate the review and suggestions from
> >> Chao and Junwang.
> >>
> >> Regarding the additional changes suggested by Chao: they go
> >> somewhat beyond the original scope of my original patch.
> >> To keep the discussion concrete, I have included Chao’s proposed
> >> diff as a separate patch (v2-0002) so it can be reviewed independently.
> >>
> >> I have reviewed v2-0002 locally, and it looks good to me.
> >>
> >> Thanks again for the guidance.
> >>
> >> Regards,
> >> Zhang Hu
> >>
> >>
>
> Hi,
>
> I am planning to add this patch to the current CommitFest, but when
> logging in to commitfest.postgresql.org I get the message:
>
> “You have not passed the cool off period yet.”
>
> It seems my account is still within the cool-off period after
registration.
>
> Could someone please add this patch to the CommitFest on my behalf?
>
> Thanks.
>
> Best regards,
> Zhang Hu
Hi Álvaro,
Thank you for pointing that out.
I have fixed the additional occurrence you mentioned and updated the patch
accordingly. I have also added the patch to the CommitFest:
https://commitfest.postgresql.org/patch/6566/
Please let me know if there is anything else I should do for this patch.
Thanks for your help.
Best regards,
Zhang Hu
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: guc: make dereference style consistent in check_backtrace_functions
@ 2026-03-24 23:38 Chao Li <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Chao Li @ 2026-03-24 23:38 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: zhanghu <[email protected]>; Junwang Zhao <[email protected]>; [email protected]
> On Mar 24, 2026, at 23:55, Álvaro Herrera <[email protected]> wrote:
>
> On 2026-Mar-04, zhanghu wrote:
>
>> Hi Álvaro,
>>
>> Thank you for pointing that out.
>>
>> I have fixed the additional occurrence you mentioned and updated the patch
>> accordingly.
>
> Thanks, I have pushed 0001.
>
> I don't intend to look at 0002 though, sorry.
>
No worries, that’s fine.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-03-24 23:38 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-03-02 03:17 Re: guc: make dereference style consistent in check_backtrace_functions zhanghu <[email protected]>
2026-03-04 10:31 ` zhanghu <[email protected]>
2026-03-24 23:38 Re: guc: make dereference style consistent in check_backtrace_functions Chao Li <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox