Skip to content

Commit 8f59f6b

Browse files
tglsfdctglsfdc
committed
Improve performance of "simple expressions" in PL/pgSQL.
For relatively simple expressions (say, "x + 1" or "x > 0"), plpgsql's management overhead exceeds the cost of evaluating the expression. This patch substantially improves that situation, providing roughly 2X speedup for such trivial expressions. First, add infrastructure in the plancache to allow fast re-validation of cached plans that contain no table access, and hence need no locks. Teach plpgsql to use this infrastructure for expressions that it's already deemed "simple" (which in particular will never contain table references). The fast path still requires checking that search_path hasn't changed, so provide a fast path for OverrideSearchPathMatchesCurrent by counting changes that have occurred to the active search path in the current session. This is simplistic but seems enough for now, seeing that PushOverrideSearchPath is not used in any performance-critical cases. Second, manage the refcounts on simple expressions' cached plans using a transaction-lifespan resource owner, so that we only need to take and release an expression's refcount once per transaction not once per expression evaluation. The management of this resource owner exactly parallels the existing management of plpgsql's simple-expression EState. Add some regression tests covering this area, in particular verifying that expression caching doesn't break semantics for search_path changes. Patch by me, but it owes something to previous work by Amit Langote, who recognized that getting rid of plancache-related overhead would be a useful thing to do here. Also thanks to Andres Freund for review. Discussion: https://postgr.es/m/CAFj8pRDRVfLdAxsWeVLzCAbkLFZhW549K+67tpOc-faC8uH8zw@mail.gmail.com
1 parent 86e5bad commit 8f59f6b

File tree

12 files changed

+627
-74
lines changed

12 files changed

+627
-74
lines changed

‎src/backend/catalog/namespace.c

Lines changed: 88 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,11 @@
126126
* namespaceUser is the userid the path has been computed for.
127127
*
128128
* Note: all data pointed to by these List variables is in TopMemoryContext.
129+
*
130+
* activePathGeneration is incremented whenever the effective values of
131+
* activeSearchPath/activeCreationNamespace/activeTempCreationPending change.
132+
* This can be used to quickly detect whether any change has happened since
133+
* a previous examination of the search path state.
129134
*/
130135

131136
/* These variables define the actually active state: */
@@ -138,6 +143,9 @@ static Oid activeCreationNamespace = InvalidOid;
138143
/* if true, activeCreationNamespace is wrong, it should be temp namespace */
139144
static bool activeTempCreationPending = false;
140145

146+
/* current generation counter; make sure this is never zero */
147+
static uint64 activePathGeneration = 1;
148+
141149
/* These variables are the values last derived from namespace_search_path: */
142150

143151
static List *baseSearchPath = NIL;
@@ -3373,6 +3381,7 @@ GetOverrideSearchPath(MemoryContext context)
33733381
schemas = list_delete_first(schemas);
33743382
}
33753383
result->schemas = schemas;
3384+
result->generation = activePathGeneration;
33763385

33773386
MemoryContextSwitchTo(oldcxt);
33783387

@@ -3393,12 +3402,18 @@ CopyOverrideSearchPath(OverrideSearchPath *path)
33933402
result->schemas = list_copy(path->schemas);
33943403
result->addCatalog = path->addCatalog;
33953404
result->addTemp = path->addTemp;
3405+
result->generation = path->generation;
33963406

33973407
return result;
33983408
}
33993409

34003410
/*
34013411
* OverrideSearchPathMatchesCurrent - does path match current setting?
3412+
*
3413+
* This is tested over and over in some common code paths, and in the typical
3414+
* scenario where the active search path seldom changes, it'll always succeed.
3415+
* We make that case fast by keeping a generation counter that is advanced
3416+
* whenever the active search path changes.
34023417
*/
34033418
bool
34043419
OverrideSearchPathMatchesCurrent(OverrideSearchPath *path)
@@ -3408,6 +3423,10 @@ OverrideSearchPathMatchesCurrent(OverrideSearchPath *path)
34083423

34093424
recomputeNamespacePath();
34103425

3426+
/* Quick out if already known equal to active path. */
3427+
if (path->generation == activePathGeneration)
3428+
return true;
3429+
34113430
/* We scan down the activeSearchPath to see if it matches the input. */
34123431
lc = list_head(activeSearchPath);
34133432

