Обсуждение: Refactoring: Use soft error reporting for *_opt_overflow functions of date/timestamp

Hi,

The attached patch proposes to use soft error reporting infrastructure
for the date/timestamp conversion function, which currently depends on
integer variables to control error throwing.

This continues the previous refactoring commit [1] where we adopted
soft error reporting for some numeric functions. This patch applies
the same pattern to the date/timestamp function. The change ensures
consistency by utilizing the existing soft error reporting
infrastructure.

Note that in the patch, I renamed the function by replacing the
"no_overflow" extension in the function name with "overflow_safe".
Alternatively, we could just use "safe" alone. Suggestions are
welcome.

1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=4246a977bad6e76c4276a0d52def8a3dced154bb

--
Regards,
Amul Sul
EDB: http://www.enterprisedb.com

On Wed, Nov 26, 2025 at 03:09:25PM +0530, Amul Sul wrote:
> This continues the previous refactoring commit [1] where we adopted
> soft error reporting for some numeric functions. This patch applies
> the same pattern to the date/timestamp function. The change ensures
> consistency by utilizing the existing soft error reporting
> infrastructure.

Thanks for continuing this work.

> Note that in the patch, I renamed the function by replacing the
> "no_overflow" extension in the function name with "overflow_safe".
> Alternatively, we could just use "safe" alone. Suggestions are
> welcome.

Hmm.  Following the previous example you have quoted, I am wondering
if we'd tweak the names a bit differently.  Rather than the
popo_overflow_safe() pattern from your patch, I would choose a simpler
popo_safe() as naming convention.  That would be also more consistent
with the names applied to the refactored routines of 4246a977bad6.

-   result = date2timestamp_opt_overflow(val, &overflow);
+   result = date2timestamp_overflow_safe(val, (Node *) &escontext);
    /* We can ignore the overflow result, since result is useful as-is */

In these cases, why don't you just pass NULL to the routines for the
error context?  (Sorry, I don't have my eyes on the code now, but I
recall that NULL should work as well, meaning the same as "ignore
me".)
--
Michael

Hi,

On Wed, Nov 26, 2025 at 8:43 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, Nov 26, 2025 at 03:09:25PM +0530, Amul Sul wrote:
> > This continues the previous refactoring commit [1] where we adopted
> > soft error reporting for some numeric functions. This patch applies
> > the same pattern to the date/timestamp function. The change ensures
> > consistency by utilizing the existing soft error reporting
> > infrastructure.
>
> Thanks for continuing this work.

+1

I see that Michael has now noticed this, I was looking at this earlier
today and thought of a couple of nitpicky things to share:

* The rename from *_opt_overflow to *_overflow_safe could be made a
separate patch (say 0002), so it can be discussed separately.  For
example, whether to keep the old *_opt_overflow variants for backward
compatibility since they’re exported and possibly used by extensions.

* Maybe it's just me, but several function comments (for example
around date2timestamptz_overflow_safe()) lost detailed explanations of
overflow behavior. It’d be better to preserve those specifics and only
adjust the wording to describe how errors are reported via escontext:

 /*
- * Promote date to timestamp with time zone.
- *
- * On successful conversion, *overflow is set to zero if it's not NULL.
- *
- * If the date is finite but out of the valid range for timestamptz, then:
- * if overflow is NULL, we throw an out-of-range error.
- * if overflow is not NULL, we store +1 or -1 there to indicate the sign
- * of the overflow, and return the appropriate timestamptz infinity.
+ * Promotes date to timestamp with time zone, including soft error reporting
+ * capabilities.

 /*
- * Convert timestamp to date.
- *
- * On successful conversion, *overflow is set to zero if it's not NULL.
- *
- * If the timestamp is finite but out of the valid range for date, then:
- * if overflow is NULL, we throw an out-of-range error.
- * if overflow is not NULL, we store +1 or -1 there to indicate the sign
- * of the overflow, and return the appropriate date infinity.
+ * Convert timestamp to date, including soft error reporting capabilities.

 /*
- * Convert timestamptz to date.
- *
- * On successful conversion, *overflow is set to zero if it's not NULL.
- *
- * If the timestamptz is finite but out of the valid range for date, then:
- * if overflow is NULL, we throw an out-of-range error.
- * if overflow is not NULL, we store +1 or -1 there to indicate the sign
- * of the overflow, and return the appropriate date infinity.
+ * Convert timestamptz to date, including soft error reporting capabilities.

--
Thanks, Amit Langote

On Wed, Nov 26, 2025 at 5:13 PM Michael Paquier <michael@paquier.xyz> wrote:
>
>
> Hmm.  Following the previous example you have quoted, I am wondering
> if we'd tweak the names a bit differently.  Rather than the
> popo_overflow_safe() pattern from your patch, I would choose a simpler
> popo_safe() as naming convention.  That would be also more consistent
> with the names applied to the refactored routines of 4246a977bad6.
>

The reason for this naming was to maintain consistency with the
function date2timestamp_no_overflow() in date.h. I am now uncertain
whether we should rename date2timestamp_no_overflow() as well to align
with the current change. I also lean towards popo_safe() as a naming
convention.

> -   result = date2timestamp_opt_overflow(val, &overflow);
> +   result = date2timestamp_overflow_safe(val, (Node *) &escontext);
>     /* We can ignore the overflow result, since result is useful as-is */
>
> In these cases, why don't you just pass NULL to the routines for the
> error context?  (Sorry, I don't have my eyes on the code now, but I
> recall that NULL should work as well, meaning the same as "ignore
> me".)

Won't that result in an error that we are trying to avoid?

Regards,
Amul

On Wed, Nov 26, 2025 at 5:41 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi,
>
> On Wed, Nov 26, 2025 at 8:43 PM Michael Paquier <michael@paquier.xyz> wrote:
> > On Wed, Nov 26, 2025 at 03:09:25PM +0530, Amul Sul wrote:
> > > This continues the previous refactoring commit [1] where we adopted
> > > soft error reporting for some numeric functions. This patch applies
> > > the same pattern to the date/timestamp function. The change ensures
> > > consistency by utilizing the existing soft error reporting
> > > infrastructure.
> >
> > Thanks for continuing this work.
>
> +1

Thank you both for taking a look at the patch.

>
> I see that Michael has now noticed this, I was looking at this earlier
> today and thought of a couple of nitpicky things to share:
>
> * The rename from *_opt_overflow to *_overflow_safe could be made a
> separate patch (say 0002), so it can be discussed separately.  For
> example, whether to keep the old *_opt_overflow variants for backward
> compatibility since they’re exported and possibly used by extensions.
>

I am probably okay with this, but it is up to the committer. In the
previous commit, however, we performed a rename within the same
commit. IIUC, the extensions are expected to be updated with respect
to the major release

> * Maybe it's just me, but several function comments (for example
> around date2timestamptz_overflow_safe()) lost detailed explanations of
> overflow behavior. It’d be better to preserve those specifics and only
> adjust the wording to describe how errors are reported via escontext:
>

