Hi, On 2022-06-17 16:53:31 +1200, David Rowley wrote: > On Fri, 17 Jun 2022 at 15:33, Peter Geoghegan <p...@bowt.ie> wrote: > > Have you tried this with the insert benchmark [1]? > > I was mostly focusing on the performance of the hashed saop feature > after having removed the additional fields that pushed ExprEvalStep > over 64 bytes in 14. > > I agree it would be good to do further benchmarking to see if there's > anything else that's snuck into 15 that's slowed that benchmark down, > but we can likely work on that after we get the ExprEvalStep size back > to 64 bytes again.
I did reproduce a regression between 14 and 15, using both pgbench -Mprepared -S (scale 1) and TPC-H Q01 (scale 5). Between 7-10% - not good, particularly that that's not been found so far. Fixing the json size issue gets that down to ~2%. Not sure what that's caused by yet. Greetings, Andres Freund
>From 8c1ec2f6dce304ae600177a4b5bdb936e8fb65b0 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Thu, 16 Jun 2022 18:28:39 -0700 Subject: [PATCH v1 1/2] wip: fix EEOP_HASHED_SCALARARRAYOP state width. Author: Reviewed-By: Discussion: https://postgr.es/m/20220616233130.rparivafipt6d...@alap3.anarazel.de Backpatch: --- src/include/executor/execExpr.h | 5 ----- src/backend/executor/execExpr.c | 10 ---------- src/backend/executor/execExprInterp.c | 18 ++++++++++++++---- 3 files changed, 14 insertions(+), 19 deletions(-) diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h index e34db8c93cb..2fba1450c65 100644 --- a/src/include/executor/execExpr.h +++ b/src/include/executor/execExpr.h @@ -582,12 +582,7 @@ typedef struct ExprEvalStep struct ScalarArrayOpExprHashTable *elements_tab; FmgrInfo *finfo; /* function's lookup data */ FunctionCallInfo fcinfo_data; /* arguments etc */ - /* faster to access without additional indirection: */ - PGFunction fn_addr; /* actual call address */ FmgrInfo *hash_finfo; /* function's lookup data */ - FunctionCallInfo hash_fcinfo_data; /* arguments etc */ - /* faster to access without additional indirection: */ - PGFunction hash_fn_addr; /* actual call address */ } hashedscalararrayop; /* for EEOP_XMLEXPR */ diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index 2831e7978b5..4128248be40 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -1204,7 +1204,6 @@ ExecInitExprRec(Expr *node, ExprState *state, FunctionCallInfo fcinfo; AclResult aclresult; FmgrInfo *hash_finfo; - FunctionCallInfo hash_fcinfo; Oid cmpfuncid; /* @@ -1263,16 +1262,10 @@ ExecInitExprRec(Expr *node, ExprState *state, if (OidIsValid(opexpr->hashfuncid)) { hash_finfo = palloc0(sizeof(FmgrInfo)); - hash_fcinfo = palloc0(SizeForFunctionCallInfo(1)); fmgr_info(opexpr->hashfuncid, hash_finfo); fmgr_info_set_expr((Node *) node, hash_finfo); - InitFunctionCallInfoData(*hash_fcinfo, hash_finfo, - 1, opexpr->inputcollid, NULL, - NULL); scratch.d.hashedscalararrayop.hash_finfo = hash_finfo; - scratch.d.hashedscalararrayop.hash_fcinfo_data = hash_fcinfo; - scratch.d.hashedscalararrayop.hash_fn_addr = hash_finfo->fn_addr; /* Evaluate scalar directly into left function argument */ ExecInitExprRec(scalararg, state, @@ -1292,11 +1285,8 @@ ExecInitExprRec(Expr *node, ExprState *state, scratch.d.hashedscalararrayop.inclause = opexpr->useOr; scratch.d.hashedscalararrayop.finfo = finfo; scratch.d.hashedscalararrayop.fcinfo_data = fcinfo; - scratch.d.hashedscalararrayop.fn_addr = finfo->fn_addr; scratch.d.hashedscalararrayop.hash_finfo = hash_finfo; - scratch.d.hashedscalararrayop.hash_fcinfo_data = hash_fcinfo; - scratch.d.hashedscalararrayop.hash_fn_addr = hash_finfo->fn_addr; ExprEvalPushStep(state, &scratch); } diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index eaec697bb38..77e1cb1b7b4 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -217,6 +217,7 @@ typedef struct ScalarArrayOpExprHashTable { saophash_hash *hashtab; /* underlying hash table */ struct ExprEvalStep *op; + FunctionCallInfo hash_fcinfo_data; /* arguments etc */ } ScalarArrayOpExprHashTable; /* Define parameters for ScalarArrayOpExpr hash table code generation. */ @@ -3474,13 +3475,13 @@ static uint32 saop_element_hash(struct saophash_hash *tb, Datum key) { ScalarArrayOpExprHashTable *elements_tab = (ScalarArrayOpExprHashTable *) tb->private_data; - FunctionCallInfo fcinfo = elements_tab->op->d.hashedscalararrayop.hash_fcinfo_data; + FunctionCallInfo fcinfo = elements_tab->hash_fcinfo_data; Datum hash; fcinfo->args[0].value = key; fcinfo->args[0].isnull = false; - hash = elements_tab->op->d.hashedscalararrayop.hash_fn_addr(fcinfo); + hash = elements_tab->op->d.hashedscalararrayop.hash_finfo->fn_addr(fcinfo); return DatumGetUInt32(hash); } @@ -3502,7 +3503,7 @@ saop_hash_element_match(struct saophash_hash *tb, Datum key1, Datum key2) fcinfo->args[1].value = key2; fcinfo->args[1].isnull = false; - result = elements_tab->op->d.hashedscalararrayop.fn_addr(fcinfo); + result = elements_tab->op->d.hashedscalararrayop.finfo->fn_addr(fcinfo); return DatumGetBool(result); } @@ -3575,6 +3576,15 @@ ExecEvalHashedScalarArrayOp(ExprState *state, ExprEvalStep *op, ExprContext *eco op->d.hashedscalararrayop.elements_tab = elements_tab; elements_tab->op = op; + elements_tab->hash_fcinfo_data = palloc0(SizeForFunctionCallInfo(1)); + InitFunctionCallInfoData(*elements_tab->hash_fcinfo_data, + op->d.hashedscalararrayop.hash_finfo, + 1, + /* FIXME */ + ((OpExpr*)op->d.hashedscalararrayop.hash_finfo->fn_expr)->inputcollid, + NULL, + NULL); + /* * Create the hash table sizing it according to the number of elements * in the array. This does assume that the array has no duplicates. @@ -3669,7 +3679,7 @@ ExecEvalHashedScalarArrayOp(ExprState *state, ExprEvalStep *op, ExprContext *eco fcinfo->args[1].value = (Datum) 0; fcinfo->args[1].isnull = true; - result = op->d.hashedscalararrayop.fn_addr(fcinfo); + result = op->d.hashedscalararrayop.finfo->fn_addr(fcinfo); resultnull = fcinfo->isnull; /* -- 2.35.1.677.gabf474a5dd
>From beb2e9aa2c6fef1eb0f513716db6d6ffedddc39b Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Thu, 16 Jun 2022 18:33:42 -0700 Subject: [PATCH v1 2/2] WIP: Fix EEOP_JSON_CONSTRUCTOR and EEOP_JSONEXPR size. Author: Reviewed-By: Discussion: https://postgr.es/m/ Backpatch: --- src/include/executor/execExpr.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h index 2fba1450c65..5bfa7e9bdd4 100644 --- a/src/include/executor/execExpr.h +++ b/src/include/executor/execExpr.h @@ -22,6 +22,8 @@ struct ExprEvalStep; struct SubscriptingRefState; struct ScalarArrayOpExprHashTable; struct JsonbValue; +struct JsonExprState; +struct JsonConstructorExprState; /* Bits in ExprState->flags (see also execnodes.h for public flag bits): */ /* expression's interpreter has been initialized */ -- 2.35.1.677.gabf474a5dd
