Skip to content

Commit 04fe805

Browse files
tglsfdctglsfdc
committed
Drop no-op CoerceToDomain nodes from expressions at planning time.
If a domain has no constraints, then CoerceToDomain doesn't really do anything and can be simplified to a RelabelType. This not only eliminates cycles at execution, but allows the planner to optimize better (for instance, match the coerced expression to an index on the underlying column). However, we do have to support invalidating the plan later if a constraint gets added to the domain. That's comparable to the case of a change to a SQL function that had been inlined into a plan, so all the necessary logic already exists for plans depending on functions. We need only duplicate or share that logic for domains. ALTER DOMAIN ADD/DROP CONSTRAINT need to be taught to send out sinval messages for the domain's pg_type entry, since those operations don't update that row. (ALTER DOMAIN SET/DROP NOT NULL do update that row, so no code change is needed for them.) Testing this revealed what's really a pre-existing bug in plpgsql: it caches the SQL-expression-tree expansion of type coercions and had no provision for invalidating entries in that cache. Up to now that was only a problem if such an expression had inlined a SQL function that got changed, which is unlikely though not impossible. But failing to track changes of domain constraints breaks an existing regression test case and would likely cause practical problems too. We could fix that locally in plpgsql, but what seems like a better idea is to build some generic infrastructure in plancache.c to store standalone expressions and track invalidation events for them. (It's tempting to wonder whether plpgsql's "simple expression" stuff could use this code with lower overhead than its current use of the heavyweight plancache APIs. But I've left that idea for later.) Other stuff fixed in passing: * Allow estimate_expression_value() to drop CoerceToDomain unconditionally, effectively assuming that the coercion will succeed. This will improve planner selectivity estimates for cases involving estimatable expressions that are coerced to domains. We could have done this independently of everything else here, but there wasn't previously any need for eval_const_expressions_mutator to know about CoerceToDomain at all. * Use a dlist for plancache.c's list of cached plans, rather than a manually threaded singly-linked list. That eliminates a potential performance problem in DropCachedPlan. * Fix a couple of inconsistencies in typecmds.c about whether operations on domains drop RowExclusiveLock on pg_type. Our common practice is that DDL operations do drop catalog locks, so standardize on that choice. Discussion: https://postgr.es/m/19958.1544122124@sss.pgh.pa.us
1 parent 52ac6cd commit 04fe805

File tree

12 files changed

+510
-92
lines changed

12 files changed

+510
-92
lines changed

‎src/backend/commands/typecmds.c

+19-3
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
#include "parser/parse_type.h"
6363
#include "utils/builtins.h"
6464
#include "utils/fmgroids.h"
65+
#include "utils/inval.h"
6566
#include "utils/lsyscache.h"
6667
#include "utils/memutils.h"
6768
#include "utils/rel.h"
@@ -2297,7 +2298,7 @@ AlterDomainDefault(List *names, Node *defaultRaw)
22972298
ObjectAddressSet(address, TypeRelationId, domainoid);
22982299

22992300
/* Clean up */
2300-
heap_close(rel, NoLock);
2301+
heap_close(rel, RowExclusiveLock);
23012302
heap_freetuple(newtuple);
23022303

23032304
return address;
@@ -2494,8 +2495,6 @@ AlterDomainDropConstraint(List *names, const char *constrName,
24942495
systable_endscan(conscan);
24952496
heap_close(conrel, RowExclusiveLock);
24962497

2497-
heap_close(rel, NoLock);
2498-
24992498
if (!found)
25002499
{
25012500
if (!missing_ok)
@@ -2509,8 +2508,18 @@ AlterDomainDropConstraint(List *names, const char *constrName,
25092508
constrName, TypeNameToString(typename))));
25102509
}
25112510

2511+
/*
2512+
* We must send out an sinval message for the domain, to ensure that any
2513+
* dependent plans get rebuilt. Since this command doesn't change the
2514+
* domain's pg_type row, that won't happen automatically; do it manually.
2515+
*/
2516+
CacheInvalidateHeapTuple(rel, tup, NULL);
2517+
25122518
ObjectAddressSet(address, TypeRelationId, domainoid);
25132519

2520+
/* Clean up */
2521+
heap_close(rel, RowExclusiveLock);
2522+
25142523
return address;
25152524
}
25162525

