guc: make dereference style consistent in check_backtrace_functions

On Feb 26, 2026, at 15:03, zhanghu <kongbaik228@gmail.com> wrote:

Hi,

In check_backtrace_functions(), most accesses to the input string follow the pattern (*newval)[i]. However, the empty-string check is currently written as:

if (*newval[0] == '\0')

While functionally correct due to how the compiler handles the address-of-address context here, this form is semantically misleading. It relies on implicit operator precedence rather than explicit intent.

The attached patch rewrites it as:

if ((*newval)[0] == '\0')

This change ensures semantic clarity and maintains a consistent dereferencing style throughout the function. No functional changes are introduced.

Regards,
Zhang Hu
<v1-0001-guc-make-dereference-style-consistent-in-check_ba.patch>

This is an interesting find.

[] has higher precedence than *, so:

- (*newval)[i] means to get the first string, then get the char at position i
- *newval[i] means to get the array element at position i, then get the first char

When i is 0, (*newval)[0] and *newval[0] happen to yield the same result, so this isn't a functional bug.

However, in the GUC context, newval is a point to a string rather than a two-dimension char array, *newval[i] is meaningless, so +1 for fixing this to improve readability.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Thu, Feb 26, 2026 at 5:21 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Feb 26, 2026, at 15:03, zhanghu <kongbaik228@gmail.com> wrote:

Hi,

In check_backtrace_functions(), most accesses to the input string follow the pattern (*newval)[i]. However, the empty-string check is currently written as:

if (*newval[0] == '\0')

While functionally correct due to how the compiler handles the address-of-address context here, this form is semantically misleading. It relies on implicit operator precedence rather than explicit intent.

The attached patch rewrites it as:

if ((*newval)[0] == '\0')

This change ensures semantic clarity and maintains a consistent dereferencing style throughout the function. No functional changes are introduced.

Regards,
Zhang Hu
<v1-0001-guc-make-dereference-style-consistent-in-check_ba.patch>

This is an interesting find.

[] has higher precedence than *, so:

- (*newval)[i] means to get the first string, then get the char at position i
- *newval[i] means to get the array element at position i, then get the first char

When i is 0, (*newval)[0] and *newval[0] happen to yield the same result, so this isn't a functional bug.

However, in the GUC context, newval is a point to a string rather than a two-dimension char array, *newval[i] is meaningless, so +1 for fixing this to improve readability.

+1
The double pointer indicates an output parameter, the commit
message should be adjusted though.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

--
Regards
Junwang Zhao

There is at least one more place in the code where this is done.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Industry suffers from the managerial dogma that for the sake of stability
and continuity, the company should be independent of the competence of
individual employees." (E. Dijkstra)

On Feb 26, 2026, at 20:37, Álvaro 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-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/

Chao Li <li.evan.chao@gmail.com> 于2026年2月27日周五 09:34写道：

zhanghu <kongbaik228@gmail.com> 于2026年2月27日周五 16:46写道：

Chao Li <li.evan.chao@gmail.com> 于2026年2月27日周五 09:34写道：

On Feb 26, 2026, at 20:37, Álvaro 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-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

On Mar 2, 2026, at 11:17, zhanghu <kongbaik228@gmail.com> wrote:

zhanghu <kongbaik228@gmail.com> 于2026年2月27日周五 16:46写道：

Chao Li <li.evan.chao@gmail.com> 于2026年2月27日周五 09:34写道：

On Feb 26, 2026, at 20:37, Álvaro 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-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

Yes, there is a cool off period when one first time registers to the CommitFest. I don’t remember exactly how many days the period is, should be just a few days. So stay tuned.

I tried to add the patch to CF, but I noticed that, if I do that, the patch author would be me, and as you are fully registered, I could not change the author to you. So, please just wait to pass the cool off period and then create the CF entry.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

zhanghu <kongbaik228@gmail.com> 于2026年3月2日周一 11:17写道：

zhanghu <kongbaik228@gmail.com> 于2026年2月27日周五 16:46写道：

Chao Li <li.evan.chao@gmail.com> 于2026年2月27日周五 09:34写道：

On Feb 26, 2026, at 20:37, Álvaro 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-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