Date: Wed, 25 Mar 2026 14:18:00 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Peter Eisentraut <peter@eisentraut.org>
Cc: Jianghua Yang <yjhjstz@gmail.com>, pgsql-hackers@lists.postgresql.org,
	jian.universality@gmail.com, amitlangote09@gmail.com
Subject: Re: [PATCH] Fix wrong argument to SOFT_ERROR_OCCURRED in
 timestamptz_date
Message-ID: <acQ06D_Lz6V93gDg@nathan>
References: 
 <CAAZLFmSGti716gWeY=DCZ9TTVOixnHZ4_4V4tDzoeE86D64vOA@mail.gmail.com>
 <acL5tt3tfsyDql9x@nathan>
 <692b9dba-3060-49e3-99c4-136b1f7cc4e9@eisentraut.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <692b9dba-3060-49e3-99c4-136b1f7cc4e9@eisentraut.org>
Archived-At: <https://www.postgresql.org/message-id/acQ06D_Lz6V93gDg%40nathan>
Precedence: bulk

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