@@ -3440,6 +3459,13 @@ OverrideSearchPathMatchesCurrent(OverrideSearchPath *path)
34403459
}
34413460
if (lc)
34423461
return false;
3462+
3463+
/*
3464+
* Update path->generation so that future tests will return quickly, so
3465+
* long as the active search path doesn't change.
3466+
*/
3467+
path->generation = activePathGeneration;
3468+
34433469
return true;
34443470
}
34453471

@@ -3510,6 +3536,14 @@ PushOverrideSearchPath(OverrideSearchPath *newpath)
35103536
activeCreationNamespace = entry->creationNamespace;
35113537
activeTempCreationPending = false; /* XXX is this OK? */
35123538

3539+
/*
3540+
* We always increment activePathGeneration when pushing/popping an
3541+
* override path. In current usage, these actions always change the
3542+
* effective path state, so there's no value in checking to see if it
3543+
* didn't change.
3544+
*/
3545+
activePathGeneration++;
3546+
35133547
MemoryContextSwitchTo(oldcxt);
35143548
}
35153549

@@ -3551,6 +3585,9 @@ PopOverrideSearchPath(void)
35513585
activeCreationNamespace = baseCreationNamespace;
35523586
activeTempCreationPending = baseTempCreationPending;
35533587
}
3588+
3589+
/* As above, the generation always increments. */
3590+
activePathGeneration++;
35543591
}
35553592

35563593

@@ -3707,6 +3744,7 @@ recomputeNamespacePath(void)
37073744
ListCell *l;
37083745
bool temp_missing;
37093746
Oid firstNS;
3747+
bool pathChanged;
37103748
MemoryContext oldcxt;
37113749

37123750
/* Do nothing if an override search spec is active. */
@@ -3814,18 +3852,31 @@ recomputeNamespacePath(void)
38143852
oidlist = lcons_oid(myTempNamespace, oidlist);
38153853

38163854
/*
3817-
* Now that we've successfully built the new list of namespace OIDs, save
3818-
* it in permanent storage.
3855+
* We want to detect the case where the effective value of the base search
3856+
* path variables didn't change. As long as we're doing so, we can avoid
3857+
* copying the OID list unncessarily.
38193858
*/
3820-
oldcxt = MemoryContextSwitchTo(TopMemoryContext);
3821-
newpath = list_copy(oidlist);
3822-
MemoryContextSwitchTo(oldcxt);
3859+
if (baseCreationNamespace == firstNS &&
3860+
baseTempCreationPending == temp_missing &&
3861+
equal(oidlist, baseSearchPath))
3862+
{
3863+
pathChanged = false;
3864+
}
3865+
else
3866+
{
3867+
pathChanged = true;
3868+
3869+
/* Must save OID list in permanent storage. */
3870+
oldcxt = MemoryContextSwitchTo(TopMemoryContext);
3871+
newpath = list_copy(oidlist);
3872+
MemoryContextSwitchTo(oldcxt);
38233873

3824-
/* Now safe to assign to state variables. */
3825-
list_free(baseSearchPath);
3826-
baseSearchPath = newpath;
3827-
baseCreationNamespace = firstNS;
3828-
baseTempCreationPending = temp_missing;
3874+
/* Now safe to assign to state variables. */
3875+
list_free(baseSearchPath);
3876+
baseSearchPath = newpath;
3877+
baseCreationNamespace = firstNS;
3878+
baseTempCreationPending = temp_missing;
3879+
}
38293880

38303881
/* Mark the path valid. */
38313882
baseSearchPathValid = true;
@@ -3836,6 +3887,16 @@ recomputeNamespacePath(void)
38363887
activeCreationNamespace = baseCreationNamespace;
38373888
activeTempCreationPending = baseTempCreationPending;
38383889

3890+
/*
3891+
* Bump the generation only if something actually changed. (Notice that
3892+
* what we compared to was the old state of the base path variables; so
3893+
* this does not deal with the situation where we have just popped an
3894+
* override path and restored the prior state of the base path. Instead
3895+
* we rely on the override-popping logic to have bumped the generation.)
3896+
*/
3897+
if (pathChanged)
3898+
activePathGeneration++;
3899+
38393900
/* Clean up. */
38403901
pfree(rawname);
38413902
list_free(namelist);
@@ -4054,6 +4115,8 @@ AtEOXact_Namespace(bool isCommit, bool parallel)
40544115
activeSearchPath = baseSearchPath;
40554116
activeCreationNamespace = baseCreationNamespace;
40564117
activeTempCreationPending = baseTempCreationPending;
4118+
/* Always bump generation --- see note in recomputeNamespacePath */
4119+
activePathGeneration++;
40574120
}
40584121
}
40594122

