public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Álvaro Herrera <[email protected]>
Cc: Junwang Zhao <[email protected]>
Cc: zhanghu <[email protected]>
Cc: [email protected]
Subject: Re: guc: make dereference style consistent in check_backtrace_functions
Date: Fri, 27 Feb 2026 09:33:45 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[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;
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: guc: make dereference style consistent in check_backtrace_functions
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox