Re: remaining sql/json patches

Thanks for the review.

On Thu, Sep 7, 2023 at 12:01 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> 0001 is quite mysterious to me.  I've been reading it but I'm not sure I
> grok it, so I don't have anything too intelligent to say about it at
> this point.  But here are my thoughts anyway.
>
> Assert()ing that a pointer is not null, and in the next line
> dereferencing that pointer, is useless: the process would crash anyway
> at the time of dereference, so the Assert() adds no value.  Better to
> leave the assert out.  (This appears both in ExecExprEnableErrorSafe and
> ExecExprDisableErrorSafe).
>
> Is it not a problem to set just the node type, and not reset the
> contents of the node to zeroes, in ExecExprEnableErrorSafe?  I'm not
> sure if it's possible to enable error-safe on a node two times with an
> error reported in between; would that result in the escontext filled
> with junk the second time around?  That might be dangerous.  Maybe a
> simple cross-check is to verify (assert) in ExecExprEnableErrorSafe()
> that the struct is already all-zeroes, so that if this happens, we'll
> get reports about it.  (After all, there are very few nodes that handle
> the SOFT_ERROR_OCCURRED case).
>
> Do we need to have the ->details_wanted flag turned on?  Maybe if we're
> having ExecExprEnableErrorSafe() as a generic tool, it should receive
> the boolean to use as an argument.
>
> Why palloc the escontext always, and not just when
> ExecExprEnableErrorSafe is called?  (At Disable time, just memset it to
> zero, and next time it is enabled for that node, we don't need to
> allocate it again, just set the nodetype.)
>
> ExecExprEnableErrorSafe() is a strange name for this operation.  Maybe
> you mean ExecExprEnableSoftErrors()?  Maybe it'd be better to leave it
> as NULL initially, so that for the majority of cases we don't even
> allocate it.

I should have clarified earlier why the ErrorSaveContext must be
allocated statically during the expression compilation phase. This is
necessary because llvm_compile_expr() requires a valid pointer to the
ErrorSaveContext to integrate into the compiled version. Thus, runtime
allocation isn't feasible.

After some consideration, I believe we shouldn't introduce the generic
ExecExprEnable/Disable* interface. Instead, we should let individual
expressions manage the ErrorSaveContext that they want to use
directly, using ExprState.escontext just as a temporary global
variable, much like ExprState.innermost_caseval is used.

The revised 0001 now only contains the changes necessary to make
CoerceViaIO evaluation code support soft error handling.

> In 0002 you're adding soft-error support for a bunch of existing
> operations, in addition to introducing SQL/JSON query functions.  Maybe
> the soft-error stuff should be done separately in a preparatory patch.

Hmm, there'd be only 1 ExecExprEnableErrorSafe() in 0002 -- that in
ExecEvalJsonExprCoercion().  I'm not sure which others you're
referring to.

Given what I said above, the code to reset the ErrorSaveContext
present in 0002 now looks different.  It now resets the error_occurred
flag directly instead of using memset-0-ing the whole struct.
details_wanted and error_data are both supposed to be NULL in this
case anyway and remain set to NULL throughout the lifetime of the
ExprState.

> I think functions such as populate_array_element() that can now save
> soft errors and which currently do not have a return value, should
> acquire a convention to let caller know that things failed: maybe return
> false if SOFT_ERROR_OCCURRED().  Otherwise it appears that, for instance
> populate_array_dim_jsonb() can return happily if an error occurs when
> parsing the last element in the array.  Splitting 0002 to have a
> preparatory patch where all such soft-error-saving changes are
> introduced separately would help review that this is indeed being
> handled by all their callers.

I've separated the changes to jsonfuncs.c into an independent patch.
Upon reviewing the code accessible from populate_record_field() --
which serves as the entry point for the executor via
json_populate_type() -- I identified a few more instances where errors
could be thrown even with a non-NULL escontext. I've included tests
for these in patch 0003. While some error reports, like those in
construct_md_array() (invoked by populate_array()), fall outside
jsonfuncs.c, I assume they're deliberately excluded from SQL/JSON's ON
ERROR support. I've opted not to modify any external interfaces.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com