public inbox for [email protected]
help / color / mirror / Atom feedRe: guc: make dereference style consistent in check_backtrace_functions
6+ messages / 3 participants
[nested] [flat]
* Re: guc: make dereference style consistent in check_backtrace_functions
@ 2026-02-26 12:37 Álvaro Herrera <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Álvaro Herrera @ 2026-02-26 12:37 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: Chao Li <[email protected]>; zhanghu <[email protected]>; [email protected]
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)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: guc: make dereference style consistent in check_backtrace_functions
@ 2026-02-27 01:33 Chao Li <[email protected]>
parent: Álvaro Herrera <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Chao Li @ 2026-02-27 01:33 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Junwang Zhao <[email protected]>; zhanghu <[email protected]>; [email protected]
> 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/
Attachments:
[application/octet-stream] nocfbot.diff (6.1K, 2-nocfbot.diff)
download | inline diff:
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 90946db72ff..7d845700163 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -3483,7 +3483,7 @@ ClearPgItmIn(struct pg_itm_in *itm_in)
* suffices.
*/
int
-DecodeInterval(char **field, int *ftype, int nf, int range,
+DecodeInterval(char *field[], int *ftype, int nf, int range,
int *dtype, struct pg_itm_in *itm_in)
{
bool force_negative = false;
@@ -3519,13 +3519,13 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
* to dump in postgres style, not SQL style.)
*----------
*/
- if (IntervalStyle == INTSTYLE_SQL_STANDARD && nf > 0 && *field[0] == '-')
+ 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] == '+')
+ if (*(field[i]) == '-' || *(field[i]) == '+')
{
force_negative = false;
break;
@@ -3557,7 +3557,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
* least one digit; there could be ':', '.', '-' embedded in
* it as well.
*/
- Assert(*field[i] == '-' || *field[i] == '+');
+ Assert(*(field[i]) == '-' || *(field[i]) == '+');
/*
* Check for signed hh:mm or hh:mm:ss. If so, process exactly
@@ -3567,7 +3567,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
DecodeTimeForInterval(field[i] + 1, fmask, range,
&tmask, itm_in) == 0)
{
- if (*field[i] == '-')
+ if (*(field[i]) == '-')
{
/* flip the sign on time field */
if (itm_in->tm_usec == PG_INT64_MIN)
@@ -3650,7 +3650,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
if (*cp != '\0')
return DTERR_BAD_FORMAT;
type = DTK_MONTH;
- if (*field[i] == '-')
+ if (*(field[i]) == '-')
val2 = -val2;
if (pg_mul_s64_overflow(val, MONTHS_PER_YEAR, &val))
return DTERR_FIELD_OVERFLOW;
@@ -3663,7 +3663,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
dterr = ParseFraction(cp, &fval);
if (dterr)
return dterr;
- if (*field[i] == '-')
+ if (*(field[i]) == '-')
fval = -fval;
}
else if (*cp == '\0')
diff --git a/src/include/utils/datetime.h b/src/include/utils/datetime.h
index f77c6acd8b6..66dd871fd7f 100644
--- a/src/include/utils/datetime.h
+++ b/src/include/utils/datetime.h
@@ -316,7 +316,7 @@ extern int DecodeTimezone(const char *str, int *tzp);
extern int DecodeTimeOnly(char **field, int *ftype, int nf,
int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
DateTimeErrorExtra *extra);
-extern int DecodeInterval(char **field, int *ftype, int nf, int range,
+extern int DecodeInterval(char *field[], int *ftype, int nf, int range,
int *dtype, struct pg_itm_in *itm_in);
extern int DecodeISO8601Interval(char *str,
int *dtype, struct pg_itm_in *itm_in);
diff --git a/src/interfaces/ecpg/pgtypeslib/dt.h b/src/interfaces/ecpg/pgtypeslib/dt.h
index 00a45799d55..47df0df4739 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt.h
+++ b/src/interfaces/ecpg/pgtypeslib/dt.h
@@ -311,7 +311,7 @@ do { \
#define TIMESTAMP_IS_NOEND(j) ((j) == DT_NOEND)
#define TIMESTAMP_NOT_FINITE(j) (TIMESTAMP_IS_NOBEGIN(j) || TIMESTAMP_IS_NOEND(j))
-int DecodeInterval(char **field, int *ftype, int nf, int *dtype, struct tm *tm, fsec_t *fsec);
+int DecodeInterval(char *field[], int *ftype, int nf, int *dtype, struct tm *tm, fsec_t *fsec);
int DecodeTime(char *str, int *tmask, struct tm *tm, fsec_t *fsec);
void EncodeDateTime(struct tm *tm, fsec_t fsec, bool print_tz, int tz, const char *tzn, int style, char *str, bool EuroDates);
void EncodeInterval(struct tm *tm, fsec_t fsec, int style, char *str);
diff --git a/src/interfaces/ecpg/pgtypeslib/interval.c b/src/interfaces/ecpg/pgtypeslib/interval.c
index e452a088f9e..4d0e75d4dd4 100644
--- a/src/interfaces/ecpg/pgtypeslib/interval.c
+++ b/src/interfaces/ecpg/pgtypeslib/interval.c
@@ -323,7 +323,7 @@ DecodeISO8601Interval(char *str,
* int IntervalStyle = INTSTYLE_POSTGRES;
*/
int
-DecodeInterval(char **field, int *ftype, int nf, /* int range, */
+DecodeInterval(char *field[], int *ftype, int nf, /* int range, */
int *dtype, struct /* pg_ */ tm *tm, fsec_t *fsec)
{
int IntervalStyle = INTSTYLE_POSTGRES_VERBOSE;
@@ -362,7 +362,7 @@ DecodeInterval(char **field, int *ftype, int nf, /* int range, */
* least one digit; there could be ':', '.', '-' embedded in
* it as well.
*/
- Assert(*field[i] == '-' || *field[i] == '+');
+ Assert(*(field[i]) == '-' || *(field[i]) == '+');
/*
* Try for hh:mm or hh:mm:ss. If not, fall through to
@@ -373,7 +373,7 @@ DecodeInterval(char **field, int *ftype, int nf, /* int range, */
DecodeTime(field[i] + 1, /* INTERVAL_FULL_RANGE, */
&tmask, tm, fsec) == 0)
{
- if (*field[i] == '-')
+ if (*(field[i]) == '-')
{
/* flip the sign on all fields */
tm->tm_hour = -tm->tm_hour;
@@ -447,7 +447,7 @@ DecodeInterval(char **field, int *ftype, int nf, /* int range, */
if (*cp != '\0')
return DTERR_BAD_FORMAT;
type = DTK_MONTH;
- if (*field[i] == '-')
+ if (*(field[i]) == '-')
val2 = -val2;
val = val * MONTHS_PER_YEAR + val2;
fval = 0;
@@ -459,7 +459,7 @@ DecodeInterval(char **field, int *ftype, int nf, /* int range, */
if (*cp != '\0' || errno != 0)
return DTERR_BAD_FORMAT;
- if (*field[i] == '-')
+ if (*(field[i]) == '-')
fval = -fval;
}
else if (*cp == '\0')
@@ -622,14 +622,14 @@ DecodeInterval(char **field, int *ftype, int nf, /* int range, */
* to dump in postgres style, not SQL style.)
*----------
*/
- if (IntervalStyle == INTSTYLE_SQL_STANDARD && *field[0] == '-')
+ 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] == '+')
+ if (*(field[i]) == '-' || *(field[i]) == '+')
{
more_signs = true;
break;
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: guc: make dereference style consistent in check_backtrace_functions
@ 2026-02-27 08:46 zhanghu <[email protected]>
parent: Chao Li <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: zhanghu @ 2026-02-27 08:46 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Junwang Zhao <[email protected]>; [email protected]
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
>
>
>
Attachments:
[application/octet-stream] v2-0001-guc-Clarify-dereference-order-in-newval-string-ch.patch (2.1K, 3-v2-0001-guc-Clarify-dereference-order-in-newval-string-ch.patch)
download | inline diff:
From e35f0e4baa0cbcca9a969fc371ec5601152050bc Mon Sep 17 00:00:00 2001
From: zhanghu <[email protected]>
Date: Fri, 27 Feb 2026 14:39:37 +0800
Subject: [PATCH v2 1/2] guc: Clarify dereference order in newval string checks
In check_backtrace_functions(), most accesses to the input string use
the form:
(*newval)[i]
However, the empty-string check was written as:
*newval[0] == '\0'
Since [] has higher precedence than *, this is parsed as
*(newval[0]). Although it produces the same result when the index
is zero, it implies array-style access on newval and is inconsistent
with the surrounding style.
Similarly, in check_archive_directory(), the empty-string check
was written as:
*newval[0] == '\0'
In both cases, newval is a pointer-to-pointer used as an output
parameter in the GUC framework rather than a two-dimensional
character array. Rewriting these checks as:
(*newval)[0] == '\0'
makes the intended dereference order explicit.
No functional change.
Author: zhanghu <[email protected]>
---
contrib/basic_archive/basic_archive.c | 2 +-
src/backend/utils/error/elog.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 6c7f985d48b..6860e4642ee 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -102,7 +102,7 @@ check_archive_directory(char **newval, void **extra, GucSource source)
* Our check_configured callback also checks for this and prevents
* archiving from proceeding if it is still empty.
*/
- if (*newval == NULL || *newval[0] == '\0')
+ if (*newval == NULL || (*newval)[0] == '\0')
return true;
/*
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 0d0bf0f6aa5..650a79b7e12 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2627,7 +2627,7 @@ check_backtrace_functions(char **newval, void **extra, GucSource source)
return false;
}
- if (*newval[0] == '\0')
+ if ((*newval)[0] == '\0')
{
*extra = NULL;
return true;
--
2.33.0
[application/octet-stream] v2-0002-datetime-Clarify-DecodeInterval-field-parameter-t.patch (7.2K, 4-v2-0002-datetime-Clarify-DecodeInterval-field-parameter-t.patch)
download | inline diff:
From ff48c8ef74e5373a466a9866c57fd0029a123cb1 Mon Sep 17 00:00:00 2001
From: zhanghu <[email protected]>
Date: Fri, 27 Feb 2026 14:51:57 +0800
Subject: [PATCH v2 2/2] datetime: Clarify DecodeInterval field parameter type
and dereference
DecodeInterval() operates on an array of string pointers, as callers
define field as:
char *field[MAXDATEFIELDS];
The existing signature used "char **field", which could suggest a
generic pointer-to-pointer and allow callers to pass a true char **,
which would not match the function’s assumptions.
Change the parameter type to "char *field[]" to reflect the expected
array-of-pointers usage. Also add parentheses to expressions such as
*field[i], rewriting them as *(field[i]) to make the intended
dereference order explicit.
No functional change.
Author: Chao Li <[email protected]>
---
src/backend/utils/adt/datetime.c | 14 +++++++-------
src/include/utils/datetime.h | 2 +-
src/interfaces/ecpg/pgtypeslib/dt.h | 2 +-
src/interfaces/ecpg/pgtypeslib/interval.c | 14 +++++++-------
4 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 90946db72ff..7d845700163 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -3483,7 +3483,7 @@ ClearPgItmIn(struct pg_itm_in *itm_in)
* suffices.
*/
int
-DecodeInterval(char **field, int *ftype, int nf, int range,
+DecodeInterval(char *field[], int *ftype, int nf, int range,
int *dtype, struct pg_itm_in *itm_in)
{
bool force_negative = false;
@@ -3519,13 +3519,13 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
* to dump in postgres style, not SQL style.)
*----------
*/
- if (IntervalStyle == INTSTYLE_SQL_STANDARD && nf > 0 && *field[0] == '-')
+ 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] == '+')
+ if (*(field[i]) == '-' || *(field[i]) == '+')
{
force_negative = false;
break;
@@ -3557,7 +3557,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
* least one digit; there could be ':', '.', '-' embedded in
* it as well.
*/
- Assert(*field[i] == '-' || *field[i] == '+');
+ Assert(*(field[i]) == '-' || *(field[i]) == '+');
/*
* Check for signed hh:mm or hh:mm:ss. If so, process exactly
@@ -3567,7 +3567,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
DecodeTimeForInterval(field[i] + 1, fmask, range,
&tmask, itm_in) == 0)
{
- if (*field[i] == '-')
+ if (*(field[i]) == '-')
{
/* flip the sign on time field */
if (itm_in->tm_usec == PG_INT64_MIN)
@@ -3650,7 +3650,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
if (*cp != '\0')
return DTERR_BAD_FORMAT;
type = DTK_MONTH;
- if (*field[i] == '-')
+ if (*(field[i]) == '-')
val2 = -val2;
if (pg_mul_s64_overflow(val, MONTHS_PER_YEAR, &val))
return DTERR_FIELD_OVERFLOW;
@@ -3663,7 +3663,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
dterr = ParseFraction(cp, &fval);
if (dterr)
return dterr;
- if (*field[i] == '-')
+ if (*(field[i]) == '-')
fval = -fval;
}
else if (*cp == '\0')
diff --git a/src/include/utils/datetime.h b/src/include/utils/datetime.h
index f77c6acd8b6..66dd871fd7f 100644
--- a/src/include/utils/datetime.h
+++ b/src/include/utils/datetime.h
@@ -316,7 +316,7 @@ extern int DecodeTimezone(const char *str, int *tzp);
extern int DecodeTimeOnly(char **field, int *ftype, int nf,
int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
DateTimeErrorExtra *extra);
-extern int DecodeInterval(char **field, int *ftype, int nf, int range,
+extern int DecodeInterval(char *field[], int *ftype, int nf, int range,
int *dtype, struct pg_itm_in *itm_in);
extern int DecodeISO8601Interval(char *str,
int *dtype, struct pg_itm_in *itm_in);
diff --git a/src/interfaces/ecpg/pgtypeslib/dt.h b/src/interfaces/ecpg/pgtypeslib/dt.h
index 00a45799d55..47df0df4739 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt.h
+++ b/src/interfaces/ecpg/pgtypeslib/dt.h
@@ -311,7 +311,7 @@ do { \
#define TIMESTAMP_IS_NOEND(j) ((j) == DT_NOEND)
#define TIMESTAMP_NOT_FINITE(j) (TIMESTAMP_IS_NOBEGIN(j) || TIMESTAMP_IS_NOEND(j))
-int DecodeInterval(char **field, int *ftype, int nf, int *dtype, struct tm *tm, fsec_t *fsec);
+int DecodeInterval(char *field[], int *ftype, int nf, int *dtype, struct tm *tm, fsec_t *fsec);
int DecodeTime(char *str, int *tmask, struct tm *tm, fsec_t *fsec);
void EncodeDateTime(struct tm *tm, fsec_t fsec, bool print_tz, int tz, const char *tzn, int style, char *str, bool EuroDates);
void EncodeInterval(struct tm *tm, fsec_t fsec, int style, char *str);
diff --git a/src/interfaces/ecpg/pgtypeslib/interval.c b/src/interfaces/ecpg/pgtypeslib/interval.c
index e452a088f9e..4d0e75d4dd4 100644
--- a/src/interfaces/ecpg/pgtypeslib/interval.c
+++ b/src/interfaces/ecpg/pgtypeslib/interval.c
@@ -323,7 +323,7 @@ DecodeISO8601Interval(char *str,
* int IntervalStyle = INTSTYLE_POSTGRES;
*/
int
-DecodeInterval(char **field, int *ftype, int nf, /* int range, */
+DecodeInterval(char *field[], int *ftype, int nf, /* int range, */
int *dtype, struct /* pg_ */ tm *tm, fsec_t *fsec)
{
int IntervalStyle = INTSTYLE_POSTGRES_VERBOSE;
@@ -362,7 +362,7 @@ DecodeInterval(char **field, int *ftype, int nf, /* int range, */
* least one digit; there could be ':', '.', '-' embedded in
* it as well.
*/
- Assert(*field[i] == '-' || *field[i] == '+');
+ Assert(*(field[i]) == '-' || *(field[i]) == '+');
/*
* Try for hh:mm or hh:mm:ss. If not, fall through to
@@ -373,7 +373,7 @@ DecodeInterval(char **field, int *ftype, int nf, /* int range, */
DecodeTime(field[i] + 1, /* INTERVAL_FULL_RANGE, */
&tmask, tm, fsec) == 0)
{
- if (*field[i] == '-')
+ if (*(field[i]) == '-')
{
/* flip the sign on all fields */
tm->tm_hour = -tm->tm_hour;
@@ -447,7 +447,7 @@ DecodeInterval(char **field, int *ftype, int nf, /* int range, */
if (*cp != '\0')
return DTERR_BAD_FORMAT;
type = DTK_MONTH;
- if (*field[i] == '-')
+ if (*(field[i]) == '-')
val2 = -val2;
val = val * MONTHS_PER_YEAR + val2;
fval = 0;
@@ -459,7 +459,7 @@ DecodeInterval(char **field, int *ftype, int nf, /* int range, */
if (*cp != '\0' || errno != 0)
return DTERR_BAD_FORMAT;
- if (*field[i] == '-')
+ if (*(field[i]) == '-')
fval = -fval;
}
else if (*cp == '\0')
@@ -622,14 +622,14 @@ DecodeInterval(char **field, int *ftype, int nf, /* int range, */
* to dump in postgres style, not SQL style.)
*----------
*/
- if (IntervalStyle == INTSTYLE_SQL_STANDARD && *field[0] == '-')
+ 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] == '+')
+ if (*(field[i]) == '-' || *(field[i]) == '+')
{
more_signs = true;
break;
--
2.33.0
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: guc: make dereference style consistent in check_backtrace_functions
@ 2026-03-02 03:17 zhanghu <[email protected]>
parent: zhanghu <[email protected]>
0 siblings, 1 reply; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ messages in thread
end of thread, other threads:[~2026-03-24 23:38 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-02-26 12:37 Re: guc: make dereference style consistent in check_backtrace_functions Álvaro Herrera <[email protected]>
2026-02-27 01:33 ` Chao Li <[email protected]>
2026-02-27 08:46 ` zhanghu <[email protected]>
2026-03-02 03:17 ` 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