@@ -2615,6 +2624,13 @@ AlterDomainAddConstraint(List *names, Node *newConstraint,
26152624
if (!constr->skip_validation)
26162625
validateDomainConstraint(domainoid, ccbin);
26172626

2627+
/*
2628+
* We must send out an sinval message for the domain, to ensure that any
2629+
* dependent plans get rebuilt. Since this command doesn't change the
2630+
* domain's pg_type row, that won't happen automatically; do it manually.
2631+
*/
2632+
CacheInvalidateHeapTuple(typrel, tup, NULL);
2633+
26182634
ObjectAddressSet(address, TypeRelationId, domainoid);
26192635

26202636
/* Clean up */

‎src/backend/optimizer/plan/planner.c

+58-1
Original file line numberDiff line numberDiff line change
@@ -5923,10 +5923,16 @@ adjust_paths_for_srfs(PlannerInfo *root, RelOptInfo *rel,
59235923
* side-effect that is useful when the expression will get evaluated more than
59245924
* once. Also, we must fix operator function IDs.
59255925
*
5926+
* This does not return any information about dependencies of the expression.
5927+
* Hence callers should use the results only for the duration of the current
5928+
* query. Callers that would like to cache the results for longer should use
5929+
* expression_planner_with_deps, probably via the plancache.
5930+
*
59265931
* Note: this must not make any damaging changes to the passed-in expression
59275932
* tree. (It would actually be okay to apply fix_opfuncids to it, but since
59285933
* we first do an expression_tree_mutator-based walk, what is returned will
5929-
* be a new node tree.)
5934+
* be a new node tree.) The result is constructed in the current memory
5935+
* context; beware that this can leak a lot of additional stuff there, too.
59305936
*/
59315937
Expr *
59325938
expression_planner(Expr *expr)
@@ -5945,6 +5951,57 @@ expression_planner(Expr *expr)
59455951
return (Expr *) result;
59465952
}
59475953

5954+
/*
5955+
* expression_planner_with_deps
5956+
* Perform planner's transformations on a standalone expression,
5957+
* returning expression dependency information along with the result.
5958+
*
5959+
* This is identical to expression_planner() except that it also returns
5960+
* information about possible dependencies of the expression, ie identities of
5961+
* objects whose definitions affect the result. As in a PlannedStmt, these
5962+
* are expressed as a list of relation Oids and a list of PlanInvalItems.
5963+
*/
5964+
Expr *
5965+
expression_planner_with_deps(Expr *expr,
5966+
List **relationOids,
5967+
List **invalItems)
5968+
{
5969+
Node *result;
5970+
PlannerGlobal glob;
5971+
PlannerInfo root;
5972+
5973+
/* Make up dummy planner state so we can use setrefs machinery */
5974+
MemSet(&glob, 0, sizeof(glob));
5975+
glob.type = T_PlannerGlobal;
5976+
glob.relationOids = NIL;
5977+
glob.invalItems = NIL;
5978+
5979+
MemSet(&root, 0, sizeof(root));
5980+
root.type = T_PlannerInfo;
5981+
root.glob = &glob;
5982+
5983+
/*
5984+
* Convert named-argument function calls, insert default arguments and
5985+
* simplify constant subexprs. Collect identities of inlined functions
5986+
* and elided domains, too.
5987+
*/
5988+
result = eval_const_expressions(&root, (Node *) expr);
5989+
5990+
/* Fill in opfuncid values if missing */
5991+
fix_opfuncids(result);
5992+
5993+
/*
5994+
* Now walk the finished expression to find anything else we ought to
5995+
* record as an expression dependency.
5996+
*/
5997+
(void) extract_query_dependencies_walker(result, &root);
5998+
5999+
*relationOids = glob.relationOids;
6000+
*invalItems = glob.invalItems;
6001+
6002+
return (Expr *) result;
6003+
}
6004+
59486005

