Re: remaining sql/json patches

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: Raw Message | Whole Thread | 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)

In response to

Responses

Browse pgsql-hackers by date

  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