From: | Amit Langote <amitlangote09(at)gmail(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
Cc: | Erik Rijkers <er(at)xs4all(dot)nl>, jian he <jian(dot)universality(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: remaining sql/json patches |
Date: | 2023-09-14 08:14:51 |
Message-ID: | CA+HiwqFzxD0nGBTQgGA+__bzcWwtwT8=NpVn8ZbJ9rq14_77nA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks for the review.
On Thu, Sep 7, 2023 at 12:01 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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
Attachment | Content-Type | Size |
---|---|---|
v14-0005-Claim-SQL-standard-compliance-for-SQL-JSON-featu.patch | application/octet-stream | 2.3 KB |
v14-0001-Add-soft-error-handling-to-CoerceViaIO-evaluatio.patch | application/octet-stream | 14.0 KB |
v14-0002-Add-soft-error-handling-to-populate_record_field.patch | application/octet-stream | 20.4 KB |
v14-0003-SQL-JSON-query-functions.patch | application/octet-stream | 203.6 KB |
v14-0004-JSON_TABLE.patch | application/octet-stream | 170.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2023-09-14 08:20:01 | Re: information_schema and not-null constraints |
Previous Message | jian he | 2023-09-14 08:14:01 | Re: Cleaning up array_in() |