Re: POC, WIP: OR-clause support for indexes
On 20.11.2023 11:52, Andrei Lepikhov wrote: On 10/11/2023 16:20, Alena Rybakina wrote: I added log like that: ERROR: unrecognized node type: 0. I fixed this issue and added some cosmetic refactoring. The changes are presented in the or_patch_changes.diff file. Looking into the patch, I found some trivial improvements (see attachment). Also, it is not obvious that using a string representation of the clause as a hash table key is needed here. Also, by making a copy of the node in the get_key_nconst_node(), you replace the location field, but in the case of complex expression, you don't do the same with other nodes. I propose to generate expression hash instead + prove the equality of two expressions by calling equal(). Thank you! I agree with your changes.
Re: POC, WIP: OR-clause support for indexes
Hi, all!
These days I was porting a patch for converting or expressions to ANY to
the choose_bitmap_and function. Unfortunately, it is not possible to
transfer the conversion there, since expressions are processed one by
one, as far as I saw. Therefore, I tried to make the conversion earlier
in the generate_bitmap_or_paths function, there is just a loop bypass.
The patch turns out to be quite complicated, in my opinion, and to be
honest, it does not work fully yet. Also, due to the fact that the index
for the ScalarOpExpr expression is created earlier (approximately 344
lines in the src/backend/optimizer/path/indxpath.c file), we had to call
the generate_bitmap_or_paths function earlier. I haven't seen yet what
problems this could potentially lead to. Patch in the attached diff file.
In the last letter, I had an incorrect numbering for the original patch,
corrected, respectively, it is unclear whether the tests in CI were
normal. Corrected it.
From 3062a2408af258b9e327219020221b75501e8530 Mon Sep 17 00:00:00 2001
From: Alena Rybakina
Date: Mon, 6 Nov 2023 16:48:00 +0300
Subject: [PATCH] Replace OR clause to ANY expressions. Replace (X=N1) OR
(X=N2) ... with X = ANY(N1, N2) on the stage of the optimiser when we are
still working with a tree expression. Firstly, we do not try to make a
transformation for "non-or" expressions or inequalities and the creation of a
relation with "or" expressions occurs according to the same scenario.
Secondly, we do not make transformations if there are less than set
or_transform_limit. Thirdly, it is worth considering that we consider "or"
expressions only at the current level.
Authors: Alena Rybakina , Andrey Lepikhov
Reviewed-by: Peter Geoghegan , Ranier Vilela
Reviewed-by: Alexander Korotkov , Robert Haas
---
src/backend/parser/parse_expr.c | 299 +-
src/backend/utils/misc/guc_tables.c | 10 +
src/include/parser/parse_expr.h | 1 +
src/test/regress/expected/create_index.out| 115 +++
src/test/regress/expected/guc.out | 3 +-
src/test/regress/expected/join.out| 50 +++
src/test/regress/expected/partition_prune.out | 179 +++
src/test/regress/expected/tidscan.out | 17 +
src/test/regress/sql/create_index.sql | 32 ++
src/test/regress/sql/join.sql | 10 +
src/test/regress/sql/partition_prune.sql | 22 ++
src/test/regress/sql/tidscan.sql | 6 +
src/tools/pgindent/typedefs.list | 1 +
13 files changed, 743 insertions(+), 2 deletions(-)
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 64c582c344c..25a4235dbd9 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -18,6 +18,7 @@
#include "catalog/pg_aggregate.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+#include "common/hashfn.h"
#include "commands/dbcommands.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
@@ -43,6 +44,7 @@
/* GUC parameters */
bool Transform_null_equals = false;
+bool or_transform_limit = false;
static Node *transformExprRecurse(ParseState *pstate, Node *expr);
@@ -98,7 +100,302 @@ static Expr *make_distinct_op(ParseState *pstate, List *opname,
Node *ltree, Node *rtree, int location);
static Node *make_nulltest_from_distinct(ParseState *pstate,
A_Expr *distincta, Node *arg);
+typedef struct OrClauseGroupEntry
+{
+ char *hash_leftvar_key;
+
+ Node *node;
+ List *consts;
+ Oidscalar_type;
+ Oidopno;
+ Expr *expr;
+} OrClauseGroupEntry;
+
+static int
+or_name_match(const void *key1, const void *key2, Size keysize)
+{
+ const char *name1 = *(const char *const *) key1;
+ const char *name2 = *(const char *const *) key2;
+
+ return strcmp(name1, name2);
+}
+
+static uint32
+or_name_hash(const void *key, Size keysize)
+{
+ const char *name = *(const char *const *) key;
+
+ return DatumGetInt32(hash_any((unsigned char *)name, strlen(name)));
+}
+
+static char *
+get_key_nconst_node(Node *nconst_node)
+{
+ if (IsA(nconst_node, OpExpr))
+ {
+ OpExpr *clause = (OpExpr*) nconst_node;
+ OpExpr *temp = makeNode(OpExpr);
+
+ temp->opno = clause->opno;
+ temp->opfuncid = InvalidOid;
+ temp->opresulttype = clause->opresulttype;
+ temp->opretset = clause->opretset;
+ temp->opcollid = clause->opcollid;
+ temp->inputcollid = clause->inputcollid;
+ temp->location = -1;
+
+ temp->args = list_copy(clause->args);
+ return nodeToString(temp);
+ }
+ else if (IsA(nconst_node, Var))
+ {
+ Var *clause = (Var*) nconst_node;
+ Var *var = makeNode(Var);
+
+ var->varno = clause->varno;
+ var->varattno = clause->varattno;
+ var->vartype = clause->vartype;
+ var->vartypmod = clause->vartypmod;
+ var->varcollid = clause->varcollid;
+ var->varlevelsup = clause->varlevelsup;
+ var->varattnosyn = clause->varattno;
+ var->location = -1;
+
+ return nodeToString(var);
+
Re: Not deleted mentions of the cleared path
Hi! Thank you for the interest to this issue.
On 31.10.2023 06:25, Richard Guo wrote:
On Mon, Oct 30, 2023 at 7:31 PM Alena Rybakina
wrote:
I have already written about the problem of InvalidPath [0]
appearing. I investigated this and found an error in the add_path()
function, when we reject a path, we free up the memory of the path,
but do not delete various mentions of it (for example, in the
ancestor of relation, as in the example below).
I agree that what you observed is true - add_path() may free a path
while it's still referenced from some lower rels. For instance, when
creating ordered paths, we may use the input path unchanged without
copying if it's already well ordered, and it might be freed afterwards
if it fails when competing in add_path().
But this doesn't seem to be a problem in practice. We will not access
these references from the lower rels.
I'm not sure if this is an issue that we need to fix, or we need to live
with. But I do think it deserves some explanation in the comment of
add_path().
I agree that the code looks like an error, but without a real request,
it is still difficult to identify it as a bug. I'll try to reproduce it.
And yes, at least a comment is required here, and to be honest, I have
already faced this problem myself.
On 30.10.2023 17:36, Ashutosh Bapat wrote:
On Mon, Oct 30, 2023 at 5:01 PM Alena Rybakina
wrote:
Hi, hackers!
I have already written about the problem of InvalidPath [0] appearing. I
investigated this and found an error in the add_path() function, when we reject
a path, we free up the memory of the path, but do not delete various mentions
of it (for example, in the ancestor of relation, as in the example below).
Thus, we may face the problem of accessing the freed memory.
I demonstrated this below using gdb when I call a query after running a test in
test/regression/sql/create_misc.sql:
Query:
:-- That ALTER TABLE should have added TOAST tables.
SELECT relname, reltoastrelid <> 0 AS has_toast_table
FROM pg_class
WHERE oid::regclass IN ('a_star', 'c_star')
ORDER BY 1;
--UPDATE b_star*
-- SET a = text 'gazpacho'
-- WHERE aa > 4;
SELECT class, aa, a FROM a_star*;
gdb:
0x7ff3f7325fda in epoll_wait (epfd=5, events=0x55bf9ee75c38, maxevents=1,
timeout=-1) at ../sysdeps/unix/sysv/linux/epoll_wait.c:30
30 ../sysdeps/unix/sysv/linux/epoll_wait.c: No such file or directory.
(gdb) b /home/alena/postgrespro_3/src/backend/optimizer/util/pathnode.c:620
Breakpoint 1 at 0x55bf9cfe4c65: file pathnode.c, line 621.
(gdb) c
Continuing.
Breakpoint 1, add_path (parent_rel=0x55bf9ef7f5c0, new_path=0x55bf9ef7f4e0) at
pathnode.c:621
621 if (!IsA(new_path, IndexPath))
(gdb) n
622 pfree(new_path);
(gdb) n
624 }
(gdb) p *new_path
$1 = {type = T_Invalid, pathtype = T_Invalid, parent = 0x7f7f7f7f7f7f7f7f,
pathtarget = 0x7f7f7f7f7f7f7f7f,
param_info = 0x7f7f7f7f7f7f7f7f, parallel_aware = 127, parallel_safe = 127,
parallel_workers = 2139062143,
rows = 1.3824172084878715e+306, startup_cost = 1.3824172084878715e+306,
total_cost = 1.3824172084878715e+306,
pathkeys = 0x7f7f7f7f7f7f7f7f}
(gdb) p new_path
$2 = (Path *) 0x55bf9ef7f4e0
At this point the new_path has not been added to the parent_rel. We do
not set the cheapest* paths while paths are being added. The stack
trace will give you an idea where this is happening.
(gdb) p ((ProjectionPath *)((SortPath*)parent_rel->pathlist->elements
[0].ptr_value)->subpath)->path->parent->cheapest_startup_path
$20 = (struct Path *) 0x55bf9ef7f4e0
This looks familiar though. There was some nearby thread where Tom
Lane, if my memory serves well, provided a case where a path from
lower rel was added to an upper rel without copying or changing its
parent. This very much looks like that case.
Thank you, I think this might help me to find a query to reproduce it.
Re: POC, WIP: OR-clause support for indexes
Hi!
On 15.10.2023 01:34, Alexander Korotkov wrote:
Hi, Alena!
Thank you for your work on the subject.
On Wed, Oct 4, 2023 at 10:21 PM a.rybakina wrote:
I fixed the kernel dump issue and all the regression tests were successful, but
I discovered another problem when I added my own regression tests.
Some queries that contain "or" expressions do not convert to "ANY". I have
described this in more detail using diff as expected and real results:
diff -U3
/home/alena/postgrespro__copy6/src/test/regress/expected/create_index.out
/home/alena/postgrespro__copy6/src/test/regress/results/create_index.out
--- /home/alena/postgrespro__copy6/src/test/regress/expected/create_index.out
2023-10-04 21:54:12.496282667 +0300
+++ /home/alena/postgrespro__copy6/src/test/regress/results/create_index.out
2023-10-04 21:55:41.665422459 +0300
@@ -1925,17 +1925,20 @@
EXPLAIN (COSTS OFF)
SELECT count(*) FROM tenk1
WHERE thousand = 42 AND (tenthous = 1 OR tenthous = 3) OR thousand = 41;
- QUERY PLAN
-
+QUERY PLAN
+---
Aggregate
-> Bitmap Heap Scan on tenk1
- Recheck Cond: (((thousand = 42) AND (tenthous = ANY
('{1,3}'::integer[]))) OR (thousand = 41))
+ Recheck Cond: thousand = 42) AND (tenthous = 1)) OR ((thousand =
42) AND (tenthous = 3))) OR (thousand = 41))
-> BitmapOr
- -> Bitmap Index Scan on tenk1_thous_tenthous
- Index Cond: ((thousand = 42) AND (tenthous = ANY
('{1,3}'::integer[])))
+ -> BitmapOr
+ -> Bitmap Index Scan on tenk1_thous_tenthous
+ Index Cond: ((thousand = 42) AND (tenthous = 1))
+ -> Bitmap Index Scan on tenk1_thous_tenthous
+ Index Cond: ((thousand = 42) AND (tenthous = 3))
-> Bitmap Index Scan on tenk1_thous_tenthous
Index Cond: (thousand = 41)
-(8 rows)
+(11 rows)
I think this query is not converted, because you only convert
top-level ORs in the transform_ors() function. But in the example
given, the target OR lays under AND, which in turn lays under another
OR. I think you need to make transform_ors() recursive to handle
cases like this.
I wonder about the default value of the parameter or_transform_limit
of 500. In [1] and [2] you show the execution time degradation from 0
to ~500 OR clauses. I made a simple SQL script with the query "SELECT
* FROM pgbench_accounts a WHERE aid = 1 OR aid = 2 OR ... OR aid =
100;". The pgbench results for a single connection in prepared mode
are the following.
master: 936 tps
patched (or_transform_limit == 0) :1414 tps
So, transformation to ANY obviously accelerates the execution.
I think it's important to identify the cases where this patch causes
the degradation. Generally, I don't see why ANY could be executed
slower than the equivalent OR clause. So, the possible degradation
cases are slower plan generation and worse plans. I managed to find
both.
As you stated before, currently the OR transformation has a quadratic
complexity depending on the number of or-clause-groups. I made a
simple test to evaluate this. containing 1 or-clause-groups.
SELECT * FROM pgbench_accounts a WHERE aid + 1 * bid = 1 OR aid + 2 *
bid = 1 OR ... OR aid + 1 * bid = 1;
master: 316ms
patched: 7142ms
Note, that the current or_transform_limit GUC parameter is not capable
of cutting such cases, because it cuts cases lower than the limit not
higher than the limit. In the comment, you mention that we could
invent something like hash to handle this. Hash should be nice, but
the problem is that we currently don't have a generic facility to hash
nodes (or even order them). It would be nice to add this facility,
that would be quite a piece of work. I would propose to limit this
patch for now to handle just a single Var node as a non-const side of
the clause and implement a simple hash for Vars.
Another problem is the possible generation of worse plans. I made an
example table with two partial indexes.
create table test as (select (random()*10)::int x, (random()*1000) y
from generate_series(1,100) i);
create index test_x_1_y on test (y) where x = 1;
create index test_x_2_y on test (y) where x = 2;
vacuum analyze test;
Without the transformation of ORs to ANY, our planner manages to use
both indexes with a Bitmap scan.
# explain select * from test where (x = 1 or x = 2) and y = 100;
QUERY PLAN
--
Bitma
Re: POC, WIP: OR-clause support for indexes
Hi! Thank you for your review!
On 15.10.2023 01:34, Alexander Korotkov wrote:
Hi, Alena!
Thank you for your work on the subject.
On Wed, Oct 4, 2023 at 10:21 PM a.rybakina wrote:
I fixed the kernel dump issue and all the regression tests were successful, but
I discovered another problem when I added my own regression tests.
Some queries that contain "or" expressions do not convert to "ANY". I have
described this in more detail using diff as expected and real results:
diff -U3
/home/alena/postgrespro__copy6/src/test/regress/expected/create_index.out
/home/alena/postgrespro__copy6/src/test/regress/results/create_index.out
--- /home/alena/postgrespro__copy6/src/test/regress/expected/create_index.out
2023-10-04 21:54:12.496282667 +0300
+++ /home/alena/postgrespro__copy6/src/test/regress/results/create_index.out
2023-10-04 21:55:41.665422459 +0300
@@ -1925,17 +1925,20 @@
EXPLAIN (COSTS OFF)
SELECT count(*) FROM tenk1
WHERE thousand = 42 AND (tenthous = 1 OR tenthous = 3) OR thousand = 41;
- QUERY PLAN
-
+QUERY PLAN
+---
Aggregate
-> Bitmap Heap Scan on tenk1
- Recheck Cond: (((thousand = 42) AND (tenthous = ANY
('{1,3}'::integer[]))) OR (thousand = 41))
+ Recheck Cond: thousand = 42) AND (tenthous = 1)) OR ((thousand =
42) AND (tenthous = 3))) OR (thousand = 41))
-> BitmapOr
- -> Bitmap Index Scan on tenk1_thous_tenthous
- Index Cond: ((thousand = 42) AND (tenthous = ANY
('{1,3}'::integer[])))
+ -> BitmapOr
+ -> Bitmap Index Scan on tenk1_thous_tenthous
+ Index Cond: ((thousand = 42) AND (tenthous = 1))
+ -> Bitmap Index Scan on tenk1_thous_tenthous
+ Index Cond: ((thousand = 42) AND (tenthous = 3))
-> Bitmap Index Scan on tenk1_thous_tenthous
Index Cond: (thousand = 41)
-(8 rows)
+(11 rows)
I think this query is not converted, because you only convert
top-level ORs in the transform_ors() function. But in the example
given, the target OR lays under AND, which in turn lays under another
OR. I think you need to make transform_ors() recursive to handle
cases like this.
Yes, you are right, it seems that a recursive method is needed here.
I wonder about the default value of the parameter or_transform_limit
of 500. In [1] and [2] you show the execution time degradation from 0
to ~500 OR clauses. I made a simple SQL script with the query "SELECT
* FROM pgbench_accounts a WHERE aid = 1 OR aid = 2 OR ... OR aid =
100;". The pgbench results for a single connection in prepared mode
are the following.
master: 936 tps
patched (or_transform_limit == 0) :1414 tps
So, transformation to ANY obviously accelerates the execution.
I think it's important to identify the cases where this patch causes
the degradation. Generally, I don't see why ANY could be executed
slower than the equivalent OR clause. So, the possible degradation
cases are slower plan generation and worse plans. I managed to find
both.
As you stated before, currently the OR transformation has a quadratic
complexity depending on the number of or-clause-groups. I made a
simple test to evaluate this. containing 1 or-clause-groups.
SELECT * FROM pgbench_accounts a WHERE aid + 1 * bid = 1 OR aid + 2 *
bid = 1 OR ... OR aid + 1 * bid = 1;
master: 316ms
patched: 7142ms
Note, that the current or_transform_limit GUC parameter is not capable
of cutting such cases, because it cuts cases lower than the limit not
higher than the limit. In the comment, you mention that we could
invent something like hash to handle this. Hash should be nice, but
the problem is that we currently don't have a generic facility to hash
nodes (or even order them). It would be nice to add this facility,
that would be quite a piece of work. I would propose to limit this
patch for now to handle just a single Var node as a non-const side of
the clause and implement a simple hash for Vars.
I ran the query and saw that you were right, this place in the patch
turns out to be very expensive. In addition to the hash, I saw a second
solution to this problem - parameterize constants and store them in the
list, but this will not be such a universal solution as hashing. If the
variable, not the constant, changes, parameterization will not help.
I agree with your suggestion to try adding hashing. I'll take a closer
look at this.
Another problem is the possible generation of worse plans. I made an
example table with two partial indexes.
create table test as (
Re: Removing unneeded self joins
On 13.10.2023 12:03, Andrei Lepikhov wrote:
On 13/10/2023 15:56, a.rybakina wrote:
Also I've incorporated improvements from Alena Rybakina except one for
skipping SJ removal when no SJ quals is found. It's not yet clear for
me if this check fix some cases. But at least optimization got skipped
in some useful cases (as you can see in regression tests).
Agree. I wouldn't say I like it too. But also, I suggest skipping
some unnecessary assertions proposed in that patch:
Assert(toKeep->relid != -1); - quite strange. Why -1? Why not all
the negative numbers, at least?
Assert(is_opclause(orinfo->clause)); - above we skip clauses with
rinfo->mergeopfamilies == NIL. Each mergejoinable clause is already
checked as is_opclause.
All these changes (see in the attachment) are optional.
I don't mind about asserts, maybe I misunderstood something in the
patch.
About skipping SJ removal when no SJ quals is found, I assume it is
about it:
split_selfjoin_quals(root, restrictlist, ,
, inner->relid,
outer->relid);
+ if (list_length(selfjoinquals) == 0)
+ {
+ /*
+ * XXX:
+ * we would detect self-join without quals like
'x==x' if we had
+ * an foreign key constraint on some of other quals
and this join
+ * haven't any columns from the outer in the target
list.
+ * But it is still complex task.
+ */
+ continue;
+ }
as far as I remember, this is the place where it is checked that the
SJ list is empty and it is logical, in my opinion, that no
transformations should be performed if no elements are found for them.
You forget we have "Degenerate" case, as Alexander mentioned above.
What if you have something like that:
SELECT ... FROM A a1, A a2 WHERE a1.id=1 AND a2.id=1;
In this case, uniqueness can be achieved by the baserestrictinfo
"A.id=1", if we have an unique index on this column.
Yes, sorry, I missed it. thanks again for the explanation
Re: Removing unneeded self joins
Also I've incorporated improvements from Alena Rybakina except one for
skipping SJ removal when no SJ quals is found. It's not yet clear for
me if this check fix some cases. But at least optimization got skipped
in some useful cases (as you can see in regression tests).
Agree. I wouldn't say I like it too. But also, I suggest skipping some
unnecessary assertions proposed in that patch:
Assert(toKeep->relid != -1); - quite strange. Why -1? Why not all the
negative numbers, at least?
Assert(is_opclause(orinfo->clause)); - above we skip clauses with
rinfo->mergeopfamilies == NIL. Each mergejoinable clause is already
checked as is_opclause.
All these changes (see in the attachment) are optional.
I don't mind about asserts, maybe I misunderstood something in the patch.
About skipping SJ removal when no SJ quals is found, I assume it is
about it:
split_selfjoin_quals(root, restrictlist, ,
, inner->relid,
outer->relid);
+ if (list_length(selfjoinquals) == 0)
+ {
+ /*
+ * XXX:
+ * we would detect self-join without quals like 'x==x'
if we had
+ * an foreign key constraint on some of other quals
and this join
+ * haven't any columns from the outer in the target list.
+ * But it is still complex task.
+ */
+ continue;
+ }
as far as I remember, this is the place where it is checked that the SJ
list is empty and it is logical, in my opinion, that no transformations
should be performed if no elements are found for them.
As for the cases where SJ did not work, maybe this is just right if
there are no elements for processing these cases. I'll try to check or
come up with tests for them. If I'm wrong, write.
On 11.10.2023 06:51, Andrei Lepikhov wrote:
On 11/10/2023 02:29, Alena Rybakina wrote:
I have reviewed your patch and I noticed a few things.
Thanks for your efforts,
I have looked at the latest version of the code, I assume that the
error lies in the replace_varno_walker function, especially in the
place where we check the node by type Var, and does not form any
NullTest node.
It's not a bug, it's an optimization we discussed with Alexander above.
Secondly, I added some code in some places to catch erroneous cases
and added a condition when we should not try to apply the
self-join-removal transformation due to the absence of an empty
self-join list after searching for it and in general if there are no
joins in the query. Besides, I added a query for testing and wrote
about it above. I have attached my diff file.
Ok, I will look at this
In addition, I found a comment for myself that was not clear to me. I
would be glad if you could explain it to me.
You mentioned superior outer join in the comment, unfortunately, I
didn't find anything about it in the PostgreSQL code, and this
explanation remained unclear to me. Could you explain in more detail
what you meant?
I meant here that only clauses pushed by
reconsider_outer_join_clauses() can be found in the joininfo list, and
they are not relevant, as you can understand.
Having written that, I realized that it was a false statement. ;) -
joininfo can also contain results of previous SJE iterations, look:
CREATE TABLE test (oid int PRIMARY KEY);
CREATE UNIQUE INDEX ON test((oid*oid));
explain
SELECT count(*)
FROM test c1, test c2, test c3
WHERE c1.oid=c2.oid AND c1.oid*c2.oid=c3.oid*c3.oid;
explain
SELECT count(*)
FROM test c1, test c2, test c3
WHERE c1.oid=c3.oid AND c1.oid*c3.oid=c2.oid*c2.oid;
explain
SELECT count(*)
FROM test c1, test c2, test c3
WHERE c3.oid=c2.oid AND c3.oid*c2.oid=c1.oid*c1.oid;
Ok, I understood. Thank you for explanation.
--
Regards,
Alena Rybakina
Re: POC, WIP: OR-clause support for indexes
On 29.09.2023 20:35, a.rybakina wrote:
I'm sorry I didn't write for a long time, but I really had a very
difficult month, now I'm fully back to work.
*I was able to implement the patches to the end and moved the
transformation of "OR" expressions to ANY.* I haven't seen a big
difference between them yet, one has a transformation before
calculating selectivity (v7.1-Replace-OR-clause-to-ANY.patch), the
other after (v7.2-Replace-OR-clause-to-ANY.patch). Regression tests
are passing, I don't see any problems with selectivity, nothing has
fallen into the coredump, but I found some incorrect transformations.
What is the reason for these inaccuracies, I have not found, but, to
be honest, they look unusual). Gave the error below.
In the patch, I don't like that I had to drag three libraries from
parsing until I found a way around it.The advantage of this approach
compared to the other ([1]) is that at this stage all possible or
transformations are performed, compared to the patch, where the
transformation was done at the parsing stage. That is, here, for
example, there are such optimizations in the transformation:
I took the common element out of the bracket and the rest is
converted to ANY, while, as noted by Peter Geoghegan, we did not have
several bitmapscans, but only one scan through the array.
postgres=# explain analyze SELECT p1.oid, p1.proname
FROM pg_proc as p1
WHERE prolang = 13 AND prolang=1 OR prolang = 13 AND prolang = 2 OR
prolang = 13 AND prolang = 3;
QUERY PLAN
---
Seq Scan on pg_proc p1 (cost=0.00..151.66 rows=1 width=68) (actual
time=1.167..1.168 rows=0 loops=1)
Filter: ((prolang = '13'::oid) AND (prolang = ANY (ARRAY['1'::oid,
'2'::oid, '3'::oid])))
Rows Removed by Filter: 3302
Planning Time: 0.146 ms
Execution Time: 1.191 ms
(5 rows)
*Falls into coredump at me:*
explain analyze SELECT p1.oid, p1.proname
FROM pg_proc as p1
WHERE prolang = 13 OR prolang = 2 AND prolang = 2 OR prolang = 13;
Hi, all!
I fixed the kernel dump issue and all the regression tests were
successful, but I discovered another problem when I added my own
regression tests.
Some queries that contain "or" expressions do not convert to "ANY". I
have described this in more detail using diff as expected and real results:
diff -U3
/home/alena/postgrespro__copy6/src/test/regress/expected/create_index.out
/home/alena/postgrespro__copy6/src/test/regress/results/create_index.out
---
/home/alena/postgrespro__copy6/src/test/regress/expected/create_index.out
2023-10-04 21:54:12.496282667 +0300
+++
/home/alena/postgrespro__copy6/src/test/regress/results/create_index.out
2023-10-04 21:55:41.665422459 +0300
@@ -1925,17 +1925,20 @@
EXPLAIN (COSTS OFF)
SELECT count(*) FROM tenk1
WHERE thousand = 42 AND (tenthous = 1 OR tenthous = 3) OR thousand = 41;
- QUERY PLAN
-
+ QUERY PLAN
+---
Aggregate
-> Bitmap Heap Scan on tenk1
- Recheck Cond: (((thousand = 42) AND (tenthous = ANY
('{1,3}'::integer[]))) OR (thousand = 41))
+ Recheck Cond: thousand = 42) AND (tenthous = 1)) OR
((thousand = 42) AND (tenthous = 3))) OR (thousand = 41))
-> BitmapOr
- -> Bitmap Index Scan on tenk1_thous_tenthous
- Index Cond: ((thousand = 42) AND (tenthous = ANY
('{1,3}'::integer[])))
+ -> BitmapOr
+ -> Bitmap Index Scan on tenk1_thous_tenthous
+ Index Cond: ((thousand = 42) AND (tenthous = 1))
+ -> Bitmap Index Scan on tenk1_thous_tenthous
+ Index Cond: ((thousand = 42) AND (tenthous = 3))
-> Bitmap Index Scan on tenk1_thous_tenthous
Index Cond: (thousand = 41)
-(8 rows)
+(11 rows)
@@ -1946,24 +1949,50 @@
EXPLAIN (COSTS OFF)
SELECT count(*) FROM tenk1
+ WHERE thousand = 42 OR tenthous = 1 AND thousand = 42 OR tenthous = 1;
+ QUERY PLAN
+---
+ Aggregate
+ -> Bitmap Heap Scan on tenk1
+ Recheck Cond: ((thousand = 42) OR ((thousand = 42) AND
(tenthous = 1)) OR (tenthous = 1))
+ -> BitmapOr
+ -> Bitmap Index Scan on tenk1_thous_tenthous
+ Index Cond: (thousand = 42)
+ -> Bitmap Index Scan on tenk1_thous_tenthous
+ Index Cond: ((thou
Re: POC, WIP: OR-clause support for indexes
I'm sorry I didn't write for a long time, but I really had a very difficult month, now I'm fully back to work. *I was able to implement the patches to the end and moved the transformation of "OR" expressions to ANY.* I haven't seen a big difference between them yet, one has a transformation before calculating selectivity (v7.1-Replace-OR-clause-to-ANY.patch), the other after (v7.2-Replace-OR-clause-to-ANY.patch). Regression tests are passing, I don't see any problems with selectivity, nothing has fallen into the coredump, but I found some incorrect transformations. What is the reason for these inaccuracies, I have not found, but, to be honest, they look unusual). Gave the error below. In the patch, I don't like that I had to drag three libraries from parsing until I found a way around it.The advantage of this approach compared to the other ([1]) is that at this stage all possible or transformations are performed, compared to the patch, where the transformation was done at the parsing stage. That is, here, for example, there are such optimizations in the transformation: I took the common element out of the bracket and the rest is converted to ANY, while, as noted by Peter Geoghegan, we did not have several bitmapscans, but only one scan through the array. postgres=# explain analyze SELECT p1.oid, p1.proname FROM pg_proc as p1 WHERE prolang = 13 AND prolang=1 OR prolang = 13 AND prolang = 2 OR prolang = 13 AND prolang = 3; QUERY PLAN --- Seq Scan on pg_proc p1 (cost=0.00..151.66 rows=1 width=68) (actual time=1.167..1.168 rows=0 loops=1) Filter: ((prolang = '13'::oid) AND (prolang = ANY (ARRAY['1'::oid, '2'::oid, '3'::oid]))) Rows Removed by Filter: 3302 Planning Time: 0.146 ms Execution Time: 1.191 ms (5 rows) *Falls into coredump at me:* explain analyze SELECT p1.oid, p1.proname FROM pg_proc as p1 WHERE prolang = 13 OR prolang = 2 AND prolang = 2 OR prolang = 13; I continue to try to move transformations of "OR" expressions at the optimization stage, unfortunately I have not been able to figure out coredump yet, but I saw an important thing that it is already necessary to process RestrictInfo expressions here. I corrected it. To be honest, despite some significant advantages in the fact that we are already processing pre-converted "or" expressions (logical transformations have been performed and duplicates have been removed), I have big doubts about this approach. We already have quite a lot of objects at this stage that can refer to the RestrictInfo variable in ReplOptInfo, and updating these links can be costly for us. By the way, right now I suspect that the current coredump appeared precisely because there is a link somewhere that refers to an un-updated RestrictInfo, but so far I can't find this place. coredump occurs at the request execution stage, looks like this: Core was generated by `postgres: alena regression [local] SELECT '. --Type for more, q to quit, c to continue without paging-- Program terminated with signal SIGSEGV, Segmentation fault. #0 0x5565f3ec4947 in ExecInitExprRec (node=0x5565f530b290, state=0x5565f53383d8, resv=0x5565f53383e0, resnull=0x5565f53383dd) at execExpr.c:1331 1331 Expr *arg = (Expr *) lfirst(lc); (gdb) bt #0 0x5565f3ec4947 in ExecInitExprRec (node=0x5565f530b290, state=0x5565f53383d8, resv=0x5565f53383e0, resnull=0x5565f53383dd) at execExpr.c:1331 #1 0x5565f3ec2708 in ExecInitQual (qual=0x5565f531d950, parent=0x5565f5337948) at execExpr.c:258 #2 0x5565f3f2f080 in ExecInitSeqScan (node=0x5565f5309700, estate=0x5565f5337700, eflags=32) at nodeSeqscan.c:172 #3 0x5565f3ee70c9 in ExecInitNode (node=0x5565f5309700, estate=0x5565f5337700, eflags=32) at execProcnode.c:210 #4 0x5565f3edbe3a in InitPlan (queryDesc=0x5565f53372f0, eflags=32) at execMain.c:968 #5 0x5565f3edabe3 in standard_ExecutorStart (queryDesc=0x5565f53372f0, eflags=32) at execMain.c:266 #6 0x5565f3eda927 in ExecutorStart (queryDesc=0x5565f53372f0, eflags=0) at execMain.c:145 #7 0x5565f419921e in PortalStart (portal=0x5565f52ace90, params=0x0, eflags=0, snapshot=0x0) at pquery.c:517 #8 0x5565f4192635 in exec_simple_query ( query_string=0x5565f5233af0 "SELECT p1.oid, p1.proname\nFROM pg_proc as p1\nWHERE prolang = 13 AND (probin IS NULL OR probin = '' OR probin = '-');") at postgres.c:1233 #9 0x5565f41976ef in PostgresMain (dbname=0x5565f526ad10 "regression", username=0x5565f526acf8 "alena") at postgres.c:4652 #10 0x5565f40b8417 in BackendRun (port=0x5565f525f830) at postmaster.c:4439 #11 0x5565f40b7ca3 in BackendStartup (port=0x5565f525f830) at postmaster.c:4167 #12 0x5565f40b40f1 in ServerLoop () at postmaster.c:1781 #13
Re: POC, WIP: OR-clause support for indexes
Sorry for the duplicates, I received a letter that my letter did not reach the addressee, I thought the design was incorrect. On 26.09.2023 12:21, a.rybakina wrote: I'm sorry I didn't write for a long time, but I really had a very difficult month, now I'm fully back to work. *I was able to implement the patches to the end and moved the transformation of "OR" expressions to ANY.* I haven't seen a big difference between them yet, one has a transformation before calculating selectivity (v7.1-Replace-OR-clause-to-ANY.patch), the other after (v7.2-Replace-OR-clause-to-ANY.patch). Regression tests are passing, I don't see any problems with selectivity, nothing has fallen into the coredump, but I found some incorrect transformations. What is the reason for these inaccuracies, I have not found, but, to be honest, they look unusual). Gave the error below. In the patch, I don't like that I had to drag three libraries from parsing until I found a way around it.The advantage of this approach compared to the other ([1]) is that at this stage all possible or transformations are performed, compared to the patch, where the transformation was done at the parsing stage. That is, here, for example, there are such optimizations in the transformation: I took the common element out of the bracket and the rest is converted to ANY, while, as noted by Peter Geoghegan, we did not have several bitmapscans, but only one scan through the array. postgres=# explain analyze SELECT p1.oid, p1.proname FROM pg_proc as p1 WHERE prolang = 13 AND prolang=1 OR prolang = 13 AND prolang = 2 OR prolang = 13 AND prolang = 3; QUERY PLAN --- Seq Scan on pg_proc p1 (cost=0.00..151.66 rows=1 width=68) (actual time=1.167..1.168 rows=0 loops=1) Filter: ((prolang = '13'::oid) AND (prolang = ANY (ARRAY['1'::oid, '2'::oid, '3'::oid]))) Rows Removed by Filter: 3302 Planning Time: 0.146 ms Execution Time: 1.191 ms (5 rows) *While I was testing, I found some transformations that don't work, although in my opinion, they should:** ** **1. First case:* explain analyze SELECT p1.oid, p1.proname FROM pg_proc as p1 WHERE prolang = 13 AND prolang=1 OR prolang = 2 AND prolang = 2 OR prolang = 13 AND prolang = 13; QUERY PLAN -- Seq Scan on pg_proc p1 (cost=0.00..180.55 rows=2 width=68) (actual time=2.959..3.335 rows=89 loops=1) Filter: (((prolang = '13'::oid) AND (prolang = '1'::oid)) OR ((prolang = '2'::oid) AND (prolang = '2'::oid)) OR ((prolang = '13'::oid) AND (prolang = '13'::oid))) Rows Removed by Filter: 3213 Planning Time: 1.278 ms Execution Time: 3.486 ms (5 rows) Should have left only prolang = '13'::oid: QUERY PLAN --- Seq Scan on pg_proc p1 (cost=0.00..139.28 rows=1 width=68) (actual time=2.034..2.034 rows=0 loops=1) Filter: ((prolang = '13'::oid )) Rows Removed by Filter: 3302 Planning Time: 0.181 ms Execution Time: 2.079 ms (5 rows) *2. Also does not work:* postgres=# explain analyze SELECT p1.oid, p1.proname FROM pg_proc as p1 WHERE prolang = 13 OR prolang = 2 AND prolang = 2 OR prolang = 13; QUERY PLAN --- Seq Scan on pg_proc p1 (cost=0.00..164.04 rows=176 width=68) (actual time=2.422..2.686 rows=89 loops=1) Filter: ((prolang = '13'::oid) OR ((prolang = '2'::oid) AND (prolang = '2'::oid)) OR (prolang = '13'::oid)) Rows Removed by Filter: 3213 Planning Time: 1.370 ms Execution Time: 2.799 ms (5 rows) Should have left: Filter: ((prolang = '13'::oid) OR (prolang = '2'::oid)) *3. Or another:* explain analyze SELECT p1.oid, p1.proname FROM pg_proc as p1 WHERE prolang = 13 OR prolang=13 OR prolang = 2 AND prolang = 2; QUERY PLAN --- Seq Scan on pg_proc p1 (cost=0.00..164.04 rows=176 width=68) (actual time=2.350..2.566 rows=89 loops=1) Filter: ((prolang = '13'::oid) OR (prolang = '13'::oid) OR ((prolang = '2'::oid) AND (prolang = '2'::oid))) Rows Removed by Filter: 3213 Planning Time: 0.215 ms Execution Time: 2.624 ms (5 rows) Should have left: Filter: ((prolang = '13'::oid) OR (prolang = '2'::oid)) *Falls into coredump at me:* explain analyze SELECT p1.oid, p1.proname FROM pg_proc as p1 WHERE prolang = 13 OR prolang = 2 AND prolang = 2 OR prolang = 13; explain analyze SELECT
Re: POC, WIP: OR-clause support for indexes
Hi! When I sent the patch version to commitfest, I thought that the work on this topic was completed. Patch version and test results in [0]. But in the process of discussing this patch, we found out that there is another place where you can make a transformation, specifically, during the calculation of selectivity. I implemented the raw version [1], but unfortunately it didn't work in regression tests. I'm sorry that I didn't write about the status earlier, I was very overwhelmed with tasks at work due to releases and preparations for the conference. I returned to the work of this patch, today or tomorrow I'll drop the version. [0] https://www.postgresql.org/message-id/4bac271d-1700-db24-74ac-8414f2baf9fd%40postgrespro.ru https://www.postgresql.org/message-id/11403645-b342-c400-859e-47d0f41ec22a%40postgrespro.ru [1] https://www.postgresql.org/message-id/b301dce1-09fd-72b1-834a-527ca428db5e%40yandex.ru On 20.09.2023 12:37, Peter Eisentraut wrote: On 29.08.23 05:37, a.rybakina wrote: Thank you for your interest in this problem and help, and I'm sorry that I didn't respond to this email for a long time. To be honest, I wanted to investigate the problems in more detail and already answer more clearly, but unfortunately I have not found anything more significant yet. What is the status of this patch? It is registered in the commitfest. It looks like a stalled research project? The last posted patch doesn't contain any description or tests, so it doesn't look very ready.
Re: POC, WIP: OR-clause support for indexes
Thank you for your interest in this problem and help, and I'm sorry that I didn't respond to this email for a long time. To be honest, I wanted to investigate the problems in more detail and already answer more clearly, but unfortunately I have not found anything more significant yet. On 21.08.2023 01:26, Peter Geoghegan wrote: There was actually support for OR lists in index AMs prior to ScalarArrayOpExpr. Even though ScalarArrayOpExpr don't really seem all that related to bitmap scans these days (since at least nbtree knows how to execute them "natively"), that wasn't always the case. ScalarArrayOpExpr were invented the same year that bitmap index scans were first added (2005), and seem more or less related to that work. See commits bc843d39, 5b051852, 1e9a6ba5, and 290166f9 (all from 2005). Particularly the last one, which has a commit message that heavily suggests that my interpretation is correct. Back in 2003, commit 9888192f removed (or at least simplified) what were then called "CNF/DNF CONVERSION ROUTINES". Prior to that point the optimizer README had something about leaving clause lists un-normalized leading to selectivity estimation problems. Bear in mind that this is a couple of years before ScalarArrayOpExpr was first invented. Apparently even back then "The OR-of-ANDs format is useful for indexscan implementation". It's possible that that old work will offer some hints on what to do now. In a way it's not surprising that work in this area would have some impact on selectivies. The surprising part is the extent of the problem, I suppose. I see that a lot of the things in this area are just used by BitmapOr clauses, such as build_paths_for_OR() -- but you're not necessarily able to use any of that stuff. Also, choose_bitmap_and() has some stuff about how it compensates to avoid "too-small selectivity that makes a redundant AND step look like it reduces the total cost". It also mentions some problems with match_join_clauses_to_index() + extract_restriction_or_clauses(). Again, this might be a good place to look for more clues. I agree with your assumption about looking at the source of the error related to selectivity in these places. But honestly, no matter how many times I looked, until enough sensible thoughts appeared, which could cause a problem. I keep looking, maybe I'll find something. EXPLAIN (COSTS OFF) SELECT * FROM tenk1 WHERE thousand = 42 AND (tenthous = 1 OR tenthous = 3 OR tenthous = 42); - QUERY PLAN -- - Bitmap Heap Scan on tenk1 - Recheck Cond: (((thousand = 42) AND (tenthous = 1)) OR ((thousand = 42) AND (tenthous = 3)) OR ((thousand = 42) AND (tenthous = 42))) - -> BitmapOr - -> Bitmap Index Scan on tenk1_thous_tenthous - Index Cond: ((thousand = 42) AND (tenthous = 1)) - -> Bitmap Index Scan on tenk1_thous_tenthous - Index Cond: ((thousand = 42) AND (tenthous = 3)) - -> Bitmap Index Scan on tenk1_thous_tenthous - Index Cond: ((thousand = 42) AND (tenthous = 42)) -(9 rows) + QUERY PLAN + + Index Scan using tenk1_thous_tenthous on tenk1 + Index Cond: ((thousand = 42) AND (tenthous = ANY (ARRAY[1, 3, 42]))) +(2 rows) I think that we currently over-rely on BitmapOr for OR clauses. It's useful that they're so general, of course, but ISTM that we shouldn't even try to use a BitmapOr in simple cases. Things like the "WHERE thousand = 42 AND (tenthous = 1 OR tenthous = 3 OR tenthous = 42)" tenk1 query that you brought up probably shouldn't even have a BitmapOr path (which I guess they don't with you patch). Note that I recently discussed the same query at length with Tomas Vondra on the ongoing thread for his index filter patch (you probably knew that already). I think so too, but it's still quite difficult to find a stable enough optimization to implement this, in my opinion. But I will finish the current optimization with OR->ANY, given that something interesting has appeared.
Re: POC, WIP: OR-clause support for indexes
Sorry, I didn't write correctly enough, about the second second place in the code where the conversion works well enough is the removal of duplicate OR expressions. I attached patch to learn it in more detail. On 17.08.2023 13:08, a.rybakina wrote: Hi, all! The optimizer will itself do a limited form of "normalizing to CNF". Are you familiar with extract_restriction_or_clauses(), from orclauses.c? Comments above the function have an example of how this can work: * Although a join clause must reference multiple relations overall, * an OR of ANDs clause might contain sub-clauses that reference just one * relation and can be used to build a restriction clause for that rel. * For example consider * WHERE ((a.x = 42 AND b.y = 43) OR (a.x = 44 AND b.z = 45)); * We can transform this into * WHERE ((a.x = 42 AND b.y = 43) OR (a.x = 44 AND b.z = 45)) * AND (a.x = 42 OR a.x = 44) * AND (b.y = 43 OR b.z = 45); * which allows the latter clauses to be applied during the scans of a and b, * perhaps as index qualifications, and in any case reducing the number of * rows arriving at the join. In essence this is a partial transformation to * CNF (AND of ORs format). It is not complete, however, because we do not * unravel the original OR --- doing so would usually bloat the qualification * expression to little gain. This is an interesting feature. I didn't notice this function before, I studied many times consider_new_or_cause, which were called there. As far as I know, there is a selectivity calculation going on there, but as far as I remember, I called it earlier after my conversion, and unfortunately it didn't solve my problem with calculating selectivity. I'll reconsider it again, maybe I can find something I missed. Of course this immediately makes me wonder: shouldn't your patch be able to perform an additional transformation here? You know, by transforming "a.x = 42 OR a.x = 44" into "a IN (42, 44)"? Although I haven't checked for myself, I assume that this doesn't happen right now, since your patch currently performs all of its transformations during parsing. I also noticed that the same comment block goes on to say something about "clauselist_selectivity's inability to recognize redundant conditions". Perhaps that is relevant to the problems you were having with selectivity estimation, back when the code was in preprocess_qual_conditions() instead? I have no reason to believe that there should be any redundancy left behind by your transformation, so this is just one possibility to consider. Separately, the commit message of commit 25a9e54d2d says something about how the planner builds RestrictInfos, which seems possibly-relevant. That commit enhanced extended statistics for OR clauses, so the relevant paragraph describes a limitation of extended statistics with OR clauses specifically. I'm just guessing, but it still seems like it might be relevant to the problem you ran into with selectivity estimation. Another possibility to consider. I understood what is said about AND clauses in this comment. It seems to me that AND clauses saved like (BoolExpr *) expr->args->(RestrictInfo *) clauseA->(RestrictInfo *)clauseB lists and OR clauses saved like (BoolExpr *) expr -> orclause->(RestrictInfo *)clause A->(RestrictInfo *)clause B. As I understand it, selectivity is calculated for each expression. But I'll exploring it deeper, because I think this place may contain the answer to the question, what's wrong with selectivity calculation in my patch. I could move transformation in there (extract_restriction_or_clauses) and didn't have any problem with selectivity calculation, besides it also works on the redundant or duplicates stage. So, it looks like: CREATE TABLE tenk1 (unique1 int, unique2 int, ten int, hundred int); insert into tenk1 SELECT x,x,x,x FROM generate_series(1,5) as x; CREATE INDEX a_idx1 ON tenk1(unique1); CREATE INDEX a_idx2 ON tenk1(unique2); CREATE INDEX a_hundred ON tenk1(hundred); explain analyze select * from tenk1 a join tenk1 b on ((a.unique2 = 3 or a.unique2 = 7)); PLAN -- Nested Loop (cost=0.29..2033.62 rows=10 width=32) (actual time=0.090..60.258 rows=10 loops=1) -> Seq Scan on tenk1 b (cost=0.00..771.00 rows=5 width=16) (actual time=0.016..9.747 rows=5 loops=1) -> Materialize (cost=0.29..12.62 rows=2 width=16) (actual time=0.000..0.000 rows=2 loops=5) -> Index Scan using a_idx2 on tenk1 a (cost=0.29..12.62 rows=2 width=16) (actual time=0.063..0.068 rows=2 loops=1) Index Cond: (unique2 = ANY (ARRAY[3, 7])) Planning Time: 8.257 ms Execution Time: 64.453 ms (7 rows) Overall, this was due to incorrectly defined types of elements in the array, and if we had applied th
Re: POC, WIP: OR-clause support for indexes
Hi, all! The optimizer will itself do a limited form of "normalizing to CNF". Are you familiar with extract_restriction_or_clauses(), from orclauses.c? Comments above the function have an example of how this can work: * Although a join clause must reference multiple relations overall, * an OR of ANDs clause might contain sub-clauses that reference just one * relation and can be used to build a restriction clause for that rel. * For example consider * WHERE ((a.x = 42 AND b.y = 43) OR (a.x = 44 AND b.z = 45)); * We can transform this into * WHERE ((a.x = 42 AND b.y = 43) OR (a.x = 44 AND b.z = 45)) * AND (a.x = 42 OR a.x = 44) * AND (b.y = 43 OR b.z = 45); * which allows the latter clauses to be applied during the scans of a and b, * perhaps as index qualifications, and in any case reducing the number of * rows arriving at the join. In essence this is a partial transformation to * CNF (AND of ORs format). It is not complete, however, because we do not * unravel the original OR --- doing so would usually bloat the qualification * expression to little gain. This is an interesting feature. I didn't notice this function before, I studied many times consider_new_or_cause, which were called there. As far as I know, there is a selectivity calculation going on there, but as far as I remember, I called it earlier after my conversion, and unfortunately it didn't solve my problem with calculating selectivity. I'll reconsider it again, maybe I can find something I missed. Of course this immediately makes me wonder: shouldn't your patch be able to perform an additional transformation here? You know, by transforming "a.x = 42 OR a.x = 44" into "a IN (42, 44)"? Although I haven't checked for myself, I assume that this doesn't happen right now, since your patch currently performs all of its transformations during parsing. I also noticed that the same comment block goes on to say something about "clauselist_selectivity's inability to recognize redundant conditions". Perhaps that is relevant to the problems you were having with selectivity estimation, back when the code was in preprocess_qual_conditions() instead? I have no reason to believe that there should be any redundancy left behind by your transformation, so this is just one possibility to consider. Separately, the commit message of commit 25a9e54d2d says something about how the planner builds RestrictInfos, which seems possibly-relevant. That commit enhanced extended statistics for OR clauses, so the relevant paragraph describes a limitation of extended statistics with OR clauses specifically. I'm just guessing, but it still seems like it might be relevant to the problem you ran into with selectivity estimation. Another possibility to consider. I understood what is said about AND clauses in this comment. It seems to me that AND clauses saved like (BoolExpr *) expr->args->(RestrictInfo *) clauseA->(RestrictInfo *)clauseB lists and OR clauses saved like (BoolExpr *) expr -> orclause->(RestrictInfo *)clause A->(RestrictInfo *)clause B. As I understand it, selectivity is calculated for each expression. But I'll exploring it deeper, because I think this place may contain the answer to the question, what's wrong with selectivity calculation in my patch. I could move transformation in there (extract_restriction_or_clauses) and didn't have any problem with selectivity calculation, besides it also works on the redundant or duplicates stage. So, it looks like: CREATE TABLE tenk1 (unique1 int, unique2 int, ten int, hundred int); insert into tenk1 SELECT x,x,x,x FROM generate_series(1,5) as x; CREATE INDEX a_idx1 ON tenk1(unique1); CREATE INDEX a_idx2 ON tenk1(unique2); CREATE INDEX a_hundred ON tenk1(hundred); explain analyze select * from tenk1 a join tenk1 b on ((a.unique2 = 3 or a.unique2 = 7)); PLAN -- Nested Loop (cost=0.29..2033.62 rows=10 width=32) (actual time=0.090..60.258 rows=10 loops=1) -> Seq Scan on tenk1 b (cost=0.00..771.00 rows=5 width=16) (actual time=0.016..9.747 rows=5 loops=1) -> Materialize (cost=0.29..12.62 rows=2 width=16) (actual time=0.000..0.000 rows=2 loops=5) -> Index Scan using a_idx2 on tenk1 a (cost=0.29..12.62 rows=2 width=16) (actual time=0.063..0.068 rows=2 loops=1) Index Cond: (unique2 = ANY (ARRAY[3, 7])) Planning Time: 8.257 ms Execution Time: 64.453 ms (7 rows) Overall, this was due to incorrectly defined types of elements in the array, and if we had applied the transformation with the definition of the tup operator, we could have avoided such problems (I used make_scalar_array_op and have not yet found an alternative to this). When I moved the transformation on the index creation stage, it couldn't work properly and as a result I faced the same problem of selectivity
Re: RFC: Logging plan of the running query
Hi,
I'm sorry,if this message is duplicated previous this one, but the
previous message is sent incorrectly. I sent it from email address
lena.riback...@yandex.ru.
I liked this idea and after reviewing code I noticed some moments and
I'd rather ask you some questions.
Firstly, I suggest some editing in the comment of commit. I think, it is
turned out the more laconic and the same clear. I wrote it below since I
can't think of any other way to add it.
```
Currently, we have to wait for finishing of the query execution to check
its plan.
This is not so convenient in investigation long-running queries on
production
environments where we cannot use debuggers.
To improve this situation there is proposed the patch containing the
pg_log_query_plan()
function which request to log plan of the specified backend process.
By default, only superusers are allowed to request log of the plan
otherwise
allowing any users to issue this request could create cause lots of log
messages
and it can lead to denial of service.
At the next requesting CHECK_FOR_INTERRUPTS(), the target backend logs
its plan at
LOG_SERVER_ONLY level and therefore this plan will appear in the server
log only,
not to be sent to the client.
```
Secondly, I have question about deleting USE_ASSERT_CHECKING in lock.h?
It supposed to have been checked in another placed of the code by
matching values. I am worry about skipping errors due to untesting with
assert option in the places where it (GetLockMethodLocalHash)
participates and we won't able to get core file in segfault cases. I
might not understand something, then can you please explain to me?
Thirdly, I have incomprehension of the point why save_ActiveQueryDesc is
declared in the pquery.h? I am seemed to save_ActiveQueryDesc to be used
in an once time in the ExecutorRun function and its declaration
superfluous. I added it in the attached patch.
Fourthly, it seems to me there are not enough explanatory comments in
the code. I also added them in the attached patch.
Lastly, I have incomprehension about handling signals since have been
unused it before. Could another signal disabled calling this signal to
log query plan? I noticed this signal to be checked the latest in
procsignal_sigusr1_handler function.
Regards,
--
Alena Rybakina
Postgres Professional
19.09.2022, 11:01, "torikoshia" :
On 2022-05-16 17:02, torikoshia wrote:
On 2022-03-09 19:04, torikoshia wrote:
On 2022-02-08 01:13, Fujii Masao wrote:
AbortSubTransaction() should reset ActiveQueryDesc to
save_ActiveQueryDesc that ExecutorRun() set, instead
of NULL?
Otherwise ActiveQueryDesc of top-level statement will
be unavailable
after subtransaction is aborted in the nested statements.
I once agreed above suggestion and made v20 patch making
save_ActiveQueryDesc a global variable, but it caused
segfault when
calling pg_log_query_plan() after FreeQueryDesc().
OTOH, doing some kind of reset of ActiveQueryDesc seems
necessary
since it also caused segfault when running
pg_log_query_plan() during
installcheck.
There may be a better way, but resetting ActiveQueryDesc
to NULL seems
safe and simple.
Of course it makes pg_log_query_plan() useless after a
subtransaction
is aborted.
However, if it does not often happen that people want to
know the
running query's plan whose subtransaction is aborted,
resetting
ActiveQueryDesc to NULL would be acceptable.
Attached is a patch that sets ActiveQueryDesc to NULL when a
subtransaction is aborted.
How do you think?
Attached new patch to fix patch apply failures again.
--
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index a82ac87457e..65b692b0ddf 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -306,6 +306,12 @@ ExecutorRun(QueryDesc *queryDesc,
{
QueryDesc *save_ActiveQueryDesc;
+ /*
+ * Save value of ActiveQueryDesc before having called
+ * ExecutorRun_hook function due to having reset by
+ * AbortSubTransaction.
+ */
+
save_ActiveQueryDesc = ActiveQueryDesc;
ActiveQueryDesc = queryDesc;
@@ -314,6 +320,7 @@ ExecutorRun(QueryDesc *queryDesc,
else
standard_ExecutorRun(queryDesc, direction, count, execute_once);
+ /* We set the actual value of ActiveQueryDesc */
ActiveQueryDesc = save_ActiveQueryDesc;
}
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index fc9f9f8e3f0..8e7ce3c976f 100644
--- a/src/include/commands/explain.h
+++
RFC: Logging plan of the running query
Hi,
I liked this idea and after reviewing code I noticed some moments and
I'd rather ask you some questions.
Firstly, I suggest some editing in the comment of commit. I think, it is
turned out the more laconic and the same clear. I wrote it below since I
can't think of any other way to add it.
```
Currently, we have to wait for finishing of the query execution to check
its plan.
This is not so convenient in investigation long-running queries on
production
environments where we cannot use debuggers.
To improve this situation there is proposed the patch containing the
pg_log_query_plan()
function which request to log plan of the specified backend process.
By default, only superusers are allowed to request log of the plan
otherwise
allowing any users to issue this request could create cause lots of log
messages
and it can lead to denial of service.
At the next requesting CHECK_FOR_INTERRUPTS(), the target backend logs
its plan at
LOG_SERVER_ONLY level and therefore this plan will appear in the server
log only,
not to be sent to the client.
```
Secondly, I have question about deleting USE_ASSERT_CHECKING in lock.h?
It supposed to have been checked in another placed of the code by
matching values. I am worry about skipping errors due to untesting with
assert option in the places where it (GetLockMethodLocalHash)
participates and we won't able to get core file in segfault cases. I
might not understand something, then can you please explain to me?
Thirdly, I have incomprehension of the point why save_ActiveQueryDesc is
declared in the pquery.h? I am seemed to save_ActiveQueryDesc to be used
in an once time in the ExecutorRun function and its declaration
superfluous. I added it in the attached patch.
Fourthly, it seems to me there are not enough explanatory comments in
the code. I also added them in the attached patch.
Lastly, I have incomprehension about handling signals since have been
unused it before. Could another signal disabled calling this signal to
log query plan? I noticed this signal to be checked the latest in
procsignal_sigusr1_handler function.
Regards,
--
Alena Rybakina
Postgres Professional
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index a82ac87457e..65b692b0ddf 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -306,6 +306,12 @@ ExecutorRun(QueryDesc *queryDesc,
{
QueryDesc *save_ActiveQueryDesc;
+ /*
+ * Save value of ActiveQueryDesc before having called
+ * ExecutorRun_hook function due to having reset by
+ * AbortSubTransaction.
+ */
+
save_ActiveQueryDesc = ActiveQueryDesc;
ActiveQueryDesc = queryDesc;
@@ -314,6 +320,7 @@ ExecutorRun(QueryDesc *queryDesc,
else
standard_ExecutorRun(queryDesc, direction, count, execute_once);
+ /* We set the actual value of ActiveQueryDesc */
ActiveQueryDesc = save_ActiveQueryDesc;
}
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index fc9f9f8e3f0..8e7ce3c976f 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -126,6 +126,7 @@ extern void ExplainOpenGroup(const char *objtype, const char *labelname,
extern void ExplainCloseGroup(const char *objtype, const char *labelname,
bool labeled, ExplainState *es);
+/* Function to handle the signal to output the query plan. */
extern void HandleLogQueryPlanInterrupt(void);
extern void ProcessLogQueryPlanInterrupt(void);
#endif /* EXPLAIN_H */
diff --git a/src/include/tcop/pquery.h b/src/include/tcop/pquery.h
index 227d24b9d60..d04de1f5566 100644
--- a/src/include/tcop/pquery.h
+++ b/src/include/tcop/pquery.h
@@ -22,7 +22,6 @@ struct PlannedStmt;/* avoid including plannodes.h here */
extern PGDLLIMPORT Portal ActivePortal;
extern PGDLLIMPORT QueryDesc *ActiveQueryDesc;
-extern PGDLLIMPORT QueryDesc *save_ActiveQueryDesc;
extern PortalStrategy ChoosePortalStrategy(List *stmts);
