Re: Error-safe user functions

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