public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date
5+ messages / 4 participants
[nested] [flat]
* [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date
@ 2026-03-24 15:44 Jianghua Yang <[email protected]>
2026-03-24 20:53 ` Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date Nathan Bossart <[email protected]>
2026-03-25 06:16 ` Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date Peter Eisentraut <[email protected]>
0 siblings, 2 replies; 5+ messages in thread
From: Jianghua Yang @ 2026-03-24 15:44 UTC (permalink / raw)
To: [email protected]
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.
Attachments:
[application/octet-stream] v1-0001-Fix-wrong-argument-to-SOFT_ERROR_OCCURRED-in-time.patch (1.6K, 3-v1-0001-Fix-wrong-argument-to-SOFT_ERROR_OCCURRED-in-time.patch)
download | inline diff:
From f553acbaafa8c06eb2a37cadff1c838aad3cb70f Mon Sep 17 00:00:00 2001
From: Jianghua Yang <[email protected]>
Date: Tue, 24 Mar 2026 08:35:32 -0700
Subject: [PATCH v1] Fix wrong argument to SOFT_ERROR_OCCURRED in
timestamptz_date
In commit e2f289e5b9b, which made many cast functions error safe,
timestamptz_date() mistakenly passes fcinfo->args to the
SOFT_ERROR_OCCURRED() macro instead of fcinfo->context.
fcinfo->args is a NullableDatum[] array, not a Node pointer. The macro
casts its argument to Node* and checks the NodeTag field. When given
fcinfo->args, it reads the first argument's Datum value as a NodeTag,
which will almost certainly not match T_ErrorSaveContext, causing 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 will return a garbage 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.
Author: Jianghua Yang <[email protected]>
---
src/backend/utils/adt/date.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 71ea048d251..c3327440380 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -1402,7 +1402,7 @@ timestamptz_date(PG_FUNCTION_ARGS)
DateADT result;
result = timestamptz2date_safe(timestamp, fcinfo->context);
- if (SOFT_ERROR_OCCURRED(fcinfo->args))
+ if (SOFT_ERROR_OCCURRED(fcinfo->context))
PG_RETURN_NULL();
PG_RETURN_DATEADT(result);
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date
2026-03-24 15:44 [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date Jianghua Yang <[email protected]>
@ 2026-03-24 20:53 ` Nathan Bossart <[email protected]>
2026-03-25 03:13 ` Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date Amit Langote <[email protected]>
2026-03-25 19:18 ` Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date Nathan Bossart <[email protected]>
1 sibling, 2 replies; 5+ messages in thread
From: Nathan Bossart @ 2026-03-24 20:53 UTC (permalink / raw)
To: Jianghua Yang <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date
2026-03-24 15:44 [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date Jianghua Yang <[email protected]>
2026-03-24 20:53 ` Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date Nathan Bossart <[email protected]>
@ 2026-03-25 03:13 ` Amit Langote <[email protected]>
1 sibling, 0 replies; 5+ messages in thread
From: Amit Langote @ 2026-03-25 03:13 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Jianghua Yang <[email protected]>; [email protected]; [email protected]; [email protected]
On Wed, Mar 25, 2026 at 5:53 Nathan Bossart <[email protected]>
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;
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date
2026-03-24 15:44 [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date Jianghua Yang <[email protected]>
2026-03-24 20:53 ` Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date Nathan Bossart <[email protected]>
@ 2026-03-25 19:18 ` Nathan Bossart <[email protected]>
1 sibling, 0 replies; 5+ messages in thread
From: Nathan Bossart @ 2026-03-25 19:18 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Jianghua Yang <[email protected]>; [email protected]; [email protected]; [email protected]
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date
2026-03-24 15:44 [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date Jianghua Yang <[email protected]>
@ 2026-03-25 06:16 ` Peter Eisentraut <[email protected]>
1 sibling, 0 replies; 5+ messages in thread
From: Peter Eisentraut @ 2026-03-25 06:16 UTC (permalink / raw)
To: Jianghua Yang <[email protected]>; [email protected]
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
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-03-25 19:18 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-03-24 15:44 [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in timestamptz_date Jianghua Yang <[email protected]>
2026-03-24 20:53 ` Nathan Bossart <[email protected]>
2026-03-25 03:13 ` Amit Langote <[email protected]>
2026-03-25 19:18 ` Nathan Bossart <[email protected]>
2026-03-25 06:16 ` Peter Eisentraut <[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