| From: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> | 
|---|---|
| To: | Amit Langote <amitlangote09(at)gmail(dot)com> | 
| Cc: | Andrew Dunstan <andrew(at)dunslane(dot)net>, jian he <jian(dot)universality(at)gmail(dot)com>, Erik Rijkers <er(at)xs4all(dot)nl>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> | 
| Subject: | Re: remaining sql/json patches | 
| Date: | 2023-09-21 08:57:54 | 
| Message-ID: | 202309210857.3i4xp5oojokl@alvherre.pgsql | 
| Views: | Whole Thread | Raw Message | Download mbox | Resend email | 
| Thread: | |
| Lists: | pgsql-hackers | 
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)
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Amit Kapila | 2023-09-21 09:01:24 | Re: pg_upgrade and logical replication | 
| Previous Message | Amit Langote | 2023-09-21 08:32:01 | Re: remaining sql/json patches |