The previous comments are no longer relevant now that we are utilizing
the established error-safe infrastructure, but, I would think more on
this later since I am out of time today.

Regards,
Amul

On Wed, Nov 26, 2025 at 06:40:38PM +0530, Amul Sul wrote:
> On Wed, Nov 26, 2025 at 5:41 PM Amit Langote <amitlangote09@gmail.com> wrote:
>> * The rename from *_opt_overflow to *_overflow_safe could be made a
>> separate patch (say 0002), so it can be discussed separately.  For
>> example, whether to keep the old *_opt_overflow variants for backward
>> compatibility since they’re exported and possibly used by extensions.
>
> I am probably okay with this, but it is up to the committer. In the
> previous commit, however, we performed a rename within the same
> commit. IIUC, the extensions are expected to be updated with respect
> to the major release

I am not sure to see a point in doing a rename of the routines
separately.  We are changing one of the argument type of the
functions, replacing the "overflow" integer with an error context
Node.  If we were to do a rename in one patch, then a redesign of the
arguments, this leads to more code churn at the same end as the same
code paths would get rewritten twice instead of once.  This move could
have made more sense if the existing popo_opt_overflow() used NULL for
the overflow value like in btree_gin.c in the past, but that makes the
change less appealing with the soft reporting APIs available in the
core backend.

>> * Maybe it's just me, but several function comments (for example
>> around date2timestamptz_overflow_safe()) lost detailed explanations of
>> overflow behavior. It’d be better to preserve those specifics and only
>> adjust the wording to describe how errors are reported via escontext:
>
> The previous comments are no longer relevant now that we are utilizing
> the established error-safe infrastructure, but, I would think more on
> this later since I am out of time today.

It seems to me that it is important to keep documented the overflow
check in some way rather than removing it as the patch is doing,
particularly regarding the finite vs infinite value behaviors.  We do
not need anymore the documentation about how "overflow" is set in this
routines, of course, but keeping these expectations documented would
be better.
--
Michael

