Feedback-ID: ia2694551:Fastmail
Date: Thu, 21 Sep 2023 10:57:54 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Andrew Dunstan <andrew@dunslane.net>,
	jian he <jian.universality@gmail.com>, Erik Rijkers <er@xs4all.nl>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: remaining sql/json patches
Message-ID: <202309210857.3i4xp5oojokl@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+HiwqE38o73m78Xvf8Lzt2X5O9vfhj2fNxNg+tCotoQ_xwz3A@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202309210857.3i4xp5oojokl%40alvherre.pgsql>
Precedence: bulk

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.

However, if you put it before steps_len, it would push members steps_len
and steps_alloc beyond the struct's first cache line(*).  If those
struct members are critical for expression init performance, then maybe
it's not a good tradeoff.  I don't know if this was struct laid out
carefully with that consideration in mind or not.

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 ...


(*) This is what pahole says about the struct as 0001 would put it:

struct ExprState {
    NodeTag                    type;                 /*     0     4 */
    uint8                      flags;                /*     4     1 */
    _Bool                      resnull;              /*     5     1 */

    /* XXX 2 bytes hole, try to pack */

    Datum                      resvalue;             /*     8     8 */
    TupleTableSlot *           resultslot;           /*    16     8 */
    struct ExprEvalStep *      steps;                /*    24     8 */
    ExprStateEvalFunc          evalfunc;             /*    32     8 */
    Expr *                     expr;                 /*    40     8 */
    void *                     evalfunc_private;     /*    48     8 */
    int                        steps_len;            /*    56     4 */
    int                        steps_alloc;          /*    60     4 */
    /* --- cacheline 1 boundary (64 bytes) --- */
    struct PlanState *         parent;               /*    64     8 */
    ParamListInfo              ext_params;           /*    72     8 */
    Datum *                    innermost_caseval;    /*    80     8 */
    _Bool *                    innermost_casenull;   /*    88     8 */
    Datum *                    innermost_domainval;  /*    96     8 */
    _Bool *                    innermost_domainnull; /*   104     8 */
    ErrorSaveContext *         escontext;            /*   112     8 */

    /* size: 120, cachelines: 2, members: 18 */
    /* sum members: 118, holes: 1, sum holes: 2 */
    /* last cacheline: 56 bytes */
};

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)