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 <[email protected]>
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