On Thu, Nov 27, 2025 at 7:59 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, Nov 26, 2025 at 06:40:38PM +0530, Amul Sul wrote:
> > On Wed, Nov 26, 2025 at 5:41 PM Amit Langote <amitlangote09@gmail.com> wrote:
> >> * The rename from *_opt_overflow to *_overflow_safe could be made a
> >> separate patch (say 0002), so it can be discussed separately.  For
> >> example, whether to keep the old *_opt_overflow variants for backward
> >> compatibility since they’re exported and possibly used by extensions.
> >
> > I am probably okay with this, but it is up to the committer. In the
> > previous commit, however, we performed a rename within the same
> > commit. IIUC, the extensions are expected to be updated with respect
> > to the major release
>
> I am not sure to see a point in doing a rename of the routines
> separately.  We are changing one of the argument type of the
> functions, replacing the "overflow" integer with an error context
> Node.  If we were to do a rename in one patch, then a redesign of the
> arguments, this leads to more code churn at the same end as the same
> code paths would get rewritten twice instead of once.

Ok, let's drop the patch breakdown part of my comment then.

>  This move could
> have made more sense if the existing popo_opt_overflow() used NULL for
> the overflow value like in btree_gin.c in the past, but that makes the
> change less appealing with the soft reporting APIs available in the
> core backend.

I’m fine with updating all core callers to use the new *_safe(... Node
*escontext) APIs all in one patch. However, we could consider keeping
the existing *_opt_overflow() functions as thin wrappers over the new
ones, to avoid breaking third-party extensions immediately for what is
primarily a refactoring change.

> >> * Maybe it's just me, but several function comments (for example
> >> around date2timestamptz_overflow_safe()) lost detailed explanations of
> >> overflow behavior. It’d be better to preserve those specifics and only
> >> adjust the wording to describe how errors are reported via escontext:
> >
> > The previous comments are no longer relevant now that we are utilizing
> > the established error-safe infrastructure, but, I would think more on
> > this later since I am out of time today.
>
> It seems to me that it is important to keep documented the overflow
> check in some way rather than removing it as the patch is doing,
> particularly regarding the finite vs infinite value behaviors.  We do
> not need anymore the documentation about how "overflow" is set in this
> routines, of course, but keeping these expectations documented would
> be better.

Yeah, I meant we should expand "including soft error reporting
capabilities" somehow, something like this:

- * On successful conversion, *overflow is set to zero if it's not NULL.
- *
- * If the date is finite but out of the valid range for timestamp, then:
- * if overflow is NULL, we throw an out-of-range error.
- * if overflow is not NULL, we store +1 or -1 there to indicate the sign
- * of the overflow, and return the appropriate timestamp infinity.
+ * If the date is finite but out of the valid range for timestamp, an
+ * out-of-range error is reported.  When escontext is NULL this is a
+ * normal ERROR; when escontext points to an ErrorSaveContext, the error
+ * is reported softly and TIMESTAMP_NOEND is returned.

--
Thanks, Amit Langote

On Thu, Nov 27, 2025 at 10:18:38AM +0900, Amit Langote wrote:
> I’m fine with updating all core callers to use the new *_safe(... Node
> *escontext) APIs all in one patch. However, we could consider keeping
> the existing *_opt_overflow() functions as thin wrappers over the new
> ones, to avoid breaking third-party extensions immediately for what is
> primarily a refactoring change.

Like 4246a977bad6, one point is to be able to make the internals of
these functions leaner, by relying on the fact that the econtext node
includes an error to let the callers of the functions decide what to
do.  If we keep some function wrappers as the ones we have now, this
partially defeats the purpose of the change, which is to simplify
in-core code at the end.  This move also encourages users to switch to
the new facility to be fed error reports generated by the in-core
routines, meaning less duplication with error strings outside of the
core code, hence less maintenance for out-of-core code.
--
Michael

On Thu, Nov 27, 2025 at 10:28 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Thu, Nov 27, 2025 at 10:18:38AM +0900, Amit Langote wrote:
> > I’m fine with updating all core callers to use the new *_safe(... Node
> > *escontext) APIs all in one patch. However, we could consider keeping
> > the existing *_opt_overflow() functions as thin wrappers over the new
> > ones, to avoid breaking third-party extensions immediately for what is
> > primarily a refactoring change.
>
> Like 4246a977bad6, one point is to be able to make the internals of
> these functions leaner, by relying on the fact that the econtext node
> includes an error to let the callers of the functions decide what to
> do.  If we keep some function wrappers as the ones we have now, this
> partially defeats the purpose of the change, which is to simplify
> in-core code at the end.  This move also encourages users to switch to
> the new facility to be fed error reports generated by the in-core
> routines, meaning less duplication with error strings outside of the
> core code, hence less maintenance for out-of-core code.