@@ -4109,6 +4172,8 @@ AtEOSubXact_Namespace(bool isCommit, SubTransactionId mySubid,
41094172
overrideStack = list_delete_first(overrideStack);
41104173
list_free(entry->searchPath);
41114174
pfree(entry);
4175+
/* Always bump generation --- see note in recomputeNamespacePath */
4176+
activePathGeneration++;
41124177
}
41134178

41144179
/* Activate the next level down. */
@@ -4118,13 +4183,25 @@ AtEOSubXact_Namespace(bool isCommit, SubTransactionId mySubid,
41184183
activeSearchPath = entry->searchPath;
41194184
activeCreationNamespace = entry->creationNamespace;
41204185
activeTempCreationPending = false; /* XXX is this OK? */
4186+
4187+
/*
4188+
* It's probably unnecessary to bump generation here, but this should
4189+
* not be a performance-critical case, so better to be over-cautious.
4190+
*/
4191+
activePathGeneration++;
41214192
}
41224193
else
41234194
{
41244195
/* If not baseSearchPathValid, this is useless but harmless */
41254196
activeSearchPath = baseSearchPath;
41264197
activeCreationNamespace = baseCreationNamespace;
41274198
activeTempCreationPending = baseTempCreationPending;
4199+
4200+
/*
4201+
* If we popped an override stack entry, then we already bumped the
4202+
* generation above. If we did not, then the above assignments did
4203+
* nothing and we need not bump the generation.
4204+
*/
41284205
}
41294206
}
41304207

@@ -4264,6 +4341,7 @@ InitializeSearchPath(void)
42644341
activeSearchPath = baseSearchPath;
42654342
activeCreationNamespace = baseCreationNamespace;
42664343
activeTempCreationPending = baseTempCreationPending;
4344+
activePathGeneration++; /* pro forma */
42674345
}
42684346
else
42694347
{

‎src/backend/utils/cache/plancache.c

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1277,6 +1277,160 @@ ReleaseCachedPlan(CachedPlan *plan, bool useResOwner)
12771277
}
12781278
}
12791279

