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

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:
> > On Wed, Nov 26, 2025 at 5:41 PM Amit Langote <amitlangote09(at)gmail(dot)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