Re: remaining sql/json patches

On Thu, Dec 14, 2023 at 5:04 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Sat, Dec 9, 2023 at 2:30 AM Andres Freund <andres@anarazel.de> wrote:
> > On 2023-12-07 21:07:59 +0900, Amit Langote wrote:
> > > From 38b53297b2d435d5cebf78c1f81e4748fed6c8b6 Mon Sep 17 00:00:00 2001
> > > From: Amit Langote <amitlan@postgresql.org>
> > > Date: Wed, 22 Nov 2023 13:18:49 +0900
> > > Subject: [PATCH v30 2/5] Add soft error handling to populate_record_field()
> > >
> > > An uncoming patch would like the ability to call it from the
> > > executor for some SQL/JSON expression nodes and ask to suppress any
> > > errors that may occur.
> > >
> > > This commit does two things mainly:
> > >
> > > * It modifies the various interfaces internal to jsonfuncs.c to pass
> > >   the ErrorSaveContext around.
> > >
> > > * Make necessary modifications to handle the cases where the
> > >   processing is aborted partway through various functions that take
> > >   an ErrorSaveContext when a soft error occurs.
> > >
> > > Note that the above changes are only intended to suppress errors in
> > > the functions in jsonfuncs.c, but not those in any external functions
> > > that the functions in jsonfuncs.c in turn call, such as those from
> > > arrayfuncs.c.  It is assumed that the various populate_* functions
> > > validate the data before passing those to external functions.
> > >
> > > Discussion: https://postgr.es/m/CA+HiwqE4XTdfb1nW=Ojoy_tQSRhYt-q_kb6i5d4xcKyrLC1Nbg@mail.gmail.com
> >
> > The code here is getting substantially more verbose / less readable.  I wonder
> > if there's something more general that could be improved to make this less
> > painful?
>
> Hmm, I can't think of anything short of a rewrite of the code under
> populate_record_field() so that any error-producing code is well
> isolated or adding a variant/wrapper with soft-error handling
> capabilities.  I'll give this some more thought, though I'm happy to
> hear ideas.

I looked at this and wasn't able to come up with alternative takes
that are better in terms of the verbosity/readability.  I'd still want
to hear if someone well-versed in the json(b) code has any advice.

I also looked at some commits touching src/backend/utils/adt/json*
files to add soft error handling and I can't help but notice that
those commits look not very different from this.  For example, commits
c60c9bad, 50428a30 contain changes like:

@@ -454,7 +474,11 @@ parse_array_element(JsonLexContext *lex,
JsonSemAction *sem)
        return result;

    if (aend != NULL)
-       (*aend) (sem->semstate, isnull);
+   {
+       result = (*aend) (sem->semstate, isnull);
+       if (result != JSON_SUCCESS)
+           return result;
+   }

Attached updated patches addressing jian he's comments, some minor
fixes, and commit message updates.



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