MIME-Version: 1.0
References: <202602261236.ttanikrvfx6w@alvherre.pgsql>
 <63229AD5-48DB-417C-9361-EA478DAF57AF@gmail.com>
 <CAB5m2QvGC7g03Z-ceobf+D2Q4jCZ5YKfsNOyU05peTywTzw4iA@mail.gmail.com>
In-Reply-To: 
 <CAB5m2QvGC7g03Z-ceobf+D2Q4jCZ5YKfsNOyU05peTywTzw4iA@mail.gmail.com>
From: zhanghu <kongbaik228@gmail.com>
Date: Mon, 2 Mar 2026 11:17:56 +0800
Message-ID: 
 <CAB5m2QurkJENd-fd2gKKEHAjwmTxYCqm4uuSnpwXZroE6xp3MQ@mail.gmail.com>
Subject: Re: guc: make dereference style consistent in
 check_backtrace_functions
To: Chao Li <li.evan.chao@gmail.com>
Cc: =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>,
	Junwang Zhao <zhjwpku@gmail.com>, pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAB5m2QurkJENd-fd2gKKEHAjwmTxYCqm4uuSnpwXZroE6xp3MQ%40mail.gmail.com>
Precedence: bulk

zhanghu <kongbaik228@gmail.com> =E4=BA=8E2026=E5=B9=B42=E6=9C=8827=E6=97=A5=
=E5=91=A8=E4=BA=94 16:46=E5=86=99=E9=81=93=EF=BC=9A
>
>
>
> Chao Li <li.evan.chao@gmail.com> =E4=BA=8E2026=E5=B9=B42=E6=9C=8827=E6=97=
=A5=E5=91=A8=E4=BA=94 09:34=E5=86=99=E9=81=93=EF=BC=9A
>>
>>
>>
>> > On Feb 26, 2026, at 20:37, =C3=81lvaro Herrera <alvherre@kurilemu.de> =
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-Z=
a-z0-9_]*\[0\]' src contrib --include=3D'*.c'
>>
>> Excluding irrelevant results, there are 3 more occurrences:
>>
>> 1 - contrib/basic_archive/basic_archive.c line 105
>> ```
>>         if (*newval =3D=3D NULL || *newval[0] =3D=3D '\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 =3D=3D INTSTYLE_SQL_STANDARD && *field[0] =3D=
=3D '-')
>>         {
>>                 /* Check for additional explicit signs */
>>                 bool            more_signs =3D false;
>>
>>                 for (i =3D 1; i < nf; i++)
>>                 {
>>                         if (*field[i] =3D=3D '-' || *field[i] =3D=3D '+'=
)
>>                         {
>>                                 more_signs =3D 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 =3D=3D INTSTYLE_SQL_STANDARD && nf > 0 && *fie=
ld[0] =3D=3D '-')
>>         {
>>                 force_negative =3D true;
>>                 /* Check for additional explicit signs */
>>                 for (i =3D 1; i < nf; i++)
>>                 {
>>                         if (*field[i] =3D=3D '-' || *field[i] =3D=3D '+'=
)
>>                         {
>>                                 force_negative =3D 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=E2=80=99t 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 get=
ting 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 explici=
t as *(field[i]). Furthermore, I think we should change the function signat=
ures 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 DecodeInter=
val, 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.
>>
>> =C3=81lvaro, 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=E2=80=99s 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:

=E2=80=9CYou have not passed the cool off period yet.=E2=80=9D

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