Re: remaining sql/json patches

On Thu, Sep 21, 2023 at 5:58 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> I keep looking at 0001, and in the struct definition I think putting the
> escontext at the bottom is not great, because there's a comment a few
> lines above that says "XXX: following fields only needed during
> "compilation"), could be thrown away afterwards". This comment is not
> strictly true, because innermost_caseval is actually used by
> array_map(); yet it seems that ->escontext should appear before that
> comment.

Hmm. Actually, we can make it so that *escontext* is only needed
during ExecInitExprRec() and never after that. I've done that in the
attached updated patch, where you can see that ExprState.escontext is
only ever touched in execExpr.c. Also, I noticed that I had
forgotten to extract one more expression node type's conversion to use
soft errors from the main patch (0003). That is CoerceToDomain, which
I've now moved into 0001.

> Also, ->escontext's own comment in ExprState seems to be saying too much
> and not saying enough. I would reword it as "For expression nodes that
> support soft errors. NULL if caller wants them thrown instead". The
> shortest I could make so that it fits in a single is "For nodes that can
> error softly. NULL if caller wants them thrown", or "For
> soft-error-enabled nodes. NULL if caller wants errors thrown". Not
> sure if those are good enough, or just make the comment the whole four
> lines ...

How about:

+ /*
+ * For expression nodes that support soft errors. Set to NULL before
+ * calling ExecInitExprRec() if the caller wants errors thrown.
+ */

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