Thread

Commits

Fix typo
- c79e4141273c 19 (unreleased) landed
Make many cast functions error safe
- e2f289e5b9b8 19 (unreleased) cited

[PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date

Jianghua Yang <yjhjstz@gmail.com> — 2026-03-24T15:44:29Z

 Hi hackers,

  I found a small bug in commit e2f289e5b9b ("Make many cast functions
error safe").

  In timestamptz_date(), the SOFT_ERROR_OCCURRED() check mistakenly
  uses fcinfo->args instead of fcinfo->context:

  result = timestamptz2date_safe(timestamp, fcinfo->context);
  if (SOFT_ERROR_OCCURRED(fcinfo->args))   /* should be fcinfo->context */
      PG_RETURN_NULL();

  fcinfo->args is a NullableDatum[] array, not a Node *. The
  SOFT_ERROR_OCCURRED macro casts its argument to Node * and reads
  the NodeTag field. When given fcinfo->args, it interprets the first
  argument's Datum value (a TimestampTz) as a NodeTag, which will
  almost never match T_ErrorSaveContext. This causes the soft error
  check to always evaluate to false.

  As a result, when the timestamptz-to-date conversion encounters an
  overflow in error-safe mode, the function returns a wrong date value
  instead of the expected NULL.

  All three sibling functions modified in the same commit (date_timestamp,
  timestamp_date, date_timestamptz) correctly use fcinfo->context.
  This appears to be a copy-paste oversight.

  The fix is a one-line change: fcinfo->args → fcinfo->context.

Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date

Nathan Bossart <nathandbossart@gmail.com> — 2026-03-24T20:53:10Z

On Tue, Mar 24, 2026 at 08:44:29AM -0700, Jianghua Yang wrote:
>   I found a small bug in commit e2f289e5b9b ("Make many cast functions
> error safe").

Nice find.  For future reference, since this was just committed, it
might've been better to report it directly in the thread where the change
was discussed.

>   The fix is a one-line change: fcinfo->args → fcinfo->context.

LGTM.  To prevent this from happening in the future, I think we ought to
change SOFT_ERROR_OCCURRED to a static inline function.  I tried that, and
I got the following warnings:

    execExprInterp.c:4964:27: warning: incompatible pointer types passing 'ErrorSaveContext *' (aka 'struct ErrorSaveContext *') to parameter of type 'Node *' (aka 'struct Node *') [-Wincompatible-pointer-types]
     4964 |                 if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
          |                                         ^~~~~~~~~~~~~~~~~~~~
    ../../../src/include/nodes/miscnodes.h:54:27: note: passing argument to parameter 'escontext' here
       54 | SOFT_ERROR_OCCURRED(Node *escontext)
          |                           ^
    execExprInterp.c:5200:26: warning: incompatible pointer types passing 'ErrorSaveContext *' (aka 'struct ErrorSaveContext *') to parameter of type 'Node *' (aka 'struct Node *') [-Wincompatible-pointer-types]
     5200 |         if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
          |                                 ^~~~~~~~~~~~~~~~~~~~
    ../../../src/include/nodes/miscnodes.h:54:27: note: passing argument to parameter 'escontext' here
       54 | SOFT_ERROR_OCCURRED(Node *escontext)
          |                           ^

I think we just need to add casts to "Node *" for those.  AFAICT there
isn't an actual bug.

[... looks for past discussions ...]

Ah, I noticed this thread, where the same lines of code were discussed:

	https://postgr.es/m/flat/20240724.155525.366150353176322967.ishii%40postgresql.org

-- 
nathan

Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date

Amit Langote <amitlangote09@gmail.com> — 2026-03-25T03:13:55Z

 On Wed, Mar 25, 2026 at 5:53 Nathan Bossart <nathandbossart@gmail.com>
wrote:

> On Tue, Mar 24, 2026 at 08:44:29AM -0700, Jianghua Yang wrote:
> >   I found a small bug in commit e2f289e5b9b ("Make many cast functions
> > error safe").
>
> Nice find.  For future reference, since this was just committed, it
> might've been better to report it directly in the thread where the change
> was discussed.
>
> >   The fix is a one-line change: fcinfo->args → fcinfo->context.
>
> LGTM.  To prevent this from happening in the future, I think we ought to
> change SOFT_ERROR_OCCURRED to a static inline function.  I tried that, and
> I got the following warnings:
>
>     execExprInterp.c:4964:27: warning: incompatible pointer types passing
> 'ErrorSaveContext *' (aka 'struct ErrorSaveContext *') to parameter of type
> 'Node *' (aka 'struct Node *') [-Wincompatible-pointer-types]
>      4964 |                 if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
>           |                                         ^~~~~~~~~~~~~~~~~~~~
>     ../../../src/include/nodes/miscnodes.h:54:27: note: passing argument
> to parameter 'escontext' here
>        54 | SOFT_ERROR_OCCURRED(Node *escontext)
>           |                           ^
>     execExprInterp.c:5200:26: warning: incompatible pointer types passing
> 'ErrorSaveContext *' (aka 'struct ErrorSaveContext *') to parameter of type
> 'Node *' (aka 'struct Node *') [-Wincompatible-pointer-types]
>      5200 |         if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
>           |                                 ^~~~~~~~~~~~~~~~~~~~
>     ../../../src/include/nodes/miscnodes.h:54:27: note: passing argument
> to parameter 'escontext' here
>        54 | SOFT_ERROR_OCCURRED(Node *escontext)
>           |                           ^
>
> I think we just need to add casts to "Node *" for those.  AFAICT there
> isn't an actual bug.


That seems ok to me.

[... looks for past discussions ...]
>
> Ah, I noticed this thread, where the same lines of code were discussed:
>
>
> https://postgr.es/m/flat/20240724.155525.366150353176322967.ishii%40postgresql.org


ISTM the fix proposed by Ishii-san in that thread is the same thing, but
yours LGTM too.

- Amit

>
> <https://postgr.es/m/flat/20240724.155525.366150353176322967.ishii%40postgresql.org>

Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date

Peter Eisentraut <peter@eisentraut.org> — 2026-03-25T06:16:26Z

On 24.03.26 16:44, Jianghua Yang wrote:
>   Hi hackers,
> 
>    I found a small bug in commit e2f289e5b9b ("Make many cast functions 
> error safe").
> 
>    In timestamptz_date(), the SOFT_ERROR_OCCURRED() check mistakenly
>    uses fcinfo->args instead of fcinfo->context:
> 
>    result = timestamptz2date_safe(timestamp, fcinfo->context);
>    if (SOFT_ERROR_OCCURRED(fcinfo->args))   /* should be fcinfo->context */
>        PG_RETURN_NULL();
> 
>    fcinfo->args is a NullableDatum[] array, not a Node *. The
>    SOFT_ERROR_OCCURRED macro casts its argument to Node * and reads
>    the NodeTag field. When given fcinfo->args, it interprets the first
>    argument's Datum value (a TimestampTz) as a NodeTag, which will
>    almost never match T_ErrorSaveContext. This causes the soft error
>    check to always evaluate to false.
> 
>    As a result, when the timestamptz-to-date conversion encounters an
>    overflow in error-safe mode, the function returns a wrong date value
>    instead of the expected NULL.
> 
>    All three sibling functions modified in the same commit (date_timestamp,
>    timestamp_date, date_timestamptz) correctly use fcinfo->context.
>    This appears to be a copy-paste oversight.
> 
>    The fix is a one-line change: fcinfo->args → fcinfo->context.

committed the fix, thanks

Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date

Peter Eisentraut <peter@eisentraut.org> — 2026-03-25T06:17:15Z

On 24.03.26 21:53, Nathan Bossart wrote:
> LGTM.  To prevent this from happening in the future, I think we ought to
> change SOFT_ERROR_OCCURRED to a static inline function.  I tried that, and
> I got the following warnings:
> 
>      execExprInterp.c:4964:27: warning: incompatible pointer types passing 'ErrorSaveContext *' (aka 'struct ErrorSaveContext *') to parameter of type 'Node *' (aka 'struct Node *') [-Wincompatible-pointer-types]
>       4964 |                 if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
>            |                                         ^~~~~~~~~~~~~~~~~~~~
>      ../../../src/include/nodes/miscnodes.h:54:27: note: passing argument to parameter 'escontext' here
>         54 | SOFT_ERROR_OCCURRED(Node *escontext)
>            |                           ^
>      execExprInterp.c:5200:26: warning: incompatible pointer types passing 'ErrorSaveContext *' (aka 'struct ErrorSaveContext *') to parameter of type 'Node *' (aka 'struct Node *') [-Wincompatible-pointer-types]
>       5200 |         if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
>            |                                 ^~~~~~~~~~~~~~~~~~~~
>      ../../../src/include/nodes/miscnodes.h:54:27: note: passing argument to parameter 'escontext' here
>         54 | SOFT_ERROR_OCCURRED(Node *escontext)
>            |                           ^
> 
> I think we just need to add casts to "Node *" for those.  AFAICT there
> isn't an actual bug.

Or maybe we change the escontext field to be of type Node *?

Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date

Nathan Bossart <nathandbossart@gmail.com> — 2026-03-25T19:18:00Z

On Wed, Mar 25, 2026 at 07:17:15AM +0100, Peter Eisentraut wrote:
> On 24.03.26 21:53, Nathan Bossart wrote:
>> LGTM.  To prevent this from happening in the future, I think we ought to
>> change SOFT_ERROR_OCCURRED to a static inline function.  I tried that, and
>> I got the following warnings:
>> 
>>      execExprInterp.c:4964:27: warning: incompatible pointer types passing 'ErrorSaveContext *' (aka 'struct ErrorSaveContext *') to parameter of type 'Node *' (aka 'struct Node *') [-Wincompatible-pointer-types]
>>       4964 |                 if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
>>            |                                         ^~~~~~~~~~~~~~~~~~~~
>>      ../../../src/include/nodes/miscnodes.h:54:27: note: passing argument to parameter 'escontext' here
>>         54 | SOFT_ERROR_OCCURRED(Node *escontext)
>>            |                           ^
>>      execExprInterp.c:5200:26: warning: incompatible pointer types passing 'ErrorSaveContext *' (aka 'struct ErrorSaveContext *') to parameter of type 'Node *' (aka 'struct Node *') [-Wincompatible-pointer-types]
>>       5200 |         if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
>>            |                                 ^~~~~~~~~~~~~~~~~~~~
>>      ../../../src/include/nodes/miscnodes.h:54:27: note: passing argument to parameter 'escontext' here
>>         54 | SOFT_ERROR_OCCURRED(Node *escontext)
>>            |                           ^
>> 
>> I think we just need to add casts to "Node *" for those.  AFAICT there
>> isn't an actual bug.
> 
> Or maybe we change the escontext field to be of type Node *?

I started looking at this, but it seems to be a rather invasive change for
the level of gain.  Not only does it require more memory management, but we
then have to cast it many places like this:

    ((ErrorSaveContext *) jsestate->escontext)->error_occured = false;

If we instead make it an ErrorSaveContext *, we'd still need to cast it to
Node * for SOFT_ERROR_OCCURRED, unless we had it accept a void * or
something, which defeats the purpose.

-- 
nathan