[PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date

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:

/messages/by-id/flat/20240724.155525.366150353176322967.ishii@postgresql.org

--
nathan

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:

/messages/by-id/flat/20240724.155525.366150353176322967.ishii@postgresql.org

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

- Amit

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

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 *?

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