Is it expected behavior index only scan shows "OPERATOR(pg_catalog." for EXPLAIN?
Hi,
While I'm researching about [1]Improve EXPLAIN output for multicolumn B-Tree Index /messages/by-id/TYWPR01MB1098260B694D27758FE2BA46FB1C92@TYWPR01MB10982.jpnprd01.prod.outlook.com, I found there are inconsistent EXPLAIN outputs.
Here is an example which shows " OPERATOR(pg_catalog.". Though it's not wrong,
I feel like there is no consistency in the output format.
-- A reproduce procedure
create temp table btree_bpchar (f1 text collate "C");
create index on btree_bpchar(f1 bpchar_ops);
insert into btree_bpchar values ('foo'), ('fool'), ('bar'), ('quux');
set enable_seqscan to false;
set enable_bitmapscan to false;
set enable_indexonlyscan to false; -- or true
explain (costs off)
select * from btree_bpchar where f1::bpchar like 'foo';
-- Index Scan result
QUERY PLAN
------------------------------------------------------
Index Scan using btree_bpchar_f1_idx on btree_bpchar
Index Cond: ((f1)::bpchar = 'foo'::bpchar)
Filter: ((f1)::bpchar ~~ 'foo'::text)
(3 rows)
-- Index Only Scan result which has 'OPERATOR'
QUERY PLAN
-----------------------------------------------------------
Index Only Scan using btree_bpchar_f1_idx on btree_bpchar
Index Cond: (f1 OPERATOR(pg_catalog.=) 'foo'::bpchar) -- Here is the point.
Filter: ((f1)::bpchar ~~ 'foo'::text)
(3 rows)
IIUC, the index only scan use fixed_indexquals, which is removed "RelabelType" nodes,
for EXPLAIN so that get_rule_expr() could not understand the left argument of the operator
(f1 if the above case) can be displayed with arg::resulttype and it doesn't need to
show "OPERATOR(pg_catalog.)".
I've attached PoC patch to show a simple solution. It just adds a new member "indexqualorig"
to the index only scan node like the index scan and the bitmap index scan. But, since I'm
a beginner about the planner, I might have misunderstood something or there should be better
ways.
BTW, I'm trying to add a new index AM interface for EXPLAIN on the thread([1]Improve EXPLAIN output for multicolumn B-Tree Index /messages/by-id/TYWPR01MB1098260B694D27758FE2BA46FB1C92@TYWPR01MB10982.jpnprd01.prod.outlook.com). As the aspect,
my above solution might not be ideal because AMs can only know index column ids (varattno)
from fixed_indexquals. In that case, to support "fixed_indexquals" as argument of deparse_expression()
is better.
[1]: Improve EXPLAIN output for multicolumn B-Tree Index /messages/by-id/TYWPR01MB1098260B694D27758FE2BA46FB1C92@TYWPR01MB10982.jpnprd01.prod.outlook.com
/messages/by-id/TYWPR01MB1098260B694D27758FE2BA46FB1C92@TYWPR01MB10982.jpnprd01.prod.outlook.com
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
Attachments:
v1-0001-Fix-inconsistent-explain-output-for-index-only-sc.patchapplication/octet-stream; name=v1-0001-Fix-inconsistent-explain-output-for-index-only-sc.patchDownload
From fe8252c21cdb003a9b4cb38ca219331e426ac7a3 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <Masahiro.Ikeda@nttdata.com>
Date: Mon, 8 Jul 2024 19:25:43 +0900
Subject: [PATCH v1] Fix inconsistent explain output for index only scan
This fixes the case which the index only scan could not understand
whether it should display var as cast for EXPLAIN. An unexpected
example output is the following.
QUERY PLAN
-----------------------------------------------------------
Index Only Scan using btree_bpchar_f1_idx on btree_bpchar
Index Cond: (f1 OPERATOR(pg_catalog.=) 'foo'::bpchar)
Filter: ((f1)::bpchar ~~ 'foo'::text)
(3 rows)
TODO
* Add comments
* Rethink that set_deparse_plan() need to setup for IndexOnlyScan
---
src/backend/commands/explain.c | 2 +-
src/backend/optimizer/plan/createplan.c | 2 ++
src/backend/optimizer/plan/setrefs.c | 2 ++
src/backend/optimizer/plan/subselect.c | 5 ++++
src/include/nodes/plannodes.h | 1 +
src/test/regress/expected/btree_index.out | 36 +++++++++++++++++++++++
src/test/regress/expected/gist.out | 6 ++--
src/test/regress/expected/inherit.out | 4 +--
src/test/regress/sql/btree_index.sql | 14 +++++++++
9 files changed, 66 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 1e80fd8b68..52882afe9b 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1981,7 +1981,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
planstate, es);
break;
case T_IndexOnlyScan:
- show_scan_qual(((IndexOnlyScan *) plan)->indexqual,
+ show_scan_qual(((IndexOnlyScan *) plan)->indexqualorig,
"Index Cond", planstate, ancestors, es);
if (((IndexOnlyScan *) plan)->recheckqual)
show_instrumentation_count("Rows Removed by Index Recheck", 2,
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 6b64c4a362..4767ddd45c 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -5584,6 +5584,7 @@ make_indexonlyscan(List *qptlist,
{
IndexOnlyScan *node = makeNode(IndexOnlyScan);
Plan *plan = &node->scan.plan;
+ List *indexqualorig = copyObject(recheckqual);
plan->targetlist = qptlist;
plan->qual = qpqual;
@@ -5592,6 +5593,7 @@ make_indexonlyscan(List *qptlist,
node->scan.scanrelid = scanrelid;
node->indexid = indexid;
node->indexqual = indexqual;
+ node->indexqualorig = indexqualorig;
node->recheckqual = recheckqual;
node->indexorderby = indexorderby;
node->indextlist = indextlist;
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 7aed84584c..72e79ae55e 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1372,6 +1372,8 @@ set_indexonlyscan_references(PlannerInfo *root,
/* indexqual is already transformed to reference index columns */
plan->indexqual = fix_scan_list(root, plan->indexqual,
rtoffset, 1);
+ plan->indexqualorig = fix_scan_list(root, plan->indexqualorig,
+ rtoffset, NUM_EXEC_QUAL((Plan *) plan));
/* indexorderby is already transformed to reference index columns */
plan->indexorderby = fix_scan_list(root, plan->indexorderby,
rtoffset, 1);
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 6d003cc8e5..c0e1745a3b 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -2400,6 +2400,11 @@ finalize_plan(PlannerInfo *root, Plan *plan,
* we need not look at indextlist, since it cannot contain Params.
*/
context.paramids = bms_add_members(context.paramids, scan_params);
+
+ /*
+ * we need not look at indexqualorig, since it will have the same
+ * param references as indexqual.
+ */
break;
case T_BitmapIndexScan:
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 1aeeaec95e..4a8ab48ebb 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -494,6 +494,7 @@ typedef struct IndexOnlyScan
Scan scan;
Oid indexid; /* OID of index to scan */
List *indexqual; /* list of index quals (usually OpExprs) */
+ List *indexqualorig; /* the same in original form */
List *recheckqual; /* index quals in recheckable form */
List *indexorderby; /* list of index ORDER BY exprs */
List *indextlist; /* TargetEntry list describing index's cols */
diff --git a/src/test/regress/expected/btree_index.out b/src/test/regress/expected/btree_index.out
index 510646cbce..dddc3ca970 100644
--- a/src/test/regress/expected/btree_index.out
+++ b/src/test/regress/expected/btree_index.out
@@ -409,6 +409,42 @@ select * from btree_bpchar where f1::bpchar like 'foo%';
fool
(2 rows)
+-- Test for indexonlyscan with binary-compatible cases
+set enable_seqscan to false;
+set enable_bitmapscan to false;
+explain (costs off)
+select * from btree_bpchar where f1::bpchar like 'foo';
+ QUERY PLAN
+-----------------------------------------------------------
+ Index Only Scan using btree_bpchar_f1_idx on btree_bpchar
+ Index Cond: ((f1)::bpchar = 'foo'::bpchar)
+ Filter: ((f1)::bpchar ~~ 'foo'::text)
+(3 rows)
+
+select * from btree_bpchar where f1::bpchar like 'foo';
+ f1
+-----
+ foo
+(1 row)
+
+explain (costs off)
+select * from btree_bpchar where f1::bpchar like 'foo%';
+ QUERY PLAN
+------------------------------------------------------------------------------------
+ Index Only Scan using btree_bpchar_f1_idx on btree_bpchar
+ Index Cond: (((f1)::bpchar >= 'foo'::bpchar) AND ((f1)::bpchar < 'fop'::bpchar))
+ Filter: ((f1)::bpchar ~~ 'foo%'::text)
+(3 rows)
+
+select * from btree_bpchar where f1::bpchar like 'foo%';
+ f1
+------
+ foo
+ fool
+(2 rows)
+
+reset enable_seqscan;
+reset enable_bitmapscan;
-- get test coverage for "single value" deduplication strategy:
insert into btree_bpchar select 'foo' from generate_series(1,1500);
--
diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out
index c75bbb23b6..fdd525e655 100644
--- a/src/test/regress/expected/gist.out
+++ b/src/test/regress/expected/gist.out
@@ -344,11 +344,11 @@ where p <@ box(point(5, 5), point(5.3, 5.3));
-- are done correctly.
explain (verbose, costs off)
select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
- QUERY PLAN
----------------------------------------------------------------------------------------
+ QUERY PLAN
+-------------------------------------------------------------------------------------
Index Only Scan using gist_tbl_multi_index on public.gist_tbl
Output: p
- Index Cond: ((circle(gist_tbl.p, '1'::double precision)) @> '<(0,0),0.95>'::circle)
+ Index Cond: (circle(gist_tbl.p, '1'::double precision) @> '<(0,0),0.95>'::circle)
(3 rows)
select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index ad73213414..cf488049bb 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2374,11 +2374,11 @@ explain (costs off) select min(a), max(a) from parted_minmax where b = '12345';
InitPlan 1
-> Limit
-> Index Only Scan using parted_minmax1i on parted_minmax1 parted_minmax
- Index Cond: ((a IS NOT NULL) AND (b = '12345'::text))
+ Index Cond: ((a IS NOT NULL) AND ((b)::text = '12345'::text))
InitPlan 2
-> Limit
-> Index Only Scan Backward using parted_minmax1i on parted_minmax1 parted_minmax_1
- Index Cond: ((a IS NOT NULL) AND (b = '12345'::text))
+ Index Cond: ((a IS NOT NULL) AND ((b)::text = '12345'::text))
(9 rows)
select min(a), max(a) from parted_minmax where b = '12345';
diff --git a/src/test/regress/sql/btree_index.sql b/src/test/regress/sql/btree_index.sql
index 0d2a33f370..930b7c6b2b 100644
--- a/src/test/regress/sql/btree_index.sql
+++ b/src/test/regress/sql/btree_index.sql
@@ -202,6 +202,20 @@ explain (costs off)
select * from btree_bpchar where f1::bpchar like 'foo%';
select * from btree_bpchar where f1::bpchar like 'foo%';
+-- Test for indexonlyscan with binary-compatible cases
+set enable_seqscan to false;
+set enable_bitmapscan to false;
+
+explain (costs off)
+select * from btree_bpchar where f1::bpchar like 'foo';
+select * from btree_bpchar where f1::bpchar like 'foo';
+explain (costs off)
+select * from btree_bpchar where f1::bpchar like 'foo%';
+select * from btree_bpchar where f1::bpchar like 'foo%';
+
+reset enable_seqscan;
+reset enable_bitmapscan;
+
-- get test coverage for "single value" deduplication strategy:
insert into btree_bpchar select 'foo' from generate_series(1,1500);
--
2.34.1
On 7/8/24 13:03, Masahiro.Ikeda@nttdata.com wrote:
Hi,
While I'm researching about [1], I found there are inconsistent EXPLAIN outputs.
Here is an example which shows " OPERATOR(pg_catalog.". Though it's not wrong,
I feel like there is no consistency in the output format.-- A reproduce procedure
create temp table btree_bpchar (f1 text collate "C");
create index on btree_bpchar(f1 bpchar_ops);
insert into btree_bpchar values ('foo'), ('fool'), ('bar'), ('quux');
set enable_seqscan to false;
set enable_bitmapscan to false;
set enable_indexonlyscan to false; -- or true
explain (costs off)
select * from btree_bpchar where f1::bpchar like 'foo';-- Index Scan result
QUERY PLAN
------------------------------------------------------
Index Scan using btree_bpchar_f1_idx on btree_bpchar
Index Cond: ((f1)::bpchar = 'foo'::bpchar)
Filter: ((f1)::bpchar ~~ 'foo'::text)
(3 rows)-- Index Only Scan result which has 'OPERATOR'
QUERY PLAN
-----------------------------------------------------------
Index Only Scan using btree_bpchar_f1_idx on btree_bpchar
Index Cond: (f1 OPERATOR(pg_catalog.=) 'foo'::bpchar) -- Here is the point.
Filter: ((f1)::bpchar ~~ 'foo'::text)
(3 rows)
This apparently comes from generate_operator_name() in ruleutils.c,
where the OPERATOR() decorator is added if:
/*
* The idea here is to schema-qualify only if the parser would fail to
* resolve the correct operator given the unqualified op name with the
* specified argtypes.
*/
So clearly, the code believes just the operator name could be ambiguous,
so it adds the namespace too. Why exactly it is considered ambiguous I
don't know, but perhaps you have other applicable operators in the
search_path, or something like that?
IIUC, the index only scan use fixed_indexquals, which is removed "RelabelType" nodes,
for EXPLAIN so that get_rule_expr() could not understand the left argument of the operator
(f1 if the above case) can be displayed with arg::resulttype and it doesn't need to
show "OPERATOR(pg_catalog.)".I've attached PoC patch to show a simple solution. It just adds a new member "indexqualorig"
to the index only scan node like the index scan and the bitmap index scan. But, since I'm
a beginner about the planner, I might have misunderstood something or there should be better
ways.
I honestly don't know if this is the correct solution. It seems to me
handling this at the EXPLAIN level might just mask the issue - it's not
clear to me why adding "indexqualorig" would remove the ambiguity (if
there's one). Perhaps it might be better to find why the ruleutils.c
code thinks the OPERATOR() is necessary, and then improve/fix that?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
I honestly don't know if this is the correct solution. It seems to me
handling this at the EXPLAIN level might just mask the issue - it's not
clear to me why adding "indexqualorig" would remove the ambiguity (if
there's one). Perhaps it might be better to find why the ruleutils.c
code thinks the OPERATOR() is necessary, and then improve/fix that?
As Masahiro-san already said, the root cause is that the planner
removes the RelabelType that is on the indexed variable. So ruleutils
sees that the operator is not the one that would be chosen by the
parser given those input expressions (cf generate_operator_name)
and it concludes it'd better schema-qualify the operator. While
that doesn't really make any difference in this particular case,
it is the right thing in typical rule-deparsing cases.
I don't think this output is really wrong, and I'm not in favor
of adding nontrivial overhead to make it better, so I don't like
the proposed patch. Maybe generate_operator_name could use some
other heuristic, but I'm unsure what. The case it wants to cover
is that the operator is in the search path but is masked by some
operator in an earlier schema, so simply testing if the operator's
schema is in the path at all would be insufficient.
regards, tom lane
Thanks for your comments.
Tom Lane <tgl@sss.pgh.pa.us> writes:
Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
I honestly don't know if this is the correct solution. It seems to me
handling this at the EXPLAIN level might just mask the issue - it's
not clear to me why adding "indexqualorig" would remove the ambiguity
(if there's one). Perhaps it might be better to find why the
ruleutils.c code thinks the OPERATOR() is necessary, and then improve/fix that?As Masahiro-san already said, the root cause is that the planner removes the
RelabelType that is on the indexed variable. So ruleutils sees that the operator is not
the one that would be chosen by the parser given those input expressions (cf
generate_operator_name) and it concludes it'd better schema-qualify the operator.
While that doesn't really make any difference in this particular case, it is the right thing
in typical rule-deparsing cases.
Yes.
The plan of index scan has a "RELABELTYPE" node, and it has "resulttype" so that
generate_operator_name() gets "operid =1054("="), arg1=1042(bpchar from resulttype),
arg2=1042(bpchar)". The plan of index only scan is removed it so that
generate_operator_name() gets "operid =1054("="), arg1=25(*text*), arg2=1042(bpchar)".
Though there is no entry "operid=1054("="), arg1=25(*text*), arg2=1042(bpchar)" in
pg_operator, it decided to check with the operator "=" for (text, text) because it is
coercion-compatible and preferred than operator "=" for (bpchar, bpchar).
But since the caller expected to use operator "=" for (bpchar, bpchar), it plus
OPERATOR() decoration sadly.
# the partial output of Index Scan plan
:indexqualorig (
{OPEXPR
:opno 1054 -- "=" operator
:opfuncid 1048
:opresulttype 16
:opretset false
:opcollid 0
:inputcollid 950
:args (
{RELABELTYPE
:arg
{VAR
:varno 1
:varattno 1
:vartype 25 -- text. But it will be evaluated as 1042(bpchar) from resulttype
:vartypmod -1
:varcollid 950
:varnullingrels (b)
:varlevelsup 0
:varnosyn 1
:varattnosyn 1
:location 33
}
:resulttype 1042 -- bpchar
:resulttypmod -1
:resultcollid 950
:relabelformat 1
:location 35
}
{CONST
:consttype 1042 -- bpchar
:consttypmod -1
:constcollid 100
:constlen -1
:constbyval false
:constisnull false
:location 46
:constvalue 7 [ 28 0 0 0 102 111 111 ]
}
)
# the partial output of Index Only Scan plan
:indexqual (
{OPEXPR
:opno 1054 -- "=" operator
:opfuncid 1048
:opresulttype 16
:opretset false
:opcollid 0
:inputcollid 950
:args (
{VAR
:varno -3
:varattno 1
:vartype 25 -- text
:vartypmod -1
:varcollid 950
:varnullingrels (b)
:varlevelsup 0
:varnosyn 1
:varattnosyn 1
:location 33
}
{CONST
:consttype 1042 -- bpchar
:consttypmod -1
:constcollid 100
:constlen -1
:constbyval false
:constisnull false
:location 46
:constvalue 7 [ 28 0 0 0 102 111 111 ]
}
)
:location 44
}
)
I don't think this output is really wrong, and I'm not in favor of adding nontrivial overhead
to make it better, so I don't like the proposed patch. Maybe generate_operator_name
could use some other heuristic, but I'm unsure what. The case it wants to cover is that
the operator is in the search path but is masked by some operator in an earlier schema,
so simply testing if the operator's schema is in the path at all would be insufficient.
Yes, that's one of my concerns.
IIUC, the above case is not related to the search path but the arguments don't match. If so,
I think there are two ways to solve the issue.
1. make callers of generate_operator_name() check the arguments first.
The callers (e.g., get_oper_expr()) of generate_operator_name() decide whether they
should/can cast the arguments. The planner already decided what operator should be used
so that get_oper_expr() can cast always on the explain context if the arguments don't match
the operator's one, doesn't it?
2. make generate_operator_name() checks all operator candidate.
Currently generate_operator_name() checks only one operator which matches the operator's name
even if although there are other candidates (e.g., to call oper()->oper_select_candidate()).
func_select_candidate() selects operator "=" for (text, text) in the above case because "=" for
(btchar, btchar) is typispreferred=false. So, it seems to me that it can solve the issue
if generate_operator_name() checks all candidates.
I think the second solution is better because callers might expect to use specified operator
even if there are other candidates' operator which can handle the arguments.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION