Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit e4326fb

Browse files
committed
Remove grotty use of disable_cost for TID scan plans.
Previously, the code charged disable_cost for CurrentOfExpr, and then subtracted disable_cost from the cost of a TID path that used CurrentOfExpr as the TID qual, effectively disabling all paths except that one. Now, we instead suppress generation of the disabled paths entirely, and generate only the one that the executor will actually understand. With this approach, we do not need to rely on disable_cost being large enough to prevent the wrong path from being chosen, and we save some CPU cycle by avoiding generating paths that we can't actually use. In my opinion, the code is also easier to understand like this. Patch by me. Review by Heikki Linnakangas. Discussion: http://postgr.es/m/591b3596-2ea0-4b8e-99c6-fad0ef2801f5@iki.fi
1 parent c0348fd commit e4326fb

File tree

4 files changed

+48
-35
lines changed

4 files changed

+48
-35
lines changed

src/backend/optimizer/path/allpaths.c

+11-3
Original file line numberDiff line numberDiff line change
@@ -772,6 +772,17 @@ set_plain_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte)
772772
*/
773773
required_outer = rel->lateral_relids;
774774

775+
/*
776+
* Consider TID scans.
777+
*
778+
* If create_tidscan_paths returns true, then a TID scan path is forced.
779+
* This happens when rel->baserestrictinfo contains CurrentOfExpr, because
780+
* the executor can't handle any other type of path for such queries.
781+
* Hence, we return without adding any other paths.
782+
*/
783+
if (create_tidscan_paths(root, rel))
784+
return;
785+
775786
/* Consider sequential scan */
776787
add_path(rel, create_seqscan_path(root, rel, required_outer, 0));
777788

@@ -781,9 +792,6 @@ set_plain_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte)
781792

782793
/* Consider index scans */
783794
create_index_paths(root, rel);
784-
785-
/* Consider TID scans */
786-
create_tidscan_paths(root, rel);
787795
}
788796

789797
/*

src/backend/optimizer/path/costsize.c

-26
Original file line numberDiff line numberDiff line change
@@ -1251,7 +1251,6 @@ cost_tidscan(Path *path, PlannerInfo *root,
12511251
{
12521252
Cost startup_cost = 0;
12531253
Cost run_cost = 0;
1254-
bool isCurrentOf = false;
12551254
QualCost qpqual_cost;
12561255
Cost cpu_per_tuple;
12571256
QualCost tid_qual_cost;
@@ -1287,7 +1286,6 @@ cost_tidscan(Path *path, PlannerInfo *root,
12871286
else if (IsA(qual, CurrentOfExpr))
12881287
{
12891288
/* CURRENT OF yields 1 tuple */
1290-
isCurrentOf = true;
12911289
ntuples++;
12921290
}
12931291
else
@@ -1297,22 +1295,6 @@ cost_tidscan(Path *path, PlannerInfo *root,
12971295
}
12981296
}
12991297

