Re: remaining sql/json patches - Mailing list pgsql-hackers

Thanks for the review.

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:
> > --- a/src/include/executor/execExpr.h
> > +++ b/src/include/executor/execExpr.h
> > @@ -547,6 +549,7 @@ typedef struct ExprEvalStep
> >                       bool       *checknull;
> >                       /* OID of domain type */
> >                       Oid                     resulttype;
> > +                     ErrorSaveContext *escontext;
> >               }                       domaincheck;
> >
> > diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
> > index 5d7f17dee0..6a7118d300 100644
> > --- a/src/include/nodes/execnodes.h
> > +++ b/src/include/nodes/execnodes.h
> > @@ -34,6 +34,7 @@
> >  #include "fmgr.h"
> >  #include "lib/ilist.h"
> >  #include "lib/pairingheap.h"
> > +#include "nodes/miscnodes.h"
> >  #include "nodes/params.h"
> >  #include "nodes/plannodes.h"
> >  #include "nodes/tidbitmap.h"
> > @@ -129,6 +130,12 @@ typedef struct ExprState
> >
> >       Datum      *innermost_domainval;
> >       bool       *innermost_domainnull;
> > +
> > +     /*
> > +      * For expression nodes that support soft errors.  Should be set to NULL
> > +      * before calling ExecInitExprRec() if the caller wants errors thrown.
> > +      */
> > +     ErrorSaveContext *escontext;
> >  } ExprState;
>
> Why do we need this both in ExprState *and* in ExprEvalStep?