Ok, I see, the idea is to encourage out-of-core code to move to the
new ErrorSaveContext-based handling, rather than keep wrappers around
just to ease the transition. IOW, we shouldn’t carry redundant code in
core once the new, simpler APIs exist. Sounds ok to me.  Some
extension authors might grumble when their code stops compiling with
v19 though,only to find that the change was mostly stylistic ;-).

--
Thanks, Amit Langote

On Thu, Nov 27, 2025 at 6:48 AM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Thu, Nov 27, 2025 at 7:59 AM Michael Paquier <michael@paquier.xyz> wrote:
> > On Wed, Nov 26, 2025 at 06:40:38PM +0530, Amul Sul wrote:
> > > On Wed, Nov 26, 2025 at 5:41 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > >> [....]
> >
> > It seems to me that it is important to keep documented the overflow
> > check in some way rather than removing it as the patch is doing,
> > particularly regarding the finite vs infinite value behaviors.  We do
> > not need anymore the documentation about how "overflow" is set in this
> > routines, of course, but keeping these expectations documented would
> > be better.
>
> Yeah, I meant we should expand "including soft error reporting
> capabilities" somehow, something like this:
>
> - * On successful conversion, *overflow is set to zero if it's not NULL.
> - *
> - * If the date is finite but out of the valid range for timestamp, then:
> - * if overflow is NULL, we throw an out-of-range error.
> - * if overflow is not NULL, we store +1 or -1 there to indicate the sign
> - * of the overflow, and return the appropriate timestamp infinity.
> + * If the date is finite but out of the valid range for timestamp, an
> + * out-of-range error is reported.  When escontext is NULL this is a
> + * normal ERROR; when escontext points to an ErrorSaveContext, the error
> + * is reported softly and TIMESTAMP_NOEND is returned.
>

Okay, I have attached an updated version -- added the necessary
comments and renamed the function, replacing "opt_overflow" with the
"_safe".

One question: Regarding date2timestamp_no_overflow(), should we rename
it to date2double? We can't use date2timestamp_safe because we already
have that function. The renaming is relevant because this function
converts a date to the double data type, which allows us to remove the
"_no_overflow" extension.

Regards,
Amul

On Thu, Nov 27, 2025 at 4:58 PM Amul Sul <sulamul@gmail.com> wrote:
> On Thu, Nov 27, 2025 at 6:48 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Thu, Nov 27, 2025 at 7:59 AM Michael Paquier <michael@paquier.xyz> wrote:
> > > On Wed, Nov 26, 2025 at 06:40:38PM +0530, Amul Sul wrote:
> > > It seems to me that it is important to keep documented the overflow
> > > check in some way rather than removing it as the patch is doing,
> > > particularly regarding the finite vs infinite value behaviors.  We do
> > > not need anymore the documentation about how "overflow" is set in this
> > > routines, of course, but keeping these expectations documented would
> > > be better.
> >
> > Yeah, I meant we should expand "including soft error reporting
> > capabilities" somehow, something like this:
> >
> > - * On successful conversion, *overflow is set to zero if it's not NULL.
> > - *
> > - * If the date is finite but out of the valid range for timestamp, then:
> > - * if overflow is NULL, we throw an out-of-range error.
> > - * if overflow is not NULL, we store +1 or -1 there to indicate the sign
> > - * of the overflow, and return the appropriate timestamp infinity.
> > + * If the date is finite but out of the valid range for timestamp, an
> > + * out-of-range error is reported.  When escontext is NULL this is a
> > + * normal ERROR; when escontext points to an ErrorSaveContext, the error
> > + * is reported softly and TIMESTAMP_NOEND is returned.
> >
>
> Okay, I have attached an updated version -- added the necessary
> comments and renamed the function, replacing "opt_overflow" with the
> "_safe".

Thanks.

LGTM beside minor nits:

+ * Promotes date to timestamp, including soft error reporting capabilities.

The part after the comma looks unnecessary IMO.  It's clear from the
rest of the description that the function has soft error reporting
capabilities.

+ * handling proceeds based on the error save context:

I don't see "error save context" anywhere else in the code.  Why not
just use 'escontext' instead or just say "proceeds as follows:"?

> One question: Regarding date2timestamp_no_overflow(), should we rename
> it to date2double? We can't use date2timestamp_safe because we already
> have that function. The renaming is relevant because this function
> converts a date to the double data type, which allows us to remove the
> "_no_overflow" extension.

Makes sense to me.

--
Thanks, Amit Langote

