On Tue, 2023-11-21 at 09:24 -0500, Robert Haas wrote: > As to the first, could we > remove the setjmp() and instead have abort-time processing know > something about this? For example, imagine we just push something > onto > a stack like we do for ErrorContextCallback, do whatever, and then > pop > it off. But if an error is thrown then the abort path knows to look > at > that variable and do whatever.
If I remove the TRY/CATCH entirely, it shows there's room for ~200ms improvement in my test. I attached a rough patch, which doesn't quite achieve that much, it's more like ~100ms improvement and starts to fall within the noise. So perhaps an improvement, but a bit disappointing. It's not a lot of code, but it's not trivial either because the nesting level needs to be tracked (so a subxact abort doesn't reset too much state). Also, it's not quite as clean as it could be, because I went to some effort to avoid an alloc/free by keeping the stack within the fcache. I didn't pay a lot of attention to correctness in this particular patch; I was mostly trying a few different formulations for performance measurement. I'm not inclined to commit this in its current form but if someone thinks that it's a worthwhile direction, I can clean it up a bit and reconsider. Regards, Jeff Davis
From e48a54d9880ab65a1e5ad6d136b849bda2e4554e Mon Sep 17 00:00:00 2001 From: Jeff Davis <j...@j-davis.com> Date: Thu, 7 Dec 2023 12:13:22 -0800 Subject: [PATCH v1] Eliminate PG_TRY/CATCH in fmgr_security_definer(). --- src/backend/access/transam/xact.c | 4 ++ src/backend/utils/fmgr/fmgr.c | 107 +++++++++++++++++++----------- src/include/fmgr.h | 2 + 3 files changed, 73 insertions(+), 40 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 536edb3792..36e75cc657 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -196,6 +196,7 @@ typedef struct TransactionStateData TBlockState blockState; /* high-level state */ int nestingLevel; /* transaction nesting depth */ int gucNestLevel; /* GUC context nesting depth */ + int fmgrNestLevel; /* fmgr wrapper nesting depth */ MemoryContext curTransactionContext; /* my xact-lifetime context */ ResourceOwner curTransactionOwner; /* my query resources */ TransactionId *childXids; /* subcommitted child XIDs, in XID order */ @@ -2797,6 +2798,7 @@ AbortTransaction(void) * do abort processing */ AfterTriggerEndXact(false); /* 'false' means it's abort */ + AtEOXact_Fmgr(false, 1); AtAbort_Portals(); smgrDoPendingSyncs(false, is_parallel_worker); AtEOXact_LargeObject(false); @@ -5151,6 +5153,7 @@ AbortSubTransaction(void) if (s->curTransactionOwner) { AfterTriggerEndSubXact(false); + AtEOXact_Fmgr(false, s->fmgrNestLevel); AtSubAbort_Portals(s->subTransactionId, s->parent->subTransactionId, s->curTransactionOwner, @@ -5294,6 +5297,7 @@ PushTransaction(void) s->parent = p; s->nestingLevel = p->nestingLevel + 1; s->gucNestLevel = NewGUCNestLevel(); + s->fmgrNestLevel = CurrentFmgrNestLevel(); s->savepointLevel = p->savepointLevel; s->state = TRANS_DEFAULT; s->blockState = TBLOCK_SUBBEGIN; diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c index 9dfdf890c5..76056d980b 100644 --- a/src/backend/utils/fmgr/fmgr.c +++ b/src/backend/utils/fmgr/fmgr.c @@ -610,13 +610,26 @@ fmgr_internal_function(const char *proname) */ struct fmgr_security_definer_cache { - FmgrInfo flinfo; /* lookup info for target function */ + FunctionCallInfo fcinfo; + FmgrInfo flinfo_target; /* lookup info for target function */ + FmgrInfo *flinfo_wrapper; /* lookup info for target function */ Oid userid; /* userid to set, or InvalidOid */ + int fmgrNestLevel; List *configNames; /* GUC names to set, or NIL */ List *configValues; /* GUC values to set, or NIL */ Datum arg; /* passthrough argument for plugin modules */ + struct fmgr_security_definer_cache *previous; }; +static int FmgrNestLevel = 0; +static struct fmgr_security_definer_cache *CurrentFmgrCache = NULL; + +int +CurrentFmgrNestLevel() +{ + return FmgrNestLevel; +} + /* * Function handler for security-definer/proconfig/plugin-hooked functions. * We extract the OID of the actual function and do a fmgr lookup again. @@ -632,12 +645,11 @@ fmgr_security_definer(PG_FUNCTION_ARGS) { Datum result; struct fmgr_security_definer_cache *volatile fcache; - FmgrInfo *save_flinfo; Oid save_userid; int save_sec_context; ListCell *lc1; ListCell *lc2; - volatile int save_nestlevel; + int save_guc_nestlevel; PgStat_FunctionCallUsage fcusage; if (!fcinfo->flinfo->fn_extra) @@ -651,10 +663,13 @@ fmgr_security_definer(PG_FUNCTION_ARGS) fcache = MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt, sizeof(*fcache)); - fmgr_info_cxt_security(fcinfo->flinfo->fn_oid, &fcache->flinfo, + fcache->fcinfo = fcinfo; + fcache->flinfo_wrapper = fcinfo->flinfo; + fmgr_info_cxt_security(fcinfo->flinfo->fn_oid, &fcache->flinfo_target, fcinfo->flinfo->fn_mcxt, true); - fcache->flinfo.fn_expr = fcinfo->flinfo->fn_expr; + fcache->flinfo_target.fn_expr = fcinfo->flinfo->fn_expr; + fcache->fmgrNestLevel = ++FmgrNestLevel; tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(fcinfo->flinfo->fn_oid)); if (!HeapTupleIsValid(tuple)) @@ -688,9 +703,9 @@ fmgr_security_definer(PG_FUNCTION_ARGS) /* GetUserIdAndSecContext is cheap enough that no harm in a wasted call */ GetUserIdAndSecContext(&save_userid, &save_sec_context); if (fcache->configNames != NIL) /* Need a new GUC nesting level */ - save_nestlevel = NewGUCNestLevel(); + save_guc_nestlevel = NewGUCNestLevel(); else - save_nestlevel = 0; /* keep compiler quiet */ + save_guc_nestlevel = 0; /* keep compiler quiet */ if (OidIsValid(fcache->userid)) SetUserIdAndSecContext(fcache->userid, @@ -711,54 +726,66 @@ fmgr_security_definer(PG_FUNCTION_ARGS) /* function manager hook */ if (fmgr_hook) - (*fmgr_hook) (FHET_START, &fcache->flinfo, &fcache->arg); + (*fmgr_hook) (FHET_START, &fcache->flinfo_target, &fcache->arg); - /* - * We don't need to restore GUC or userid settings on error, because the - * ensuing xact or subxact abort will do that. The PG_TRY block is only - * needed to clean up the flinfo link. - */ - save_flinfo = fcinfo->flinfo; + /* push onto stack */ + fcache->previous = CurrentFmgrCache; + CurrentFmgrCache = fcache; - PG_TRY(); - { - fcinfo->flinfo = &fcache->flinfo; + /* temporarily set to target function during call */ + fcinfo->flinfo = &fcache->flinfo_target; - /* See notes in fmgr_info_cxt_security */ - pgstat_init_function_usage(fcinfo, &fcusage); + /* See notes in fmgr_info_cxt_security */ + pgstat_init_function_usage(fcinfo, &fcusage); - result = FunctionCallInvoke(fcinfo); + result = FunctionCallInvoke(fcinfo); - /* - * We could be calling either a regular or a set-returning function, - * so we have to test to see what finalize flag to use. - */ - pgstat_end_function_usage(&fcusage, - (fcinfo->resultinfo == NULL || - !IsA(fcinfo->resultinfo, ReturnSetInfo) || - ((ReturnSetInfo *) fcinfo->resultinfo)->isDone != ExprMultipleResult)); - } - PG_CATCH(); - { - fcinfo->flinfo = save_flinfo; - if (fmgr_hook) - (*fmgr_hook) (FHET_ABORT, &fcache->flinfo, &fcache->arg); - PG_RE_THROW(); - } - PG_END_TRY(); + /* + * We could be calling either a regular or a set-returning function, + * so we have to test to see what finalize flag to use. + */ + pgstat_end_function_usage(&fcusage, + (fcinfo->resultinfo == NULL || + !IsA(fcinfo->resultinfo, ReturnSetInfo) || + ((ReturnSetInfo *) fcinfo->resultinfo)->isDone != ExprMultipleResult)); - fcinfo->flinfo = save_flinfo; + fcinfo->flinfo = fcache->flinfo_wrapper; if (fcache->configNames != NIL) - AtEOXact_GUC(true, save_nestlevel); + AtEOXact_GUC(true, save_guc_nestlevel); if (OidIsValid(fcache->userid)) SetUserIdAndSecContext(save_userid, save_sec_context); if (fmgr_hook) - (*fmgr_hook) (FHET_END, &fcache->flinfo, &fcache->arg); + (*fmgr_hook) (FHET_END, &fcache->flinfo_target, &fcache->arg); + + /* pop from stack */ + CurrentFmgrCache = fcache->previous; + fcache->previous = NULL; return result; } +void +AtEOXact_Fmgr(bool isCommit, int nestLevel) +{ + while (CurrentFmgrCache && CurrentFmgrCache->fmgrNestLevel >= nestLevel) + { + struct fmgr_security_definer_cache *fcache = CurrentFmgrCache; + + /* reinstall wrapper */ + fcache->fcinfo->flinfo = fcache->flinfo_wrapper; + + if (fmgr_hook) + (*fmgr_hook) (isCommit ? FHET_END : FHET_ABORT, + &fcache->flinfo_target, + &fcache->arg); + + CurrentFmgrCache = fcache->previous; + fcache->previous = NULL; + } + + FmgrNestLevel = nestLevel - 1; +} /*------------------------------------------------------------------------- * Support routines for callers of fmgr-compatible functions diff --git a/src/include/fmgr.h b/src/include/fmgr.h index edf61e53f3..56e5018d23 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -735,6 +735,8 @@ extern bytea *get_fn_opclass_options(FmgrInfo *flinfo); extern bool has_fn_opclass_options(FmgrInfo *flinfo); extern void set_fn_opclass_options(FmgrInfo *flinfo, bytea *options); extern bool CheckFunctionValidatorAccess(Oid validatorOid, Oid functionOid); +extern int CurrentFmgrNestLevel(void); +extern void AtEOXact_Fmgr(bool isCommit, int nestLevel); /* * Routines in dfmgr.c -- 2.34.1