public inbox for [email protected]help / color / mirror / Atom feed
Re: guc: make dereference style consistent in check_backtrace_functions 5+ 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; 5+ 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] 5+ 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; 5+ 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] 5+ 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; 5+ 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] 5+ 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; 5+ 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] 5+ 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; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2026-03-04 10:31 UTC | newest] Thread overview: 5+ 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]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox