| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Álvaro Herrera <alvherre(at)kurilemu(dot)de> |
| Cc: | Junwang Zhao <zhjwpku(at)gmail(dot)com>, zhanghu <kongbaik228(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: guc: make dereference style consistent in check_backtrace_functions |
| Date: | 2026-02-27 01:33:45 |
| Message-ID: | 63229AD5-48DB-417C-9361-EA478DAF57AF@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Feb 26, 2026, at 20:37, Álvaro Herrera <alvherre(at)kurilemu(dot)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/
| Attachment | Content-Type | Size |
|---|---|---|
| nocfbot.diff | application/octet-stream | 6.1 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Richard Guo | 2026-02-27 01:42:11 | Re: Convert ALL SubLinks to ANY SubLinks |
| Previous Message | Andreas Karlsson | 2026-02-27 01:15:46 | Re: Use pg_malloc macros in src/fe_utils |