Обсуждение: Refactoring: Use soft error reporting for *_opt_error functions
Hi, While reviewing version v6 of the CAST(... ON DEFAULT) patch [1], I noticed that it attempts to change the type conversion function to use soft error reporting. However, some of the underlying functions of the type conversion, such as *_opt_error, still rely on a boolean argument passed by the caller. This results in an error flag being set rather than a proper error being thrown. I believe we should update all *_opt_error functions to use the new soft error reporting infrastructure instead of boolean flags -- did the same in the attached patch. I am not sure if this patch should be part of that thread[1]. It's a significant improvement in itself, as it would make the code more compact and consistent. 1]http://postgr.es/m/CACJufxE053=bO3pDUpGba6Yz3VGpU_XCbg4HO6Rew5EJ7k7VnQ@mail.gmail.com -- Regards, Amul Sul EDB: http://www.enterprisedb.com
Вложения
On Mon, 1 Sept 2025 at 10:36, Amul Sul <sulamul@gmail.com> wrote: > > I believe we should update all *_opt_error functions to use the new > soft error reporting infrastructure instead of boolean flags -- did > the same in the attached patch. I am not sure if this patch should be > part of that thread[1]. It's a significant improvement in itself, as > it would make the code more compact and consistent. Agreed. That does look neater. Regards, Dean
On Mon, Sep 01, 2025 at 12:21:18PM +0100, Dean Rasheed wrote: > On Mon, 1 Sept 2025 at 10:36, Amul Sul <sulamul@gmail.com> wrote: >> I believe we should update all *_opt_error functions to use the new >> soft error reporting infrastructure instead of boolean flags -- did >> the same in the attached patch. I am not sure if this patch should be >> part of that thread[1]. It's a significant improvement in itself, as >> it would make the code more compact and consistent. Handling that as a separate patch seems OK here. Thanks for caring. > Agreed. That does look neater. Yep. More consistent. +/* forward declarations to avoid node.h include */ +typedef struct Node Node; s/declarations/declaration/. Singular required, only one declaration. Looking at the surroundings, would it make sense to do the same for pg_lsn_in_internal()? It requires a safe fallback when parsing the LSN from the recovery_target_lsn GUC. -- Michael
Вложения
On Tue, Sep 2, 2025 at 9:46 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Sep 01, 2025 at 12:21:18PM +0100, Dean Rasheed wrote: > > On Mon, 1 Sept 2025 at 10:36, Amul Sul <sulamul@gmail.com> wrote: > >> I believe we should update all *_opt_error functions to use the new > >> soft error reporting infrastructure instead of boolean flags -- did > >> the same in the attached patch. I am not sure if this patch should be > >> part of that thread[1]. It's a significant improvement in itself, as > >> it would make the code more compact and consistent. > > Handling that as a separate patch seems OK here. Thanks for caring. > > > Agreed. That does look neater. > > Yep. More consistent. > > +/* forward declarations to avoid node.h include */ > +typedef struct Node Node; > > s/declarations/declaration/. Singular required, only one declaration. > Ok, will fix that. > Looking at the surroundings, would it make sense to do the same for > pg_lsn_in_internal()? It requires a safe fallback when parsing the > LSN from the recovery_target_lsn GUC. Yeah, it makes sense, will change in the next version. Just a quick question regarding the naming conventions. It looks like we have a choice between two options for consistency. Should we rename the pg_lsn_in_internal function by replacing "_internal" with "_safe", or should we rename all of the *_opt_error functions by replacing "_opt_error" with "_internal"? I would choose the latter option. Regards, Amul
On Tue, Sep 02, 2025 at 12:40:25PM +0530, Amul Sul wrote: > Just a quick question regarding the naming conventions. It looks like > we have a choice between two options for consistency. Should we rename > the pg_lsn_in_internal function by replacing "_internal" with "_safe", > or should we rename all of the *_opt_error functions by replacing > "_opt_error" with "_internal"? > > I would choose the latter option. Applying "_safe" seems a bit more consistent to me, as per past changes like ccff2d20ed96, also looking at the functions that are given a ErrorSaveContext in input. I am ready to believe that there are not a lot of callers of the existing _opt_error() routines listed in numeric.h, so a renaming may be better to let existing callers know about the change. These predate the introduction of the "_safe" functions, introduced in 16d489b0fe05. -- Michael
Вложения
On Tue, Sep 2, 2025 at 12:59 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Sep 02, 2025 at 12:40:25PM +0530, Amul Sul wrote: > > Just a quick question regarding the naming conventions. It looks like > > we have a choice between two options for consistency. Should we rename > > the pg_lsn_in_internal function by replacing "_internal" with "_safe", > > or should we rename all of the *_opt_error functions by replacing > > "_opt_error" with "_internal"? > > > > I would choose the latter option. > > Applying "_safe" seems a bit more consistent to me, as per past > changes like ccff2d20ed96, also looking at the functions that are > given a ErrorSaveContext in input. I am ready to believe that there > are not a lot of callers of the existing _opt_error() routines listed > in numeric.h, so a renaming may be better to let existing callers know > about the change. These predate the introduction of the "_safe" > functions, introduced in 16d489b0fe05. Understood, thanks. The updated version is attached. In addition to the *_opt_error() functions, it also renames pg_lsn_in_internal to pg_lsn_in_safe and incorporates soft error handling. Regards, Amul
Вложения
On Tue, Sep 02, 2025 at 02:41:23PM +0530, Amul Sul wrote:
> The updated version is attached. In addition to the *_opt_error()
> functions, it also renames pg_lsn_in_internal to pg_lsn_in_safe and
> incorporates soft error handling.
Looks globally sensible to me. I was wondering for a bit if the JSON
path parts should be fed pieces of the soft errors that could be
retrieved when the numeric value parsing fails, but that does not seem
worth the extra information.
-pg_lsn_in_internal(const char *str, bool *have_error)
+pg_lsn_in_safe(const char *str, Node *escontext)
[...]
+ ereturn(escontext, InvalidXLogRecPtr,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("invalid input syntax for type %s: \"%s\"",
+ "pg_lsn", str)));
The same error message is repeated twice. How about using some gotos
and one single ereport instead of two? The same can be said for
numeric_div_safe() and numeric_mod_safe(), for the division-by-0
messages.
@@ -5629,11 +5629,11 @@ timestamp_part_common(PG_FUNCTION_ARGS, bool retnumeric)
[...]
+ PG_RETURN_NUMERIC(numeric_add_safe(int64_to_numeric(date2j(tm->tm_year, tm->tm_mon, tm->tm_mday)),
This part with DTK_JULIAN is hard to parse. Not your responsibility
here as you just replace a function name, just a remark in passing.
--
Michael
Вложения
On Wed, 3 Sept 2025 at 07:47, Michael Paquier <michael@paquier.xyz> wrote:
>
> The same error message is repeated twice. How about using some gotos
> and one single ereport instead of two? The same can be said for
> numeric_div_safe() and numeric_mod_safe(), for the division-by-0
> messages.
In numeric_div_safe() and numeric_mod_safe():
- * If "have_error" is provided, check for division by zero here
+ * If "escontext" is provided, raise division by zero soft error here
*/
- if (have_error && (arg2.ndigits == 0 || arg2.digits[0] == 0))
- {
- *have_error = true;
- return NULL;
- }
+ if (escontext && (arg2.ndigits == 0 || arg2.digits[0] == 0))
+ ereturn(escontext, NULL,
+ errcode(ERRCODE_DIVISION_BY_ZERO),
+ errmsg("division by zero"));
This might as well now be made to check for division-by-zero even if
escontext is NULL.
Regards,
Dean
On Wed, Sep 3, 2025 at 1:04 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Wed, 3 Sept 2025 at 07:47, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > The same error message is repeated twice. How about using some gotos
> > and one single ereport instead of two? The same can be said for
> > numeric_div_safe() and numeric_mod_safe(), for the division-by-0
> > messages.
>
> In numeric_div_safe() and numeric_mod_safe():
>
> - * If "have_error" is provided, check for division by zero here
> + * If "escontext" is provided, raise division by zero soft error here
> */
> - if (have_error && (arg2.ndigits == 0 || arg2.digits[0] == 0))
> - {
> - *have_error = true;
> - return NULL;
> - }
> + if (escontext && (arg2.ndigits == 0 || arg2.digits[0] == 0))
> + ereturn(escontext, NULL,
> + errcode(ERRCODE_DIVISION_BY_ZERO),
> + errmsg("division by zero"));
>
> This might as well now be made to check for division-by-zero even if
> escontext is NULL.
Agreed -- did the same in the attached version including the error
deduplication that Michael suggested.
Regards,
Amul
Вложения
On Wed, Sep 3, 2025 at 7:52 PM Amul Sul <sulamul@gmail.com> wrote: > --- a/src/include/utils/numeric.h +++ b/src/include/utils/numeric.h -extern int32 numeric_int4_opt_error(Numeric num, bool *have_error); ..... +extern int32 numeric_int4_safe(Numeric num, Node *escontext); would any extensions using these functions (such as numeric_int4_opt_error) may encounter upgrade compatibility issues in the future?
On Wed, Sep 03, 2025 at 10:41:14PM +0800, jian he wrote: > would any extensions using these functions (such as > numeric_int4_opt_error) may encounter upgrade compatibility issues in > the future? That would be also the point, so as callers are made aware of these "upgraded" versions. I have found one call outside of core here, so the practive is rare, so doing the switch does not worry me much at the end: https://github.com/siilike/postgresql-vpack pg_lsn_in_internal() was worrying me a little bit more, because it could impact backend-side code that wants to parse LSN strings with the extra error handling, like potentially some HA tools, but looking around I am just seeing references based on forked code of the core repository. There's no way to know about closed source code, of course, but I'm not going to qualify that as something that's worth the cost of a compatibility layer with equivalent macros. + * Internal version of numeric_add() supports "soft" error reporting. These comments still read incorrect to me (no point in quoting the soft part, incorrect grammar). I'd suggest the following wording, applied to all the new functions: "Internal version of xxx_safe with support for soft error reporting." I'd be pretty much OK with this version of the patch, plus a few tweaks. Dean, you have commented on this patch before me and the numeric code is something you have touched more than me lately (the LSN part is tainted with my fingerprints, but it's minimal here). Would you prefer handle this patch yourself? I'm OK to perform a final lookup of it for commit, just won't get in your way if you have been looking at it seriously. -- Michael
Вложения
On Thu, 4 Sept 2025 at 01:18, Michael Paquier <michael@paquier.xyz> wrote:
>
> I'd be pretty much OK with this version of the patch, plus a few
> tweaks.
Agreed.
In numeric_div_safe():
+div_error:
+ ereturn(escontext, NULL,
+ errcode(ERRCODE_DIVISION_BY_ZERO),
+ errmsg("division by zero"));
I would make that label something more specific like "division_by_zero".
> Dean, you have commented on this patch before me and the numeric code
> is something you have touched more than me lately (the LSN part is
> tainted with my fingerprints, but it's minimal here). Would you
> prefer handle this patch yourself? I'm OK to perform a final lookup
> of it for commit, just won't get in your way if you have been looking
> at it seriously.
I don't mind. I haven't looked at it too closely, but I'm broadly
happy with it. I think any issues are probably now minor cosmetic
things, so if you've been looking in more detail and are happy to
commit it, then go ahead. Otherwise, I can take it if you prefer.
Regards,
Dean
On Thu, Sep 04, 2025 at 10:50:38AM +0100, Dean Rasheed wrote: > I don't mind. I haven't looked at it too closely, but I'm broadly > happy with it. I think any issues are probably now minor cosmetic > things, so if you've been looking in more detail and are happy to > commit it, then go ahead. Otherwise, I can take it if you prefer. Sure. I'll handle it. Thanks. -- Michael
Вложения
On Fri, Sep 05, 2025 at 08:10:00AM +0900, Michael Paquier wrote: > Sure. I'll handle it. Thanks. Applied after a few tweaks, including changes to the comments, the suggestion of "division_by_zero" for the goto labels, and splitting the patch into two parts for pg_lsn and numeric. -- Michael
Вложения
On Fri, Sep 5, 2025 at 10:28 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, Sep 05, 2025 at 08:10:00AM +0900, Michael Paquier wrote: > > Sure. I'll handle it. Thanks. > > Applied after a few tweaks, including changes to the comments, the > suggestion of "division_by_zero" for the goto labels, and splitting > the patch into two parts for pg_lsn and numeric. Thanks for improving and committing the patch. I see now that two separate commits were definitely needed, and I should have submitted them that way. Thank you again for splitting it into two parts. Regards, Amul