59496006
/*
59506007
* plan_cluster_use_sort

‎src/backend/optimizer/plan/setrefs.c

+57-7
Original file line numberDiff line numberDiff line change
@@ -138,8 +138,7 @@ static List *set_returning_clause_references(PlannerInfo *root,
138138
Plan *topplan,
139139
Index resultRelation,
140140
int rtoffset);
141-
static bool extract_query_dependencies_walker(Node *node,
142-
PlannerInfo *context);
141+
143142

144143
/*****************************************************************************
145144
*
@@ -175,8 +174,8 @@ static bool extract_query_dependencies_walker(Node *node,
175174
* This will be used by plancache.c to drive invalidation of cached plans.
176175
* Relation dependencies are represented by OIDs, and everything else by
177176
* PlanInvalItems (this distinction is motivated by the shared-inval APIs).
178-
* Currently, relations and user-defined functions are the only types of
179-
* objects that are explicitly tracked this way.
177+
* Currently, relations, user-defined functions, and domains are the only
178+
* types of objects that are explicitly tracked this way.
180179
*
181180
* 8. We assign every plan node in the tree a unique ID.
182181
*
@@ -2577,6 +2576,42 @@ record_plan_function_dependency(PlannerInfo *root, Oid funcid)
25772576
}
25782577
}
25792578

2579+
/*
2580+
* record_plan_type_dependency
2581+
* Mark the current plan as depending on a particular type.
2582+
*
2583+
* This is exported so that eval_const_expressions can record a
2584+
* dependency on a domain that it's removed a CoerceToDomain node for.
2585+
*
2586+
* We don't currently need to record dependencies on domains that the
2587+
* plan contains CoerceToDomain nodes for, though that might change in
2588+
* future. Hence, this isn't actually called in this module, though
2589+
* someday fix_expr_common might call it.
2590+
*/
2591+
void
2592+
record_plan_type_dependency(PlannerInfo *root, Oid typeid)
2593+
{
2594+
/*
2595+
* As in record_plan_function_dependency, ignore the possibility that
2596+
* someone would change a built-in domain.
2597+
*/
2598+
if (typeid >= (Oid) FirstBootstrapObjectId)
2599+
{
2600+
PlanInvalItem *inval_item = makeNode(PlanInvalItem);
2601+
2602+
/*
2603+
* It would work to use any syscache on pg_type, but the easiest is
2604+
* TYPEOID since we already have the type's OID at hand. Note that
2605+
* plancache.c knows we use TYPEOID.
2606+
*/
2607+
inval_item->cacheId = TYPEOID;
2608+
inval_item->hashValue = GetSysCacheHashValue1(TYPEOID,
2609+
ObjectIdGetDatum(typeid));
2610+
2611+
root->glob->invalItems = lappend(root->glob->invalItems, inval_item);
2612+
}
2613+
}
2614+
25802615
/*
25812616
* extract_query_dependencies
25822617
* Given a rewritten, but not yet planned, query or queries
@@ -2586,6 +2621,13 @@ record_plan_function_dependency(PlannerInfo *root, Oid funcid)
25862621
*
25872622
* This is needed by plancache.c to handle invalidation of cached unplanned
25882623
* queries.
2624+
*
2625+
* Note: this does not go through eval_const_expressions, and hence doesn't
2626+
* reflect its additions of inlined functions and elided CoerceToDomain nodes
2627+
* to the invalItems list. This is obviously OK for functions, since we'll
2628+
* see them in the original query tree anyway. For domains, it's OK because
2629+
* we don't care about domains unless they get elided. That is, a plan might
2630+
* have domain dependencies that the query tree doesn't.
25892631
*/
25902632
void
25912633
extract_query_dependencies(Node *query,
@@ -2615,14 +2657,20 @@ extract_query_dependencies(Node *query,
26152657
*hasRowSecurity = glob.dependsOnRole;
26162658
}
26172659

2618-
static bool
2660+
/*
2661+
* Tree walker for extract_query_dependencies.
2662+
*
2663+
* This is exported so that expression_planner_with_deps can call it on
2664+
* simple expressions (post-planning, not before planning, in that case).
2665+
* In that usage, glob.dependsOnRole isn't meaningful, but the relationOids
2666+
* and invalItems lists are added to as needed.
2667+
*/
2668+
bool
26192669
extract_query_dependencies_walker(Node *node, PlannerInfo *context)
26202670
{
26212671
if (node == NULL)
26222672
return false;
26232673
Assert(!IsA(node, PlaceHolderVar));
2624-
/* Extract function dependencies and check for regclass Consts */
2625-
fix_expr_common(context, node);
26262674
if (IsA(node, Query))
26272675
{
26282676
Query *query = (Query *) node;
@@ -2662,6 +2710,8 @@ extract_query_dependencies_walker(Node *node, PlannerInfo *context)
26622710
return query_tree_walker(query, extract_query_dependencies_walker,
26632711
(void *) context, 0);
26642712
}
2713+
/* Extract function dependencies and check for regclass Consts */
2714+
fix_expr_common(context, node);
26652715
return expression_tree_walker(node, extract_query_dependencies_walker,
26662716
(void *) context);
26672717
}

‎src/backend/optimizer/util/clauses.c

+65-1
Original file line numberDiff line numberDiff line change
@@ -3699,6 +3699,70 @@ eval_const_expressions_mutator(Node *node,
36993699
newbtest->location = btest->location;
37003700
return (Node *) newbtest;
37013701
}
3702+
case T_CoerceToDomain:
3703+
{
3704+
/*
3705+
* If the domain currently has no constraints, we replace the
3706+
* CoerceToDomain node with a simple RelabelType, which is
3707+
* both far faster to execute and more amenable to later
3708+
* optimization. We must then mark the plan as needing to be
3709+
* rebuilt if the domain's constraints change.
3710+
*
3711+
* Also, in estimation mode, always replace CoerceToDomain
3712+
* nodes, effectively assuming that the coercion will succeed.
3713+
*/
3714+
CoerceToDomain *cdomain = (CoerceToDomain *) node;
3715+
CoerceToDomain *newcdomain;
3716+
Node *arg;
3717+
3718+
arg = eval_const_expressions_mutator((Node *) cdomain->arg,
3719+
context);
3720+
if (context->estimate ||
3721+
!DomainHasConstraints(cdomain->resulttype))
3722+
{
3723+
/* Record dependency, if this isn't estimation mode */
3724+
if (context->root && !context->estimate)
3725+
record_plan_type_dependency(context->root,
3726+
cdomain->resulttype);
3727+
3728+
/* Generate RelabelType to substitute for CoerceToDomain */
3729+
/* This should match the RelabelType logic above */
3730+
3731+
while (arg && IsA(arg, RelabelType))
3732+
arg = (Node *) ((RelabelType *) arg)->arg;
3733+
3734+
if (arg && IsA(arg, Const))
3735+
{
3736+
Const *con = (Const *) arg;
3737+
3738+
con->consttype = cdomain->resulttype;
3739+
con->consttypmod = cdomain->resulttypmod;
3740+
con->constcollid = cdomain->resultcollid;
3741+
return (Node *) con;
3742+
}
3743+
else
3744+
{
3745+
RelabelType *newrelabel = makeNode(RelabelType);
3746+
3747+
newrelabel->arg = (Expr *) arg;
3748+
newrelabel->resulttype = cdomain->resulttype;
3749+
newrelabel->resulttypmod = cdomain->resulttypmod;
3750+
newrelabel->resultcollid = cdomain->resultcollid;
3751+
newrelabel->relabelformat = cdomain->coercionformat;
3752+
newrelabel->location = cdomain->location;
3753+
return (Node *) newrelabel;
3754+
}
3755+
}
3756+
3757+
newcdomain = makeNode(CoerceToDomain);
3758+
newcdomain->arg = (Expr *) arg;
3759+
newcdomain->resulttype = cdomain->resulttype;
3760+
newcdomain->resulttypmod = cdomain->resulttypmod;
3761+
newcdomain->resultcollid = cdomain->resultcollid;
3762+
newcdomain->coercionformat = cdomain->coercionformat;
3763+
newcdomain->location = cdomain->location;
3764+
return (Node *) newcdomain;
3765+
}
37023766
case T_PlaceHolderVar:
37033767

37043768
/*
@@ -3770,7 +3834,7 @@ eval_const_expressions_mutator(Node *node,
37703834
* For any node type not handled above, copy the node unchanged but
37713835
* const-simplify its subexpressions. This is the correct thing for node
37723836
* types whose behavior might change between planning and execution, such
3773-
* as CoerceToDomain. It's also a safe default for new node types not
3837+
* as CurrentOfExpr. It's also a safe default for new node types not
37743838
* known to this routine.
37753839
*/
37763840
return ece_generic_processing(node);

0 commit comments

Comments
 (0)