On Thu, Nov 27, 2025 at 06:18:32PM +0900, Amit Langote wrote:
> On Thu, Nov 27, 2025 at 4:58 PM Amul Sul <sulamul@gmail.com> wrote:
>> One question: Regarding date2timestamp_no_overflow(), should we rename
>> it to date2double? We can't use date2timestamp_safe because we already
>> have that function. The renaming is relevant because this function
>> converts a date to the double data type, which allows us to remove the
>> "_no_overflow" extension.
>
> Makes sense to me.

Yes, it would be nice to change all this area at once to remain
consistent across the board.  date2timestamp_no_overflow() is the last
"no_overflow" routine in date.h.
--
Michael

On Fri, Nov 28, 2025 at 6:16 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Nov 27, 2025 at 06:18:32PM +0900, Amit Langote wrote:
> > On Thu, Nov 27, 2025 at 4:58 PM Amul Sul <sulamul@gmail.com> wrote:
> >> One question: Regarding date2timestamp_no_overflow(), should we rename
> >> it to date2double? We can't use date2timestamp_safe because we already
> >> have that function. The renaming is relevant because this function
> >> converts a date to the double data type, which allows us to remove the
> >> "_no_overflow" extension.
> >
> > Makes sense to me.
>
> Yes, it would be nice to change all this area at once to remain
> consistent across the board.  date2timestamp_no_overflow() is the last
> "no_overflow" routine in date.h.

I have attached patch 0002 that renames it. I also updated patch 0001
to accommodate Amit's comment suggestions.

Regards,
Amul

On Fri, Nov 28, 2025 at 09:46:43AM +0530, Amul Sul wrote:
> I have attached patch 0002 that renames it. I also updated patch 0001
> to accommodate Amit's comment suggestions.

Thanks, applied this one after more tweaks.  Regarding 0002, just
doing a renaming makes me a bit uncomfortable after a second look.
Another way to look at the problem while being consistent would be to
convert date2timestamp_no_overflow() to use soft error reports,
requiring its caller in selfuncs.c to use an error context node.  I
cannot get really excited at the end just for the sake of the planner
stats.

There were two more functions that btree_gin.c is pointing at that
could to the switch: timestamp->timestamptz and its opposite.  This
also shaves some code, which is nice.  Please see the attached.
--
Michael

On Mon, Dec 1, 2025 at 12:32 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Nov 28, 2025 at 09:46:43AM +0530, Amul Sul wrote:
> > I have attached patch 0002 that renames it. I also updated patch 0001
> > to accommodate Amit's comment suggestions.
>
> Thanks, applied this one after more tweaks.  Regarding 0002, just
> doing a renaming makes me a bit uncomfortable after a second look.
> Another way to look at the problem while being consistent would be to
> convert date2timestamp_no_overflow() to use soft error reports,
> requiring its caller in selfuncs.c to use an error context node.  I
> cannot get really excited at the end just for the sake of the planner
> stats.
>

Understood. Thanks for committing the patch.

> There were two more functions that btree_gin.c is pointing at that
> could to the switch: timestamp->timestamptz and its opposite.  This
> also shaves some code, which is nice.  Please see the attached.

Yes, the patch looks good to me.

Regards,
Amul

On Mon, Dec 01, 2025 at 05:31:43PM +0530, Amul Sul wrote:
> On Mon, Dec 1, 2025 at 12:32 PM Michael Paquier <michael@paquier.xyz> wrote:
>> There were two more functions that btree_gin.c is pointing at that
>> could to the switch: timestamp->timestamptz and its opposite.  This
>> also shaves some code, which is nice.  Please see the attached.
>
> Yes, the patch looks good to me.

Thanks for double-checking.  Applied this one as well.
--
Michael

On Mon, Dec 1, 2025 at 9:02 PM Amul Sul <sulamul@gmail.com> wrote:
> On Mon, Dec 1, 2025 at 12:32 PM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Fri, Nov 28, 2025 at 09:46:43AM +0530, Amul Sul wrote:
> > > I have attached patch 0002 that renames it. I also updated patch 0001
> > > to accommodate Amit's comment suggestions.
> >
> > Thanks, applied this one after more tweaks.  Regarding 0002, just
> > doing a renaming makes me a bit uncomfortable after a second look.
> > Another way to look at the problem while being consistent would be to
> > convert date2timestamp_no_overflow() to use soft error reports,
> > requiring its caller in selfuncs.c to use an error context node.  I
> > cannot get really excited at the end just for the sake of the planner
> > stats.
> >
>
> Understood. Thanks for committing the patch.

+1, thanks Michael for taking care of this and Amul too.

--
Thanks, Amit Langote