1300-
/*
1301-
* We must force TID scan for WHERE CURRENT OF, because only nodeTidscan.c
1302-
* understands how to do it correctly. Therefore, honor enable_tidscan
1303-
* only when CURRENT OF isn't present. Also note that cost_qual_eval
1304-
* counts a CurrentOfExpr as having startup cost disable_cost, which we
1305-
* subtract off here; that's to prevent other plan types such as seqscan
1306-
* from winning.
1307-
*/
1308-
if (isCurrentOf)
1309-
{
1310-
Assert(baserel->baserestrictcost.startup >= disable_cost);
1311-
startup_cost -= disable_cost;
1312-
}
1313-
else if (!enable_tidscan)
1314-
startup_cost += disable_cost;
1315-
13161298
/*
13171299
* The TID qual expressions will be computed once, any other baserestrict
13181300
* quals once per retrieved tuple.
@@ -1399,9 +1381,6 @@ cost_tidrangescan(Path *path, PlannerInfo *root,
13991381
ntuples = selectivity * baserel->tuples;
14001382
nseqpages = pages - 1.0;
14011383

1402-
if (!enable_tidscan)
1403-
startup_cost += disable_cost;
1404-
14051384
/*
14061385
* The TID qual expressions will be computed once, any other baserestrict
14071386
* quals once per retrieved tuple.
@@ -4884,11 +4863,6 @@ cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
48844863
/* Treat all these as having cost 1 */
48854864
context->total.per_tuple += cpu_operator_cost;
48864865
}
4887-
else if (IsA(node, CurrentOfExpr))
4888-
{
4889-
/* Report high cost to prevent selection of anything but TID scan */
4890-
context->total.startup += disable_cost;
4891-
}
48924866
else if (IsA(node, SubLink))
48934867
{
48944868
/* This routine should not be applied to un-planned expressions */

src/backend/optimizer/path/tidpath.c

+36-5
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
#include "catalog/pg_operator.h"
4343
#include "catalog/pg_type.h"
4444
#include "nodes/nodeFuncs.h"
45+
#include "optimizer/cost.h"
4546
#include "optimizer/optimizer.h"
4647
#include "optimizer/pathnode.h"
4748
#include "optimizer/paths.h"
@@ -277,12 +278,15 @@ RestrictInfoIsTidQual(PlannerInfo *root, RestrictInfo *rinfo, RelOptInfo *rel)
277278
* that there's more than one choice.
278279
*/
279280
static List *
280-
TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel)
281+
TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel,
282+
bool *isCurrentOf)
281283
{
282284
RestrictInfo *tidclause = NULL; /* best simple CTID qual so far */
283285
List *orlist = NIL; /* best OR'ed CTID qual so far */
284286
ListCell *l;
285287

288+
*isCurrentOf = false;
289+
286290
foreach(l, rlist)
287291
{
288292
RestrictInfo *rinfo = lfirst_node(RestrictInfo, l);
@@ -305,9 +309,13 @@ TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel)
305309
if (is_andclause(orarg))
306310
{
307311
List *andargs = ((BoolExpr *) orarg)->args;
312+
bool sublistIsCurrentOf;
308313

309314
/* Recurse in case there are sub-ORs */
310-
sublist = TidQualFromRestrictInfoList(root, andargs, rel);
315+
sublist = TidQualFromRestrictInfoList(root, andargs, rel,
316+
&sublistIsCurrentOf);
317+
if (sublistIsCurrentOf)
318+
elog(ERROR, "IS CURRENT OF within OR clause");
311319
}
312320
else
313321
{
@@ -353,7 +361,10 @@ TidQualFromRestrictInfoList(PlannerInfo *root, List *rlist, RelOptInfo *rel)
353361
{
354362
/* We can stop immediately if it's a CurrentOfExpr */
355363
if (IsCurrentOfClause(rinfo, rel))
364+
{
365+
*isCurrentOf = true;
356366
return list_make1(rinfo);
367+
}
357368

358369
/*
359370
* Otherwise, remember the first non-OR CTID qual. We could
@@ -483,19 +494,24 @@ ec_member_matches_ctid(PlannerInfo *root, RelOptInfo *rel,
483494
*
484495
* Candidate paths are added to the rel's pathlist (using add_path).
485496
*/
486-
void
497+
bool
487498
create_tidscan_paths(PlannerInfo *root, RelOptInfo *rel)
488499
{
489500
List *tidquals;
490501
List *tidrangequals;
502+
bool isCurrentOf;
491503

492504
/*
493505
* If any suitable quals exist in the rel's baserestrict list, generate a
494506
* plain (unparameterized) TidPath with them.
507+
*
508+
* We skip this when enable_tidscan = false, except when the qual is
509+
* CurrentOfExpr. In that case, a TID scan is the only correct path.
495510
*/
496-
tidquals = TidQualFromRestrictInfoList(root, rel->baserestrictinfo, rel);
511+
tidquals = TidQualFromRestrictInfoList(root, rel->baserestrictinfo, rel,
512+
&isCurrentOf);
497513

498-
if (tidquals != NIL)
514+
if (tidquals != NIL && (enable_tidscan || isCurrentOf))
499515
{
500516
/*
501517
* This path uses no join clauses, but it could still have required
@@ -505,8 +521,21 @@ create_tidscan_paths(PlannerInfo *root, RelOptInfo *rel)
505521

506522
add_path(rel, (Path *) create_tidscan_path(root, rel, tidquals,
507523
required_outer));
524+
525+
/*
526+
* When the qual is CurrentOfExpr, the path that we just added is the
527+
* only one the executor can handle, so we should return before adding
528+
* any others. Returning true lets the caller know not to add any
529+
* others, either.
530+
*/
531+
if (isCurrentOf)
532+
return true;
508533
}
509534

535+
/* Skip the rest if TID scans are disabled. */
536+
if (!enable_tidscan)
537+
return false;
538+
510539
/*
511540
* If there are range quals in the baserestrict list, generate a
512541
* TidRangePath.
@@ -553,4 +582,6 @@ create_tidscan_paths(PlannerInfo *root, RelOptInfo *rel)
553582
* join quals, for example.
554583
*/
555584
BuildParameterizedTidPaths(root, rel, rel->joininfo);
585+
586+
return false;
556587
}

src/include/optimizer/paths.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ extern void check_index_predicates(PlannerInfo *root, RelOptInfo *rel);
8383
* tidpath.c
8484
* routines to generate tid paths
8585
*/
86-
extern void create_tidscan_paths(PlannerInfo *root, RelOptInfo *rel);
86+
extern bool create_tidscan_paths(PlannerInfo *root, RelOptInfo *rel);
8787

8888
/*
8989
* joinpath.c

0 commit comments

Comments
 (0)