1280+
/*
1281+
* CachedPlanAllowsSimpleValidityCheck: can we use CachedPlanIsSimplyValid?
1282+
*
1283+
* This function, together with CachedPlanIsSimplyValid, provides a fast path
1284+
* for revalidating "simple" generic plans. The core requirement to be simple
1285+
* is that the plan must not require taking any locks, which translates to
1286+
* not touching any tables; this happens to match up well with an important
1287+
* use-case in PL/pgSQL. This function tests whether that's true, along
1288+
* with checking some other corner cases that we'd rather not bother with
1289+
* handling in the fast path. (Note that it's still possible for such a plan
1290+
* to be invalidated, for example due to a change in a function that was
1291+
* inlined into the plan.)
1292+
*
1293+
* This must only be called on known-valid generic plans (eg, ones just
1294+
* returned by GetCachedPlan). If it returns true, the caller may re-use
1295+
* the cached plan as long as CachedPlanIsSimplyValid returns true; that
1296+
* check is much cheaper than the full revalidation done by GetCachedPlan.
1297+
* Nonetheless, no required checks are omitted.
1298+
*/
1299+
bool
1300+
CachedPlanAllowsSimpleValidityCheck(CachedPlanSource *plansource,
1301+
CachedPlan *plan)
1302+
{
1303+
ListCell *lc;
1304+
1305+
/* Sanity-check that the caller gave us a validated generic plan. */
1306+
Assert(plansource->magic == CACHEDPLANSOURCE_MAGIC);
1307+
Assert(plan->magic == CACHEDPLAN_MAGIC);
1308+
Assert(plansource->is_valid);
1309+
Assert(plan->is_valid);
1310+
Assert(plan == plansource->gplan);
1311+
1312+
/* We don't support oneshot plans here. */
1313+
if (plansource->is_oneshot)
1314+
return false;
1315+
Assert(!plan->is_oneshot);
1316+
1317+
/*
1318+
* If the plan is dependent on RLS considerations, or it's transient,
1319+
* reject. These things probably can't ever happen for table-free
1320+
* queries, but for safety's sake let's check.
1321+
*/
1322+
if (plansource->dependsOnRLS)
1323+
return false;
1324+
if (plan->dependsOnRole)
1325+
return false;
1326+
if (TransactionIdIsValid(plan->saved_xmin))
1327+
return false;
1328+
1329+
/*
1330+
* Reject if AcquirePlannerLocks would have anything to do. This is
1331+
* simplistic, but there's no need to inquire any more carefully; indeed,
1332+
* for current callers it shouldn't even be possible to hit any of these
1333+
* checks.
1334+
*/
1335+
foreach(lc, plansource->query_list)
1336+
{
1337+
Query *query = lfirst_node(Query, lc);
1338+
1339+
if (query->commandType == CMD_UTILITY)
1340+
return false;
1341+
if (query->rtable || query->cteList || query->hasSubLinks)
1342+
return false;
1343+
}
1344+
1345+
/*
1346+
* Reject if AcquireExecutorLocks would have anything to do. This is
1347+
* probably unnecessary given the previous check, but let's be safe.
1348+
*/
1349+
foreach(lc, plan->stmt_list)
1350+
{
1351+
PlannedStmt *plannedstmt = lfirst_node(PlannedStmt, lc);
1352+
ListCell *lc2;
1353+
1354+
if (plannedstmt->commandType == CMD_UTILITY)
1355+
return false;
1356+
1357+
/*
1358+
* We have to grovel through the rtable because it's likely to contain
1359+
* an RTE_RESULT relation, rather than being totally empty.
1360+
*/
1361+
foreach(lc2, plannedstmt->rtable)
1362+
{
1363+
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc2);
1364+
1365+
if (rte->rtekind == RTE_RELATION)
1366+
return false;
1367+
}
1368+
}
1369+
1370+
/*
1371+
* Okay, it's simple. Note that what we've primarily established here is
1372+
* that no locks need be taken before checking the plan's is_valid flag.
1373+
*/
1374+
return true;
1375+
}
1376+
1377+
/*
1378+
* CachedPlanIsSimplyValid: quick check for plan still being valid
1379+
*
1380+
* This function must not be used unless CachedPlanAllowsSimpleValidityCheck
1381+
* previously said it was OK.
1382+
*
1383+
* If the plan is valid, and "owner" is not NULL, record a refcount on
1384+
* the plan in that resowner before returning. It is caller's responsibility
1385+
* to be sure that a refcount is held on any plan that's being actively used.
1386+
*
1387+
* The code here is unconditionally safe as long as the only use of this
1388+
* CachedPlanSource is in connection with the particular CachedPlan pointer
1389+
* that's passed in. If the plansource were being used for other purposes,
1390+
* it's possible that its generic plan could be invalidated and regenerated
1391+
* while the current caller wasn't looking, and then there could be a chance
1392+
* collision of address between this caller's now-stale plan pointer and the
1393+
* actual address of the new generic plan. For current uses, that scenario
1394+
* can't happen; but with a plansource shared across multiple uses, it'd be
1395+
* advisable to also save plan->generation and verify that that still matches.
1396+
*/
1397+
bool
1398+
CachedPlanIsSimplyValid(CachedPlanSource *plansource, CachedPlan *plan,
1399+
ResourceOwner owner)
1400+
{
1401+
/*
1402+
* Careful here: since the caller doesn't necessarily hold a refcount on
1403+
* the plan to start with, it's possible that "plan" is a dangling
1404+
* pointer. Don't dereference it until we've verified that it still
1405+
* matches the plansource's gplan (which is either valid or NULL).
1406+
*/
1407+
Assert(plansource->magic == CACHEDPLANSOURCE_MAGIC);
1408+
1409+
/*
1410+
* Has cache invalidation fired on this plan? We can check this right
1411+
* away since there are no locks that we'd need to acquire first.
1412+
*/
1413+
if (!plansource->is_valid || plan != plansource->gplan || !plan->is_valid)
1414+
return false;
1415+
1416+
Assert(plan->magic == CACHEDPLAN_MAGIC);
1417+
1418+
/* Is the search_path still the same as when we made it? */
1419+
Assert(plansource->search_path != NULL);
1420+
if (!OverrideSearchPathMatchesCurrent(plansource->search_path))
1421+
return false;
1422+
1423+
/* It's still good. Bump refcount if requested. */
1424+
if (owner)
1425+
{
1426+
ResourceOwnerEnlargePlanCacheRefs(owner);
1427+
plan->refcount++;
1428+
ResourceOwnerRememberPlanCacheRef(owner, plan);
1429+
}
1430+
1431+
return true;
1432+
}
1433+
12801434
/*
12811435
* CachedPlanSetParentContext: move a CachedPlanSource to a new memory context
12821436
*

0 commit comments

Comments
 (0)