Re: Refactoring: Use soft error reporting for *_opt_overflow functions of date/timestamp

On Thu, Nov 27, 2025 at 4:58 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> On Thu, Nov 27, 2025 at 6:48 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > On Thu, Nov 27, 2025 at 7:59 AM Michael Paquier <michael(at)paquier(dot)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