Re: Error-safe user functions - Mailing list pgsql-hackers

Hi,

On 2022-12-07 17:32:21 -0500, Tom Lane wrote:
> I already pushed the 0000 elog-refactoring patch, since that seemed
> uncontroversial.  0001 attached covers the same territory as before,
> but I regrouped the rest so that 0002 installs the new test support
> functions, then 0003 adds both the per-datatype changes and
> corresponding test cases for bool, int4, arrays, and records.
> The idea here is that 0003 can be pointed to as a sample of what
> has to be done to datatype input functions, while the preceding
> patches can be cited as relevant documentation.  (I've not decided
> whether to squash 0001 and 0002 together or commit them separately.

I think they make sense as is.


> Does it make sense to break 0003 into 4 separate commits, or is
> that overkill?)

I think it'd be fine either way.


> + * If "context" is an ErrorSaveContext node, but the node creator only wants
> + * notification of the fact of a soft error without any details, just set
> + * the error_occurred flag in the ErrorSaveContext node and return false,
> + * which will cause us to skip the remaining error processing steps.
> + *
> + * Otherwise, create and initialize error stack entry and return true.
> + * Subsequently, errmsg() and perhaps other routines will be called to further
> + * populate the stack entry.  Finally, errsave_finish() will be called to
> + * tidy up.
> + */
> +bool
> +errsave_start(NodePtr context, const char *domain)

I wonder if there are potential use-cases for levels other than ERROR. I can
potentially see us wanting to defer some FATALs, e.g. when they occur in
process exit hooks.


> +{
> +    ErrorSaveContext *escontext;
> +    ErrorData  *edata;
> +
> +    /*
> +     * Do we have a context for soft error reporting?  If not, just punt to
> +     * errstart().
> +     */
> +    if (context == NULL || !IsA(context, ErrorSaveContext))
> +        return errstart(ERROR, domain);
> +
> +    /* Report that a soft error was detected */
> +    escontext = (ErrorSaveContext *) context;
> +    escontext->error_occurred = true;
> +
> +    /* Nothing else to do if caller wants no further details */
> +    if (!escontext->details_wanted)
> +        return false;
> +
> +    /*
> +     * Okay, crank up a stack entry to store the info in.
> +     */
> +
> +    recursion_depth++;
> +
> +    /* Initialize data for this error frame */
> +    edata = get_error_stack_entry();

For a moment I was worried that it could lead to odd behaviour that we don't
do get_error_stack_entry() when !details_wanted, due to not raising an error
we'd otherwise raise. But that's a should-never-be-reached case, so ...


> +/*
> + * errsave_finish --- end a "soft" error-reporting cycle
> + *
> + * If errsave_start() decided this was a regular error, behave as
> + * errfinish().  Otherwise, package up the error details and save
> + * them in the ErrorSaveContext node.
> + */
> +void
> +errsave_finish(NodePtr context, const char *filename, int lineno,
> +               const char *funcname)
> +{
> +    ErrorSaveContext *escontext = (ErrorSaveContext *) context;
> +    ErrorData  *edata = &errordata[errordata_stack_depth];
> +
> +    /* verify stack depth before accessing *edata */
> +    CHECK_STACK_DEPTH();
> +
> +    /*
> +     * If errsave_start punted to errstart, then elevel will be ERROR or
> +     * perhaps even PANIC.  Punt likewise to errfinish.
> +     */
> +    if (edata->elevel >= ERROR)
> +    {
> +        errfinish(filename, lineno, funcname);
> +        pg_unreachable();
> +    }

It seems somewhat ugly transport this knowledge via edata->elevel, but it's
not too bad.



> +/*
> + * We cannot include nodes.h yet, so make a stub reference.  (This is also
> + * used by fmgr.h, which doesn't want to depend on nodes.h either.)
> + */
> +typedef struct Node *NodePtr;

Seems like it'd be easier to just forward declare the struct, and use the
non-typedef'ed name in the header than to have to deal with these
interdependencies and the differing typenames.


> +/*----------
> + * Support for reporting "soft" errors that don't require a full transaction
> + * abort to clean up.  This is to be used in this way:
> + *        errsave(context,
> + *                errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
> + *                errmsg("invalid input syntax for type %s: \"%s\"",
> + *                       "boolean", in_str),
> + *                ... other errxxx() fields as needed ...);
> + *
> + * "context" is a node pointer or NULL, and the remaining auxiliary calls
> + * provide the same error details as in ereport().  If context is not a
> + * pointer to an ErrorSaveContext node, then errsave(context, ...)
> + * behaves identically to ereport(ERROR, ...).  If context is a pointer
> + * to an ErrorSaveContext node, then the information provided by the
> + * auxiliary calls is stored in the context node and control returns
> + * normally.  The caller of errsave() must then do any required cleanup
> + * and return control back to its caller.  That caller must check the
> + * ErrorSaveContext node to see whether an error occurred before
> + * it can trust the function's result to be meaningful.
> + *
> + * errsave_domain() allows a message domain to be specified; it is
> + * precisely analogous to ereport_domain().
> + *----------
> + */
> +#define errsave_domain(context, domain, ...)    \
> +    do { \
> +        NodePtr context_ = (context); \
> +        pg_prevent_errno_in_scope(); \
> +        if (errsave_start(context_, domain)) \
> +            __VA_ARGS__, errsave_finish(context_, __FILE__, __LINE__, __func__); \
> +    } while(0)

Perhaps worth noting here that the reason why the errsave_start/errsave_finish
split exist differs a bit from the reason in ereport_domain()? "Over there"
it's just about not wanting to incur overhead when the message isn't logged,
but here we'll always have >= ERROR, but ->details_wanted can still lead to
not wanting to incur the overhead.


>  /*
> diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
> index db843a0fbf..bdafcff02d 100644
> --- a/src/backend/utils/adt/rowtypes.c
> +++ b/src/backend/utils/adt/rowtypes.c
> @@ -77,6 +77,7 @@ record_in(PG_FUNCTION_ARGS)
>      char       *string = PG_GETARG_CSTRING(0);
>      Oid            tupType = PG_GETARG_OID(1);
>      int32        tupTypmod = PG_GETARG_INT32(2);
> +    Node       *escontext = fcinfo->context;
>      HeapTupleHeader result;
>      TupleDesc    tupdesc;
>      HeapTuple    tuple;
> @@ -100,7 +101,7 @@ record_in(PG_FUNCTION_ARGS)
>       * supply a valid typmod, and then we can do something useful for RECORD.
>       */
>      if (tupType == RECORDOID && tupTypmod < 0)
> -        ereport(ERROR,
> +        ereturn(escontext, (Datum) 0,
>                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                   errmsg("input of anonymous composite types is not implemented")));
>  

Is it ok that we throw an error in lookup_rowtype_tupdesc()? Normally those
should not be reachable by users, I think? The new testing functions might
reach it, but that seems fine, they're test functions.


Greetings,

Andres Freund