On Thu, Nov 27, 2025 at 7:59 AM Michael Paquier <[email protected]> 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 <[email protected]> > > 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