In the current design, ExprState.escontext is only set when
initializing sub-expressions that should have their errors handled
softly and is supposed to be NULL at the runtime.   So, the design
expects the expressions to save the ErrorSaveContext pointer into
their struct in ExecEvalStep or somewhere else (input function's
FunctionCallInfo in CoerceViaIO's case).

> > 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'd not at all be surprised if this caused a measurable slowdown.   Patches 0004, 0005, and 0006 are new.

I don't notice a significant slowdown.  The benchmark I used is the
time to run the following query:

select json_populate_record(row(1,1), '{"f1":1, "f2":1}') from
generate_series(1, 1000000)

Here are the times:

Unpatched:
Time: 1262.011 ms (00:01.262)
Time: 1202.354 ms (00:01.202)
Time: 1187.708 ms (00:01.188)
Time: 1171.752 ms (00:01.172)
Time: 1174.249 ms (00:01.174)

Patched:
Time: 1233.927 ms (00:01.234)
Time: 1185.381 ms (00:01.185)
Time: 1202.245 ms (00:01.202)
Time: 1164.994 ms (00:01.165)
Time: 1179.009 ms (00:01.179)

perf shows that a significant amount of time is spent is json_lex()
dwarfing the time spent in dispatching code that is being changed
here.

> > ---
> >  src/backend/utils/adt/jsonfuncs.c | 310 +++++++++++++++++++++++-------
> >  1 file changed, 236 insertions(+), 74 deletions(-)
>
> >  /* functions supporting jsonb_delete, jsonb_set and jsonb_concat */
> >  static JsonbValue *IteratorConcat(JsonbIterator **it1, JsonbIterator **it2,
> > @@ -2484,12 +2491,12 @@ populate_array_report_expected_array(PopulateArrayContext *ctx, int ndim)
> >       if (ndim <= 0)
> >       {
> >               if (ctx->colname)
> > -                     ereport(ERROR,
> > +                     errsave(ctx->escontext,
> >                                       (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
> >                                        errmsg("expected JSON array"),
> >                                        errhint("See the value of key \"%s\".", ctx->colname)));
> >               else
> > -                     ereport(ERROR,
> > +                     errsave(ctx->escontext,
> >                                       (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
> >                                        errmsg("expected JSON array")));
> >       }
> > @@ -2506,13 +2513,13 @@ populate_array_report_expected_array(PopulateArrayContext *ctx, int ndim)
> >                       appendStringInfo(&indices, "[%d]", ctx->sizes[i]);
> >
> >               if (ctx->colname)
> > -                     ereport(ERROR,
> > +                     errsave(ctx->escontext,
> >                                       (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
> >                                        errmsg("expected JSON array"),
> >                                        errhint("See the array element %s of key \"%s\".",
> >                                                        indices.data, ctx->colname)));
> >               else
> > -                     ereport(ERROR,
> > +                     errsave(ctx->escontext,
> >                                       (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
> >                                        errmsg("expected JSON array"),
> >                                        errhint("See the array element %s.",
> > @@ -2520,8 +2527,13 @@ populate_array_report_expected_array(PopulateArrayContext *ctx, int ndim)
> >       }
> >  }
>
> It seems mildly errorprone to use errsave() but not have any returns in the
> code after the errsave()s - it seems plausible that somebody later would come
> and add more code expecting to not reach the later code.

Having returns in the code blocks containing errsave() sounds prudent, so done.

> > +/*
> > + * Validate and set ndims for populating an array with some
> > + * populate_array_*() function.
> > + *
> > + * Returns false if the input (ndims) is erratic.
>
> I don't think "erratic" is the right word, "erroneous" maybe?

"erroneous" sounds better.

> > ---
> >  doc/src/sgml/func.sgml                      |  151 +++
> >  src/backend/catalog/sql_features.txt        |   12 +-
> >  src/backend/executor/execExpr.c             |  363 +++++++
> >  src/backend/executor/execExprInterp.c       |  365 ++++++-
> >  src/backend/jit/llvm/llvmjit.c              |    2 +
> >  src/backend/jit/llvm/llvmjit_expr.c         |  140 +++
> >  src/backend/jit/llvm/llvmjit_types.c        |    4 +
> >  src/backend/nodes/makefuncs.c               |   18 +
> >  src/backend/nodes/nodeFuncs.c               |  238 ++++-
> >  src/backend/optimizer/path/costsize.c       |    3 +-
> >  src/backend/optimizer/util/clauses.c        |   19 +
> >  src/backend/parser/gram.y                   |  178 +++-
> >  src/backend/parser/parse_expr.c             |  621 ++++++++++-
> >  src/backend/parser/parse_target.c           |   15 +
> >  src/backend/utils/adt/formatting.c          |   44 +
> >  src/backend/utils/adt/jsonb.c               |   31 +
> >  src/backend/utils/adt/jsonfuncs.c           |   52 +-
> >  src/backend/utils/adt/jsonpath.c            |  255 +++++
> >  src/backend/utils/adt/jsonpath_exec.c       |  391 ++++++-
> >  src/backend/utils/adt/ruleutils.c           |  136 +++
> >  src/include/executor/execExpr.h             |  133 +++
> >  src/include/fmgr.h                          |    1 +
> >  src/include/jit/llvmjit.h                   |    1 +
> >  src/include/nodes/makefuncs.h               |    2 +
> >  src/include/nodes/parsenodes.h              |   47 +
> >  src/include/nodes/primnodes.h               |  130 +++
> >  src/include/parser/kwlist.h                 |   11 +
> >  src/include/utils/formatting.h              |    1 +
> >  src/include/utils/jsonb.h                   |    1 +
> >  src/include/utils/jsonfuncs.h               |    5 +
> >  src/include/utils/jsonpath.h                |   27 +
> >  src/interfaces/ecpg/preproc/ecpg.trailer    |   28 +
> >  src/test/regress/expected/json_sqljson.out  |   18 +
> >  src/test/regress/expected/jsonb_sqljson.out | 1032 +++++++++++++++++++
> >  src/test/regress/parallel_schedule          |    2 +-
> >  src/test/regress/sql/json_sqljson.sql       |   11 +
> >  src/test/regress/sql/jsonb_sqljson.sql      |  337 ++++++
> >  src/tools/pgindent/typedefs.list            |   18 +
> >  38 files changed, 4767 insertions(+), 76 deletions(-)
>
> I think it'd be worth trying to break this into smaller bits - it's not easy
> to review this at once.

ISTM that the only piece that can be broken out at this point is the
additions under src/backend/utils/adt.  I'm not entirely sure if it'd
be a good idea to commit the various bits on their own, that is,
without tests which cannot be added without the rest of the
parser/executor additions for JsonFuncExpr, JsonExpr, and the
supporting child nodes.

I've extracted those bits as separate patches even if only for the
ease of review.

> > +/*
> > + * Information about the state of JsonPath* evaluation.
> > + */
> > +typedef struct JsonExprPostEvalState
> > +{
> > +     /* Did JsonPath* evaluation cause an error? */
> > +     NullableDatum   error;
> > +
> > +     /* Is the result of JsonPath* evaluation empty? */
> > +     NullableDatum   empty;
> > +
> > +     /*
> > +      * ExecEvalJsonExprPath() will set this to the address of the step to
> > +      * use to coerce the result of JsonPath* evaluation to the RETURNING type.
> > +      * Also see the description of possible step addresses that this could be
> > +      * set to in the definition of JsonExprState.
> > +      */
> > +#define FIELDNO_JSONEXPRPOSTEVALSTATE_JUMP_EVAL_COERCION     2
> > +     int                     jump_eval_coercion;
> > +} JsonExprPostEvalState;
> > +
> > +/* State for evaluating a JsonExpr, too big to inline */
> > +typedef struct JsonExprState
> > +{
> > +     /* original expression node */
> > +     JsonExpr   *jsexpr;
> > +
> > +     /* value/isnull for formatted_expr */
> > +     NullableDatum formatted_expr;
> > +
> > +     /* value/isnull for pathspec */
> > +     NullableDatum pathspec;
> > +
> > +     /* JsonPathVariable entries for passing_values */
> > +     List       *args;
> > +
> > +     /*
> > +      * Per-row result status info populated by ExecEvalJsonExprPath()
> > +      * and ExecEvalJsonCoercionFinish().
> > +      */
> > +     JsonExprPostEvalState post_eval;
> > +
> > +     /*
> > +      * Address of the step that implements the non-ERROR variant of ON ERROR
> > +      * and ON EMPTY behaviors, to be jumped to when ExecEvalJsonExprPath()
> > +      * returns false on encountering an error during JsonPath* evaluation
> > +      * (ON ERROR) or on finding that no matching JSON item was returned (ON
> > +      * EMPTY).  The same steps are also performed on encountering an error
> > +      * when coercing JsonPath* result to the RETURNING type.
> > +      */
> > +     int                     jump_error;
> > +
> > +     /*
> > +      * Addresses of steps to perform the coercion of the JsonPath* result value
> > +      * to the RETURNING type.  Each address points to either 1) a special
> > +      * EEOP_JSONEXPR_COERCION step that handles coercion using the RETURNING
> > +      * type's input function or by using json_via_populate(), or 2) an
> > +      * expression such as CoerceViaIO.  It may be -1 if no coercion is
> > +      * necessary.
> > +      *
> > +      * jump_eval_result_coercion points to the step to evaluate the coercion
> > +      * given in JsonExpr.result_coercion.
> > +      */
> > +     int                     jump_eval_result_coercion;
> > +
> > +     /* eval_item_coercion_jumps is an array of num_item_coercions elements
> > +      * each containing a step address to evaluate the coercion from a value of
> > +      * the given JsonItemType to the RETURNING type, or -1 if no coercion is
> > +      * necessary.  item_coercion_via_expr is an array of boolean flags of the
> > +      * same length that indicates whether each valid step address in the
> > +      * eval_item_coercion_jumps array points to an expression or a
> > +      * EEOP_JSONEXPR_COERCION step.  ExecEvalJsonExprPath() will cause an
> > +      * error if it's the latter, because that mode of coercion is not
> > +      * supported for all JsonItemTypes.
> > +      */
> > +     int                     num_item_coercions;
> > +     int                *eval_item_coercion_jumps;
> > +     bool       *item_coercion_via_expr;
> > +
> > +     /*
> > +      * For passing when initializing a EEOP_IOCOERCE_SAFE step for any
> > +      * CoerceViaIO nodes in the expression that must be evaluated in an
> > +      * error-safe manner.
> > +      */
> > +     ErrorSaveContext escontext;
> > +} JsonExprState;
> > +
> > +/*
> > + * State for coercing a value to the target type specified in 'coercion' using
> > + * either json_populate_type() or by calling the type's input function.
> > + */
> > +typedef struct JsonCoercionState
> > +{
> > +     /* original expression node */
> > +     JsonCoercion   *coercion;
> > +
> > +     /* Input function info for the target type. */
> > +     struct
> > +     {
> > +             FmgrInfo   *finfo;
> > +             Oid                     typioparam;
> > +     }                       input;
> > +
> > +     /* Cache for json_populate_type() */
> > +     void       *cache;
> > +
> > +     /*
> > +      * For soft-error handling in json_populate_type() or
> > +      * in InputFunctionCallSafe().
> > +      */
> > +     ErrorSaveContext *escontext;
> > +} JsonCoercionState;
>
> Does all of this stuff need to live in this header? Some of it seems like it
> doesn't need to be in a header at all, and other bits seem like they belong
> somewhere more json specific?

I've gotten rid of JsonCoercionState, moving the fields directly into
ExprEvalStep.d.jsonexpr_coercion.

Regarding JsonExprState and JsonExprPostEvalState, maybe they're
better put in execnodes.h to be near other expression state nodes like
WindowFuncExprState, so have moved them there.  I'm not sure of a
json-specific place for this.  All of the information contained in
those structs is populated and used by execInterpExpr.c, so
execnodes.h seems appropriate to me.

> > +/*
> > + * JsonItemType
> > + *           Represents type codes to identify a JsonCoercion node to use when
> > + *           coercing a given SQL/JSON items to the output SQL type
> > + *
> > + * The comment next to each item type mentions the JsonbValue.jbvType of the
> > + * source JsonbValue value to be coerced using the expression in the
> > + * JsonCoercion node.
> > + *
> > + * Also, see InitJsonItemCoercions() and ExecPrepareJsonItemCoercion().
> > + */
> > +typedef enum JsonItemType
> > +{
> > +     JsonItemTypeNull = 0,           /* jbvNull */
> > +     JsonItemTypeString = 1,         /* jbvString */
> > +     JsonItemTypeNumeric = 2,        /* jbvNumeric */
> > +     JsonItemTypeBoolean = 3,        /* jbvBool */
> > +     JsonItemTypeDate = 4,           /* jbvDatetime: DATEOID */
> > +     JsonItemTypeTime = 5,           /* jbvDatetime: TIMEOID */
> > +     JsonItemTypeTimetz = 6,         /* jbvDatetime: TIMETZOID */
> > +     JsonItemTypeTimestamp = 7,      /* jbvDatetime: TIMESTAMPOID */
> > +     JsonItemTypeTimestamptz = 8,    /* jbvDatetime: TIMESTAMPTZOID */
> > +     JsonItemTypeComposite = 9,      /* jbvArray, jbvObject, jbvBinary */
> > +     JsonItemTypeInvalid = 10,
> > +} JsonItemType;
>
> Why do we need manually assigned values here?

Not really necessary here.  I think I simply copied the style from
some other json-related enum where assigning values seems necessary.

> > +/*
> > + * JsonCoercion -
> > + *           coercion from SQL/JSON item types to SQL types
> > + */
> > +typedef struct JsonCoercion
> > +{
> > +     NodeTag         type;
> > +
> > +     Oid                     targettype;
> > +     int32           targettypmod;
> > +     bool            via_populate;   /* coerce result using json_populate_type()? */
> > +     bool            via_io;                 /* coerce result using type input function? */
> > +     Oid                     collation;              /* collation for coercion via I/O or populate */
> > +} JsonCoercion;
> > +
> > +typedef struct JsonItemCoercion
> > +{
> > +     NodeTag         type;
> > +
> > +     JsonItemType item_type;
> > +     Node       *coercion;
> > +} JsonItemCoercion;
>
> What's the difference between an "ItemCoercion" and a "Coercion"?

ItemCoercion is used to store the coercion expression used at runtime
to convert the value of given JsonItemType to the target type
specified in the JsonExpr.returning.  It can either be a cast
expression node found by the parser or a JsonCoercion node.

I'll update the comments.

> > +/*
> > + * JsonBehavior -
> > + *           representation of a given JSON behavior
>
> My editor warns about space-before-tab here.

Fixed.

> > + */
> > +typedef struct JsonBehavior
> > +{
> > +     NodeTag         type;
>
> > +     JsonBehaviorType btype;         /* behavior type */
> > +     Node       *expr;                       /* behavior expression */
>
> These comment don't seem helpful. I think there's need for comments here, but
> restating the field name in different words isn't helpful. What's needed is an
> explanation of how things interact, perhaps also why that's the appropriate
> representation.
>
> > +     JsonCoercion *coercion;         /* to coerce behavior expression when there is
> > +                                                              * no cast to the target type */
> > +     int                     location;               /* token location, or -1 if unknown */
>
> > +} JsonBehavior;
> > +
> > +/*
> > + * JsonExpr -
> > + *           transformed representation of JSON_VALUE(), JSON_QUERY(), JSON_EXISTS()
> > + */
> > +typedef struct JsonExpr
> > +{
> > +     Expr            xpr;
> > +
> > +     JsonExprOp      op;                             /* json function ID */
> > +     Node       *formatted_expr; /* formatted context item expression */
> > +     Node       *result_coercion; /* resulting coercion to RETURNING type */
> > +     JsonFormat *format;                     /* context item format (JSON/JSONB) */
> > +     Node       *path_spec;          /* JSON path specification expression */
> > +     List       *passing_names;      /* PASSING argument names */
> > +     List       *passing_values; /* PASSING argument values */
> > +     JsonReturning *returning;       /* RETURNING clause type/format info */
> > +     JsonBehavior *on_empty;         /* ON EMPTY behavior */
> > +     JsonBehavior *on_error;         /* ON ERROR behavior */
> > +     List       *item_coercions; /* coercions for JSON_VALUE */
> > +     JsonWrapper wrapper;            /* WRAPPER for JSON_QUERY */
> > +     bool            omit_quotes;    /* KEEP/OMIT QUOTES for JSON_QUERY */
> > +     int                     location;               /* token location, or -1 if unknown */
> > +} JsonExpr;
>
> These comments seem even worse.

OK, I've rewritten the comments about JsonBehavior and JsonExpr.

> > +static void ExecInitJsonExpr(JsonExpr *jexpr, ExprState *state,
> > +                                                      Datum *resv, bool *resnull,
> > +                                                      ExprEvalStep *scratch);
> > +static int ExecInitJsonExprCoercion(ExprState *state, Node *coercion,
> > +                                              ErrorSaveContext *escontext,
> > +                                              Datum *resv, bool *resnull);
> >
> >
> >  /*
> > @@ -2416,6 +2423,36 @@ ExecInitExprRec(Expr *node, ExprState *state,
> >                               break;
> >                       }
> >
> > +             case T_JsonExpr:
> > +                     {
> > +                             JsonExpr   *jexpr = castNode(JsonExpr, node);
> > +
> > +                             ExecInitJsonExpr(jexpr, state, resv, resnull, &scratch);
> > +                             break;
> > +                     }
> > +
> > +             case T_JsonCoercion:
> > +                     {
> > +                             JsonCoercion    *coercion = castNode(JsonCoercion, node);
> > +                             JsonCoercionState *jcstate = palloc0(sizeof(JsonCoercionState));
> > +                             Oid                     typinput;
> > +                             FmgrInfo   *finfo;
> > +
> > +                             getTypeInputInfo(coercion->targettype, &typinput,
> > +                                                              &jcstate->input.typioparam);
> > +                             finfo = palloc0(sizeof(FmgrInfo));
> > +                             fmgr_info(typinput, finfo);
> > +                             jcstate->input.finfo = finfo;
> > +
> > +                             jcstate->coercion = coercion;
> > +                             jcstate->escontext = state->escontext;
> > +
> > +                             scratch.opcode = EEOP_JSONEXPR_COERCION;
> > +                             scratch.d.jsonexpr_coercion.jcstate = jcstate;
> > +                             ExprEvalPushStep(state, &scratch);
> > +                             break;
> > +                     }
>
> It's confusing that we have ExecInitJsonExprCoercion, but aren't using that
> here, but then use it later, in ExecInitJsonExpr().

I had moved this code out of ExecInitJsonExprCoercion() into
ExecInitExprRec() to make the JsonCoercion node look like a first
class citizen of execExpr.c, but maybe that's not such a good idea
after all.  I've moved it back to make it just another implementation
detail of JsonExpr.

> > +             EEO_CASE(EEOP_JSONEXPR_PATH)
> > +             {
> > +                     JsonExprState *jsestate = op->d.jsonexpr.jsestate;
> > +
> > +                     /* too complex for an inline implementation */
> > +                     if (!ExecEvalJsonExprPath(state, op, econtext))
> > +                             EEO_JUMP(jsestate->jump_error);
> > +                     else if (jsestate->post_eval.jump_eval_coercion >= 0)
> > +                             EEO_JUMP(jsestate->post_eval.jump_eval_coercion);
> > +
> > +                     EEO_NEXT();
> > +             }
>
> Why do we need post_eval.jump_eval_coercion? Seems like that could more
> cleanly be implemented by just emitting a constant JUMP step?  Oh, I see -
> you're changing post_eval.jump_eval_coercion at runtime.  This seems like a
> BAD idea.  I strongly suggest that instead of modifying the field, you instead
> return the target jump step as a return value from ExecEvalJsonExprPath or
> such.

OK, done that way.

> > +     bool            throw_error = (jexpr->on_error->btype == JSON_BEHAVIOR_ERROR);
>
> What's the deal with the parentheses here and in similar places below? There's
> no danger of ambiguity without, no?

Yes, this looks like a remnant of an old version of this condition.

> > +     if (empty)
> > +     {
> > +             if (jexpr->on_empty)
> > +             {
> > +                     if (jexpr->on_empty->btype == JSON_BEHAVIOR_ERROR)
> > +                             ereport(ERROR,
> > +                                             (errcode(ERRCODE_NO_SQL_JSON_ITEM),
> > +                                              errmsg("no SQL/JSON item")));
>
> No need for the parens around ereport() arguments anymore. Same in a few other places.

All fixed.

> > diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> > index d631ac89a9..4f92d000ec 100644
> > --- a/src/backend/parser/gram.y
> > +++ b/src/backend/parser/gram.y
> > +json_behavior:
> > +                     DEFAULT a_expr
> > +                             { $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_DEFAULT, $2, NULL, @1); }
> > +                     | ERROR_P
> > +                             { $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_ERROR, NULL, NULL, @1); }
> > +                     | NULL_P
> > +                             { $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_NULL, NULL, NULL, @1); }
> > +                     | TRUE_P
> > +                             { $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_TRUE, NULL, NULL, @1); }
> > +                     | FALSE_P
> > +                             { $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_FALSE, NULL, NULL, @1); }
> > +                     | UNKNOWN
> > +                             { $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_UNKNOWN, NULL, NULL, @1); }
> > +                     | EMPTY_P ARRAY
> > +                             { $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL, NULL, @1); }
> > +                     | EMPTY_P OBJECT_P
> > +                             { $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_EMPTY_OBJECT, NULL, NULL, @1); }
> > +                     /* non-standard, for Oracle compatibility only */
> > +                     | EMPTY_P
> > +                             { $$ = (Node *) makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL, NULL, @1); }
> > +             ;
>
> Seems like this would look better if you made json_behavior return just the
> enum values and had one makeJsonBehavior() at the place referencing it?

Yes, changed like that.

> > +json_behavior_clause_opt:
> > +                     json_behavior ON EMPTY_P
> > +                             { $$ = list_make2($1, NULL); }
> > +                     | json_behavior ON ERROR_P
> > +                             { $$ = list_make2(NULL, $1); }
> > +                     | json_behavior ON EMPTY_P json_behavior ON ERROR_P
> > +                             { $$ = list_make2($1, $4); }
> > +                     | /* EMPTY */
> > +                             { $$ = list_make2(NULL, NULL); }
> > +             ;
>
> This seems like an odd representation - why represent the behavior as a two
> element list where one needs to know what is stored at which list offset?

A previous version had a JsonBehaviorClause containing 2 JsonBehavior
nodes, but Peter didn't like it, so we have this.  Like Peter, I
prefer to use the List instead of a whole new parser node, but maybe
the damage would be less if we make the List be local to gram.y.  I've
done that by adding two JsonBehavior nodes to JsonFuncExpr itself
which are assigned appropriate values from the List in gram.y itself.

Updated patches attached.

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