public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v4 2/3] Allow CREATE INDEX CONCURRENTLY on partitioned table
12+ messages / 5 participants
[nested] [flat]
* [PATCH v4 2/3] Allow CREATE INDEX CONCURRENTLY on partitioned table
@ 2020-06-06 22:42 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 12+ messages in thread
From: Justin Pryzby @ 2020-06-06 22:42 UTC (permalink / raw)
Note, this effectively reverts 050098b14, so take care to not reintroduce the
bug it fixed.
---
doc/src/sgml/ref/create_index.sgml | 9 --
src/backend/commands/indexcmds.c | 109 +++++++++++++++++++------
src/test/regress/expected/indexing.out | 57 ++++++++++++-
src/test/regress/sql/indexing.sql | 16 +++-
4 files changed, 150 insertions(+), 41 deletions(-)
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 33aa64e81d..c780dc9547 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -657,15 +657,6 @@ Indexes:
cannot.
</para>
- <para>
- Concurrent builds for indexes on partitioned tables are currently not
- supported. However, you may concurrently build the index on each
- partition individually and then finally create the partitioned index
- non-concurrently in order to reduce the time where writes to the
- partitioned table will be locked out. In this case, building the
- partitioned index is a metadata only operation.
- </para>
-
</refsect2>
</refsect1>
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index df3e567acb..291046cb16 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -653,17 +653,6 @@ DefineIndex(Oid relationId,
partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
if (partitioned)
{
- /*
- * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
- * the error is thrown also for temporary tables. Seems better to be
- * consistent, even though we could do it on temporary table because
- * we're not actually doing it concurrently.
- */
- if (stmt->concurrent)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot create index on partitioned table \"%s\" concurrently",
- RelationGetRelationName(rel))));
if (stmt->excludeOpNames)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -1098,6 +1087,9 @@ DefineIndex(Oid relationId,
if (pd->nparts != 0)
flags |= INDEX_CREATE_INVALID;
}
+ else if (concurrent && OidIsValid(parentIndexId))
+ /* Initial concurrent build index partition as invalid */
+ flags |= INDEX_CREATE_INVALID;
if (stmt->deferrable)
constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE;
@@ -1140,6 +1132,10 @@ DefineIndex(Oid relationId,
CreateComments(indexRelationId, RelationRelationId, 0,
stmt->idxcomment);
+ /* save lockrelid and locktag for below */
+ heaprelid = rel->rd_lockInfo.lockRelId;
+ SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
+
if (partitioned)
{
/*
@@ -1150,8 +1146,17 @@ DefineIndex(Oid relationId,
*/
if (!stmt->relation || stmt->relation->inh)
{
+ /*
+ * Need to close the relation before recursing into children, so
+ * copy needed data into a longlived context.
+ */
+
+ MemoryContext ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX",
+ ALLOCSET_DEFAULT_SIZES);
+ MemoryContext oldcontext = MemoryContextSwitchTo(ind_context);
PartitionDesc partdesc = RelationGetPartitionDesc(rel);
int nparts = partdesc->nparts;
+ char *relname = pstrdup(RelationGetRelationName(rel));
Oid *part_oids = palloc(sizeof(Oid) * nparts);
bool invalidate_parent = false;
TupleDesc parentDesc;
@@ -1161,8 +1166,10 @@ DefineIndex(Oid relationId,
nparts);
memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
+ parentDesc = CreateTupleDescCopy(RelationGetDescr(rel));
+ table_close(rel, NoLock);
+ MemoryContextSwitchTo(oldcontext);
- parentDesc = RelationGetDescr(rel);
opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes);
for (i = 0; i < numberOfKeyAttributes; i++)
opfamOids[i] = get_opclass_family(classObjectId[i]);
@@ -1197,18 +1204,20 @@ DefineIndex(Oid relationId,
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot create unique index on partitioned table \"%s\"",
- RelationGetRelationName(rel)),
+ relname),
errdetail("Table \"%s\" contains partitions that are foreign tables.",
- RelationGetRelationName(rel))));
+ relname)));
table_close(childrel, lockmode);
continue;
}
+ oldcontext = MemoryContextSwitchTo(ind_context);
childidxs = RelationGetIndexList(childrel);
attmap =
build_attrmap_by_name(RelationGetDescr(childrel),
parentDesc);
+ MemoryContextSwitchTo(oldcontext);
foreach(cell, childidxs)
{
@@ -1334,10 +1343,16 @@ DefineIndex(Oid relationId,
createdConstraintId,
is_alter_table, check_rights, check_not_in_use,
skip_build, quiet);
+ if (concurrent)
+ {
+ PushActiveSnapshot(GetTransactionSnapshot());
+ invalidate_parent = true;
+ }
}
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
- i + 1);
+ if (!concurrent)
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+ i + 1);
free_attrmap(attmap);
}
@@ -1364,23 +1379,72 @@ DefineIndex(Oid relationId,
table_close(pg_index, RowExclusiveLock);
heap_freetuple(newtup);
}
+ } else
+ table_close(rel, NoLock);
+
+ if (concurrent)
+ {
+ List *childs;
+ ListCell *lc;
+ int npart = 0;
+
+ /* Reindex invalid child indexes created earlier */
+ MemoryContext ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX",
+ ALLOCSET_DEFAULT_SIZES);
+ MemoryContext oldcontext = MemoryContextSwitchTo(ind_context);
+ childs = find_inheritance_children(indexRelationId, NoLock);
+ MemoryContextSwitchTo(oldcontext);
+
+ /* Make the catalog changes visible to get_partition_parent */
+ CommandCounterIncrement();
+
+ foreach (lc, childs)
+ {
+ Oid indexrelid = lfirst_oid(lc);
+ Relation cldidx;
+ bool isvalid;
+
+ if (!OidIsValid(parentIndexId))
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+ npart++);
+
+ cldidx = index_open(indexrelid, ShareUpdateExclusiveLock);
+ isvalid = cldidx->rd_index->indisvalid;
+ index_close(cldidx, NoLock);
+ if (isvalid)
+ continue;
+
+ /* This may be a partitioned index, which is fine too */
+ ReindexRelationConcurrently(indexrelid, 0);
+ PushActiveSnapshot(GetTransactionSnapshot());
+ }
+
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+ StartTransactionCommand();
+
+ /*
+ * CIC needs to mark a partitioned index as VALID, which itself
+ * requires setting READY, which is unset for CIC (even though
+ * it's meaningless for an index without storage).
+ */
+ index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
+ CommandCounterIncrement();
+ index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
}
/*
* Indexes on partitioned tables are not themselves built, so we're
* done here.
*/
- table_close(rel, NoLock);
if (!OidIsValid(parentIndexId))
pgstat_progress_end_command();
return address;
}
+ table_close(rel, NoLock);
if (!concurrent)
{
- /* Close the heap and we're done, in the non-concurrent case */
- table_close(rel, NoLock);
-
/* If this is the top-level index, we're done. */
if (!OidIsValid(parentIndexId))
pgstat_progress_end_command();
@@ -1388,11 +1452,6 @@ DefineIndex(Oid relationId,
return address;
}
- /* save lockrelid and locktag for below, then close rel */
- heaprelid = rel->rd_lockInfo.lockRelId;
- SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
- table_close(rel, NoLock);
-
/*
* For a concurrent build, it's important to make the catalog entries
* visible to other transactions before we start to build the index. That
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index f78865ef81..1ae58e110f 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -50,11 +50,60 @@ select relname, relkind, relhassubclass, inhparent::regclass
(8 rows)
drop table idxpart;
--- Some unsupported features
+-- CIC on partitioned table
create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
-ERROR: cannot create index on partitioned table "idxpart" concurrently
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart (a); -- partitioned
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart2 (a); -- leaf
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+ERROR: could not create unique index "idxpart2_a_idx2"
+DETAIL: Key (a)=(10) is duplicated.
+\d idxpart
+ Partitioned table "public.idxpart"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+ b | integer | | |
+ c | text | | |
+Partition key: RANGE (a)
+Indexes:
+ "idxpart_a_idx" btree (a)
+ "idxpart_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 2 (Use \d+ to list them.)
+
+\d idxpart1
+ Partitioned table "public.idxpart1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+ b | integer | | |
+ c | text | | |
+Partition of: idxpart FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+ "idxpart1_a_idx" btree (a)
+ "idxpart1_a_idx1" btree (a)
+ "idxpart1_a_idx2" UNIQUE, btree (a)
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart2
+ Table "public.idxpart2"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+ b | integer | | |
+ c | text | | |
+Partition of: idxpart FOR VALUES FROM (10) TO (20)
+Indexes:
+ "idxpart2_a_idx" btree (a)
+ "idxpart2_a_idx1" btree (a)
+ "idxpart2_a_idx2" UNIQUE, btree (a) INVALID
+
drop table idxpart;
-- Verify bugfix with query on indexed partitioned table with no partitions
-- https://postgr.es/m/[email protected]
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index 35d159f41b..d908ee6d17 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -29,10 +29,20 @@ select relname, relkind, relhassubclass, inhparent::regclass
where relname like 'idxpart%' order by relname;
drop table idxpart;
--- Some unsupported features
+-- CIC on partitioned table
create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart (a); -- partitioned
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart2 (a); -- leaf
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+\d idxpart
+\d idxpart1
+\d idxpart2
drop table idxpart;
-- Verify bugfix with query on indexed partitioned table with no partitions
--
2.17.0
--JYK4vJDZwFMowpUq
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0003-Implement-CLUSTER-of-partitioned-table.patch"
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_stat_statements and "IN" conditions
@ 2022-01-04 23:02 Tom Lane <[email protected]>
2022-01-05 04:37 ` Re: pg_stat_statements and "IN" conditions Andrey V. Lepikhov <[email protected]>
2022-01-05 21:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 2 replies; 12+ messages in thread
From: Tom Lane @ 2022-01-04 23:02 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
Dmitry Dolgov <[email protected]> writes:
> And now for something completely different, here is a new patch version.
> It contains a small fix for one problem we've found during testing (one
> path code was incorrectly assuming find_const_walker results).
I've been saying from day one that pushing the query-hashing code into the
core was a bad idea, and I think this patch perfectly illustrates why.
We can debate whether the rules proposed here are good for
pg_stat_statements or not, but it seems inevitable that they will be a
disaster for some other consumers of the query hash. In particular,
dropping external parameters from the hash seems certain to break
something for somebody --- do you really think that a query with two int
parameters is equivalent to one with five float parameters for all
query-identifying purposes?
I can see the merits of allowing different numbers of IN elements
to be considered equivalent for pg_stat_statements, but this patch
seems to go far beyond that basic idea, and I fear the side-effects
will be very bad.
Also, calling eval_const_expressions in the query jumbler is flat
out unacceptable. There is way too much code that could be reached
that way (more or less the entire executor, to start with). I
don't have a lot of faith that it'd never modify the input tree,
either.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-01-04 23:02 Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
@ 2022-01-05 04:37 ` Andrey V. Lepikhov <[email protected]>
2022-01-05 05:13 ` Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
1 sibling, 1 reply; 12+ messages in thread
From: Andrey V. Lepikhov @ 2022-01-05 04:37 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Dmitry Dolgov <[email protected]>; +Cc: Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
On 1/5/22 4:02 AM, Tom Lane wrote:
> Dmitry Dolgov <[email protected]> writes:
>> And now for something completely different, here is a new patch version.
>> It contains a small fix for one problem we've found during testing (one
>> path code was incorrectly assuming find_const_walker results).
>
> I've been saying from day one that pushing the query-hashing code into the
> core was a bad idea, and I think this patch perfectly illustrates why.
> We can debate whether the rules proposed here are good for
> pg_stat_statements or not, but it seems inevitable that they will be a
> disaster for some other consumers of the query hash. In particular,
> dropping external parameters from the hash seems certain to break
> something for somebody
+1.
In a couple of extensions I use different logic of query jumbling - hash
value is more stable in some cases than in default implementation. For
example, it should be stable to permutations in 'FROM' section of a query.
And If anyone subtly changes jumbling logic when the extension is
active, the instance could get huge performance issues.
Let me suggest, that the core should allow an extension at least to
detect such interference between extensions. Maybe hook could be
replaced with callback to allow extension see an queryid with underlying
generation logic what it expects.
--
regards,
Andrey Lepikhov
Postgres Professional
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-01-04 23:02 Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-01-05 04:37 ` Re: pg_stat_statements and "IN" conditions Andrey V. Lepikhov <[email protected]>
@ 2022-01-05 05:13 ` Tom Lane <[email protected]>
0 siblings, 0 replies; 12+ messages in thread
From: Tom Lane @ 2022-01-05 05:13 UTC (permalink / raw)
To: Andrey V. Lepikhov <[email protected]>; +Cc: Dmitry Dolgov <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
"Andrey V. Lepikhov" <[email protected]> writes:
> On 1/5/22 4:02 AM, Tom Lane wrote:
>> I've been saying from day one that pushing the query-hashing code into the
>> core was a bad idea, and I think this patch perfectly illustrates why.
> +1.
> Let me suggest, that the core should allow an extension at least to
> detect such interference between extensions. Maybe hook could be
> replaced with callback to allow extension see an queryid with underlying
> generation logic what it expects.
I feel like we need to get away from the idea that there is just
one query hash, and somehow let different extensions attach
differently-calculated hashes to a query. I don't have any immediate
ideas about how to do that in a reasonably inexpensive way.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-01-04 23:02 Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
@ 2022-01-05 21:11 ` Dmitry Dolgov <[email protected]>
2022-03-10 16:38 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
1 sibling, 1 reply; 12+ messages in thread
From: Dmitry Dolgov @ 2022-01-05 21:11 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Tue, Jan 04, 2022 at 06:02:43PM -0500, Tom Lane wrote:
> We can debate whether the rules proposed here are good for
> pg_stat_statements or not, but it seems inevitable that they will be a
> disaster for some other consumers of the query hash.
Hm, which consumers do you mean here, potential extension? Isn't the
ability to use an external module to compute queryid make this situation
possible anyway?
> do you really think that a query with two int
> parameters is equivalent to one with five float parameters for all
> query-identifying purposes?
Nope, and it will be hard to figure this out no matter which approach
we're talking about, because it mostly depends on the context and type
of queries I guess. Instead, such functionality should allow some
reasonable configuration. To be clear, the use case I have in mind here
is not four or five, but rather a couple of hundreds constants where
chances that the whole construction was generated automatically by ORM
is higher than normal.
> I can see the merits of allowing different numbers of IN elements
> to be considered equivalent for pg_stat_statements, but this patch
> seems to go far beyond that basic idea, and I fear the side-effects
> will be very bad.
Not sure why it goes far beyond, but then there were two approaches
under consideration, as I've stated in the first message. I already
don't remember all the details, but another one was evolving around
doing similar things in a more limited fashion in transformAExprIn. The
problem would be then to carry the information, necessary to represent
the act of "merging" some number of queryids together. Any thoughts
here?
The idea of keeping the original queryid untouched and add another type
of id instead sounds interesting, but it will add too much overhead for
a quite small use case I guess.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-01-04 23:02 Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-01-05 21:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2022-03-10 16:38 ` Dmitry Dolgov <[email protected]>
2022-03-10 17:11 ` Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Dolgov @ 2022-03-10 16:38 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Wed, Jan 05, 2022 at 10:11:11PM +0100, Dmitry Dolgov wrote:
> > On Tue, Jan 04, 2022 at 06:02:43PM -0500, Tom Lane wrote:
> > We can debate whether the rules proposed here are good for
> > pg_stat_statements or not, but it seems inevitable that they will be a
> > disaster for some other consumers of the query hash.
>
> Hm, which consumers do you mean here, potential extension? Isn't the
> ability to use an external module to compute queryid make this situation
> possible anyway?
>
> > do you really think that a query with two int
> > parameters is equivalent to one with five float parameters for all
> > query-identifying purposes?
>
> Nope, and it will be hard to figure this out no matter which approach
> we're talking about, because it mostly depends on the context and type
> of queries I guess. Instead, such functionality should allow some
> reasonable configuration. To be clear, the use case I have in mind here
> is not four or five, but rather a couple of hundreds constants where
> chances that the whole construction was generated automatically by ORM
> is higher than normal.
>
> > I can see the merits of allowing different numbers of IN elements
> > to be considered equivalent for pg_stat_statements, but this patch
> > seems to go far beyond that basic idea, and I fear the side-effects
> > will be very bad.
>
> Not sure why it goes far beyond, but then there were two approaches
> under consideration, as I've stated in the first message. I already
> don't remember all the details, but another one was evolving around
> doing similar things in a more limited fashion in transformAExprIn. The
> problem would be then to carry the information, necessary to represent
> the act of "merging" some number of queryids together. Any thoughts
> here?
>
> The idea of keeping the original queryid untouched and add another type
> of id instead sounds interesting, but it will add too much overhead for
> a quite small use case I guess.
```
Thu, 10 Mar 2022
New status: Waiting on Author
```
This seems incorrect, as the only feedback I've got was "this is a bad
idea", and no reaction on follow-up questions.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-01-04 23:02 Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-01-05 21:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 16:38 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2022-03-10 17:11 ` Tom Lane <[email protected]>
2022-03-10 17:32 ` Re: pg_stat_statements and "IN" conditions Robert Haas <[email protected]>
2022-03-12 14:10 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 2 replies; 12+ messages in thread
From: Tom Lane @ 2022-03-10 17:11 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
Dmitry Dolgov <[email protected]> writes:
> New status: Waiting on Author
> This seems incorrect, as the only feedback I've got was "this is a bad
> idea", and no reaction on follow-up questions.
I changed the status because it seems to me there is no chance of
this being committed as-is.
1. I think an absolute prerequisite before we could even consider
changing the query jumbler rules this much is to do the work that was
put off when the jumbler was moved into core: that is, provide some
honest support for multiple query-ID generation methods being used at
the same time. Even if you successfully make a case for
pg_stat_statements to act this way, other consumers of query IDs
aren't going to be happy with it.
2. You haven't made a case for it. The original complaint was
about different lengths of IN lists not being treated as equivalent,
but this patch has decided to do I'm-not-even-sure-quite-what
about treating different Params as equivalent. Plus you're trying
to invoke eval_const_expressions in the jumbler; that is absolutely
Not OK, for both safety and semantic reasons.
If you backed off to just treating ArrayExprs containing different
numbers of Consts as equivalent, maybe that'd be something we could
adopt without fixing point 1. I don't think anything that fuzzes the
treatment of Params can get away with that, though.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-01-04 23:02 Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-01-05 21:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 16:38 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 17:11 ` Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
@ 2022-03-10 17:32 ` Robert Haas <[email protected]>
2022-03-10 19:06 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
1 sibling, 1 reply; 12+ messages in thread
From: Robert Haas @ 2022-03-10 17:32 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Dmitry Dolgov <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
On Thu, Mar 10, 2022 at 12:12 PM Tom Lane <[email protected]> wrote:
> > This seems incorrect, as the only feedback I've got was "this is a bad
> > idea", and no reaction on follow-up questions.
>
> I changed the status because it seems to me there is no chance of
> this being committed as-is.
>
> 1. I think an absolute prerequisite before we could even consider
> changing the query jumbler rules this much is to do the work that was
> put off when the jumbler was moved into core: that is, provide some
> honest support for multiple query-ID generation methods being used at
> the same time. Even if you successfully make a case for
> pg_stat_statements to act this way, other consumers of query IDs
> aren't going to be happy with it.
FWIW, I don't find this convincing at all. Query jumbling is already
somewhat expensive, and it seems unlikely that the same person is
going to want to jumble queries in one way for pg_stat_statements and
another way for pg_stat_broccoli or whatever their other extension is.
Putting a lot of engineering work into something with such a marginal
use case seems not worthwhile to me - and also likely futile, because
I don't see how it could realistically be made nearly as cheap as a
single jumble.
> 2. You haven't made a case for it. The original complaint was
> about different lengths of IN lists not being treated as equivalent,
> but this patch has decided to do I'm-not-even-sure-quite-what
> about treating different Params as equivalent. Plus you're trying
> to invoke eval_const_expressions in the jumbler; that is absolutely
> Not OK, for both safety and semantic reasons.
I think there are two separate points here, one about patch quality
and the other about whether the basic idea is good. I think the basic
idea is good. I do not contend that collapsing IN-lists of arbitrary
length is what everyone wants in all cases, but it seems entirely
reasonable to me to think that it is what some people want. So I would
say just make it a parameter and let people configure whichever
behavior they want. My bet is 95% of users would prefer to have it on,
but even if that's wildly wrong, having it as an optional behavior
hurts nobody. Let it be off by default and let those who want it flip
the toggle. On the code quality issue, I haven't read the patch but
your concerns sound well-founded to me from reading what you wrote.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-01-04 23:02 Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-01-05 21:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 16:38 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 17:11 ` Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-03-10 17:32 ` Re: pg_stat_statements and "IN" conditions Robert Haas <[email protected]>
@ 2022-03-10 19:06 ` Dmitry Dolgov <[email protected]>
0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Dolgov @ 2022-03-10 19:06 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Thu, Mar 10, 2022 at 12:32:08PM -0500, Robert Haas wrote:
> On Thu, Mar 10, 2022 at 12:12 PM Tom Lane <[email protected]> wrote:
>
> > 2. You haven't made a case for it. The original complaint was
> > about different lengths of IN lists not being treated as equivalent,
> > but this patch has decided to do I'm-not-even-sure-quite-what
> > about treating different Params as equivalent. Plus you're trying
> > to invoke eval_const_expressions in the jumbler; that is absolutely
> > Not OK, for both safety and semantic reasons.
>
> I think there are two separate points here, one about patch quality
> and the other about whether the basic idea is good. I think the basic
> idea is good. I do not contend that collapsing IN-lists of arbitrary
> length is what everyone wants in all cases, but it seems entirely
> reasonable to me to think that it is what some people want. So I would
> say just make it a parameter and let people configure whichever
> behavior they want. My bet is 95% of users would prefer to have it on,
> but even if that's wildly wrong, having it as an optional behavior
> hurts nobody. Let it be off by default and let those who want it flip
> the toggle. On the code quality issue, I haven't read the patch but
> your concerns sound well-founded to me from reading what you wrote.
I have the same understanding, there is a toggle in the patch exactly
for this purpose.
To give a bit more context, the whole development was ORM-driven rather
than pulled out of thin air -- people were complaining about huge
generated queries that could be barely displayed in monitoring, I was
trying to address it via collapsing the list where it was happening. In
other words "I'm-not-even-sure-quite-what" part may be indeed too
extensive, but was triggered by real world issues.
Of course, I could get the implementation not quite right, e.g. I wasn't
aware about dangers of using eval_const_expressions. But that's what the
CF item and the corresponding discussion is for, I guess. Let me see
what I could do to improve it.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-01-04 23:02 Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-01-05 21:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 16:38 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 17:11 ` Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
@ 2022-03-12 14:10 ` Dmitry Dolgov <[email protected]>
2022-03-14 14:17 ` Re: pg_stat_statements and "IN" conditions Robert Haas <[email protected]>
1 sibling, 1 reply; 12+ messages in thread
From: Dmitry Dolgov @ 2022-03-12 14:10 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Thu, Mar 10, 2022 at 12:11:59PM -0500, Tom Lane wrote:
> Dmitry Dolgov <[email protected]> writes:
> > New status: Waiting on Author
>
> > This seems incorrect, as the only feedback I've got was "this is a bad
> > idea", and no reaction on follow-up questions.
>
> I changed the status because it seems to me there is no chance of
> this being committed as-is.
>
> 1. I think an absolute prerequisite before we could even consider
> changing the query jumbler rules this much is to do the work that was
> put off when the jumbler was moved into core: that is, provide some
> honest support for multiple query-ID generation methods being used at
> the same time. Even if you successfully make a case for
> pg_stat_statements to act this way, other consumers of query IDs
> aren't going to be happy with it.
>
> 2. You haven't made a case for it. The original complaint was
> about different lengths of IN lists not being treated as equivalent,
> but this patch has decided to do I'm-not-even-sure-quite-what
> about treating different Params as equivalent. Plus you're trying
> to invoke eval_const_expressions in the jumbler; that is absolutely
> Not OK, for both safety and semantic reasons.
>
> If you backed off to just treating ArrayExprs containing different
> numbers of Consts as equivalent, maybe that'd be something we could
> adopt without fixing point 1. I don't think anything that fuzzes the
> treatment of Params can get away with that, though.
Here is the limited version of list collapsing functionality, which
doesn't utilize eval_const_expressions and ignores most of the stuff
except ArrayExprs. Any thoughts/more suggestions?
Attachments:
[text/x-diff] v6-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch (32.7K, ../../[email protected]/2-v6-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch)
download | inline diff:
From ce9f2ed2466d28dbbef3310383d84eba58e5791b Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <[email protected]>
Date: Sat, 12 Mar 2022 14:42:02 +0100
Subject: [PATCH v6] Prevent jumbling of every element in ArrayExpr
pg_stat_statements produces multiple entries for queries like
SELECT something FROM table WHERE col IN (1, 2, 3, ...)
depending on number of parameters, because every element of ArrayExpr is
jumbled. Make Consts contribute nothing to the jumble hash if they're
part of a series and at position further that specified threshold.
Reviewed-by: Zhihong Yu, Sergey Dudoladov
Tested-by: Chengxi Sun
---
.../expected/pg_stat_statements.out | 412 ++++++++++++++++++
.../pg_stat_statements/pg_stat_statements.c | 26 +-
.../sql/pg_stat_statements.sql | 107 +++++
src/backend/utils/misc/guc.c | 13 +
src/backend/utils/misc/queryjumble.c | 236 +++++++++-
src/include/utils/queryjumble.h | 10 +-
6 files changed, 791 insertions(+), 13 deletions(-)
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..e05a6f565a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -1077,4 +1077,416 @@ SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%';
2
(1 row)
+--
+-- Consts merging
+--
+CREATE TABLE test_merge (id int, data int);
+-- IN queries
+-- No merging
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+--------------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(7 rows)
+
+-- Normal
+SET const_merge_threshold = 5;
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3) | 1
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(3 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, ...) | 5
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 1
+(4 rows)
+
+-- On the merge threshold
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4) | 1
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(3 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, ...) | 6
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 1
+(4 rows)
+
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, ...) | 1
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(3 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, ...) | 6
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 1
+(3 rows)
+
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, ...) | 1
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(3 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, ...) | 5
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 1
+(3 rows)
+
+-- With gaps on the threshold
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4) | 1
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(3 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, ...) | 1
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 1
+(4 rows)
+
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, ...) | 1
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(3 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, ...) | 2
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 1
+(3 rows)
+
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, ...) | 1
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(3 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, ...) | 2
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 1
+(3 rows)
+
+-- test constants after merge
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10) and data = 2;
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+---------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, ...) and data = $11 | 1
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(3 rows)
+
+-- On table, numeric type causes every constant being wrapped into functions.
+CREATE TABLE test_merge_numeric (id int, data numeric(5, 2));
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+SELECT * FROM test_merge_numeric WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge_numeric WHERE id IN ($1, $2, $3, $4, ...) | 1
+ SELECT pg_stat_statements_reset() | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0
+(3 rows)
+
+-- Test find_const_walker
+WITH cte AS (
+ SELECT 'const' as const FROM test_merge
+)
+SELECT ARRAY['a', 'b', 'c', const::varchar] AS result
+FROM cte;
+ result
+--------
+(0 rows)
+
+RESET const_merge_threshold;
DROP EXTENSION pg_stat_statements;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 082bfa8f77..b872490133 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2610,6 +2610,7 @@ generate_normalized_query(JumbleState *jstate, const char *query,
n_quer_loc = 0, /* Normalized query byte location */
last_off = 0, /* Offset from start for previous tok */
last_tok_len = 0; /* Length (in bytes) of that tok */
+ bool merge = false;
/*
* Get constants' lengths (core system only gives us locations). Note
@@ -2648,12 +2649,27 @@ generate_normalized_query(JumbleState *jstate, const char *query,
len_to_wrt -= last_tok_len;
Assert(len_to_wrt >= 0);
- memcpy(norm_query + n_quer_loc, query + quer_loc, len_to_wrt);
- n_quer_loc += len_to_wrt;
- /* And insert a param symbol in place of the constant token */
- n_quer_loc += sprintf(norm_query + n_quer_loc, "$%d",
- i + 1 + jstate->highest_extern_param_id);
+ if (!jstate->clocations[i].merged)
+ {
+ memcpy(norm_query + n_quer_loc, query + quer_loc, len_to_wrt);
+ n_quer_loc += len_to_wrt;
+
+ /* And insert a param symbol in place of the constant token */
+ n_quer_loc += sprintf(norm_query + n_quer_loc, "$%d",
+ i + 1 + jstate->highest_extern_param_id);
+ if (merge)
+ merge = false;
+ }
+ else if (!merge)
+ {
+ memcpy(norm_query + n_quer_loc, query + quer_loc, len_to_wrt);
+ n_quer_loc += len_to_wrt;
+
+ /* Merge until a non merged constant appear */
+ merge = true;
+ n_quer_loc += sprintf(norm_query + n_quer_loc, "...");
+ }
quer_loc = off + tok_len;
last_off = off;
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
index dffd2c8c18..55ba3b35e7 100644
--- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -442,4 +442,111 @@ SELECT (
SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%';
+--
+-- Consts merging
+--
+CREATE TABLE test_merge (id int, data int);
+
+-- IN queries
+
+-- No merging
+SELECT pg_stat_statements_reset();
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Normal
+SET const_merge_threshold = 5;
+
+SELECT pg_stat_statements_reset();
+SELECT * FROM test_merge WHERE id IN (1, 2, 3);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- On the merge threshold
+SELECT pg_stat_statements_reset();
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT pg_stat_statements_reset();
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT pg_stat_statements_reset();
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- With gaps on the threshold
+SELECT pg_stat_statements_reset();
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT pg_stat_statements_reset();
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT pg_stat_statements_reset();
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- test constants after merge
+SELECT pg_stat_statements_reset();
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10) and data = 2;
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- On table, numeric type causes every constant being wrapped into functions.
+CREATE TABLE test_merge_numeric (id int, data numeric(5, 2));
+SELECT pg_stat_statements_reset();
+SELECT * FROM test_merge_numeric WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Test find_const_walker
+WITH cte AS (
+ SELECT 'const' as const FROM test_merge
+)
+SELECT ARRAY['a', 'b', 'c', const::varchar] AS result
+FROM cte;
+
+RESET const_merge_threshold;
+
DROP EXTENSION pg_stat_statements;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f505413a7f..45a6d593ce 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3600,6 +3600,19 @@ static struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"const_merge_threshold", PGC_SUSET, STATS_MONITORING,
+ gettext_noop("Sets the minimal numer of constants in an array"
+ " after which they will be merged"),
+ gettext_noop("Computing query id for an array of constants"
+ " will produce the same id for all arrays with length"
+ " larger than this value. Zero turns off merging."),
+ },
+ &const_merge_threshold,
+ 0, 0, INT_MAX,
+ NULL, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL
diff --git a/src/backend/utils/misc/queryjumble.c b/src/backend/utils/misc/queryjumble.c
index a67487e5fe..563de94f9f 100644
--- a/src/backend/utils/misc/queryjumble.c
+++ b/src/backend/utils/misc/queryjumble.c
@@ -42,6 +42,9 @@
/* GUC parameters */
int compute_query_id = COMPUTE_QUERY_ID_AUTO;
+/* Minimal numer of constants in an array after which they will be merged */
+int const_merge_threshold = 0;
+
/* True when compute_query_id is ON, or AUTO and a module requests them */
bool query_id_enabled = false;
@@ -52,7 +55,8 @@ static void JumbleQueryInternal(JumbleState *jstate, Query *query);
static void JumbleRangeTable(JumbleState *jstate, List *rtable);
static void JumbleRowMarks(JumbleState *jstate, List *rowMarks);
static void JumbleExpr(JumbleState *jstate, Node *node);
-static void RecordConstLocation(JumbleState *jstate, int location);
+static bool JumbleExprList(JumbleState *jstate, Node *node);
+static void RecordConstLocation(JumbleState *jstate, int location, bool merged);
/*
* Given a possibly multi-statement source string, confine our attention to the
@@ -119,7 +123,7 @@ JumbleQuery(Query *query, const char *querytext)
jstate->jumble_len = 0;
jstate->clocations_buf_size = 32;
jstate->clocations = (LocationLen *)
- palloc(jstate->clocations_buf_size * sizeof(LocationLen));
+ palloc0(jstate->clocations_buf_size * sizeof(LocationLen));
jstate->clocations_count = 0;
jstate->highest_extern_param_id = 0;
@@ -341,6 +345,225 @@ JumbleRowMarks(JumbleState *jstate, List *rowMarks)
}
}
+/*
+ * find_const_walker
+ * Locate all the Const nodes in an expression tree.
+ *
+ * Caller must provide an empty list where constants will be collected.
+ */
+static bool
+find_const_walker(Node *node, List **constants)
+{
+ if (node == NULL)
+ return false;
+
+ if (IsA(node, Const))
+ {
+ *constants = lappend(*constants, (Const *) node);
+ return false;
+ }
+
+ return expression_tree_walker(node, find_const_walker, (void *) constants);
+}
+
+static bool
+JumbleExprList(JumbleState *jstate, Node *node)
+{
+ ListCell *temp;
+ Node *firstExpr = NULL;
+ bool merged = false;
+ bool allConst = true;
+ int currentExprIdx;
+ int lastExprLength = 0;
+
+ if (node == NULL)
+ return merged;
+
+ if (const_merge_threshold == 0)
+ {
+ /* Merging is disabled, process everything one by one. */
+ JumbleExpr(jstate, node);
+ return merged;
+ }
+
+ /* Guard against stack overflow due to overly complex expressions */
+ check_stack_depth();
+
+ Assert(IsA(node, List));
+ firstExpr = (Node *) lfirst(list_head((List *) node));
+
+ /*
+ * We always emit the node's NodeTag, then any additional fields that are
+ * considered significant, and then we recurse to any child nodes.
+ */
+ APP_JUMB(node->type);
+
+ /*
+ * If the first expression is a constant or a list of constants, try to
+ * merge the following if they're constants as well. Otherwise do
+ * JumbleExpr as usual.
+ */
+ switch (nodeTag(firstExpr))
+ {
+ case T_List:
+ currentExprIdx = 0;
+
+ foreach(temp, (List *) node)
+ {
+ List *expr = (List *) lfirst(temp);
+ ListCell *lc;
+
+ foreach(lc, expr)
+ {
+ Node * subExpr = (Node *) lfirst(lc);
+
+ if (!IsA(subExpr, Const))
+ {
+ allConst = false;
+ break;
+ }
+ }
+
+ if (allConst && currentExprIdx >= const_merge_threshold - 1)
+ {
+ merged = true;
+
+ /*
+ * This hash is going to accumulate the following merged
+ * statements
+ */
+ if (currentExprIdx == const_merge_threshold - 1)
+ {
+ JumbleExpr(jstate, (Node *) expr);
+
+ /*
+ * An expr consisting of constants is already found,
+ * JumbleExpr must record it. Mark all the constants as
+ * merged, they will be the first merged but still
+ * present in the statement query.
+ */
+ Assert(jstate->clocations_count > lastExprLength - 1);
+ for (int i = 1; i < lastExprLength + 1; i++)
+ {
+ LocationLen *loc;
+ loc = &jstate->clocations[jstate->clocations_count - i];
+ loc->merged = true;
+ }
+ currentExprIdx++;
+ }
+ else
+ foreach(lc, expr)
+ {
+ /*
+ * Find the last constant to provide information
+ * for generate_normalized_query where the current
+ * expression actually ends.
+ */
+ Const *lastConst;
+ List *constants = NIL;
+ find_const_walker((Node *) lfirst(lc), &constants);
+
+ /* We should be able to find some constants, as
+ * they were discovered before. */
+ Assert(constants != NIL);
+ lastConst = (Const *) llast(constants);
+ RecordConstLocation(jstate, lastConst->location, true);
+ }
+
+ continue;
+ }
+
+ JumbleExpr(jstate, (Node *) expr);
+ currentExprIdx++;
+ lastExprLength = expr->length;
+ }
+ break;
+
+ case T_Const:
+ currentExprIdx = 0;
+
+ foreach(temp, (List *) node)
+ {
+ Node *expr = (Node *) lfirst(temp);
+
+ if (IsA(expr, Const) && currentExprIdx >= const_merge_threshold - 1)
+ {
+ merged = true;
+
+ /*
+ * This hash is going to accumulate the following merged
+ * statements
+ */
+ if (currentExprIdx == const_merge_threshold - 1)
+ {
+ JumbleExpr(jstate, expr);
+
+ /*
+ * A const expr is already found, so JumbleExpr must
+ * record it. Mark it as merged, it will be the first
+ * merged but still present in the statement query.
+ */
+ Assert(jstate->clocations_count > lastExprLength - 1);
+ for (int i = 1; i < lastExprLength + 1; i++)
+ {
+ LocationLen *loc;
+ loc = &jstate->clocations[jstate->clocations_count - i];
+ loc->merged = true;
+ }
+ currentExprIdx++;
+ }
+ else
+ {
+ /*
+ * Find the last constant to provide information
+ * for generate_normalized_query where the current
+ * expression actually ends.
+ */
+ Const *lastConst;
+ List *constants = NIL;
+ find_const_walker(expr, &constants);
+
+ /* We should be able to find some constants, as
+ * they were discovered before. */
+ Assert(constants != NIL);
+ lastConst = (Const *) llast(constants);
+ RecordConstLocation(jstate, lastConst->location, true);
+ }
+
+ continue;
+ }
+
+ JumbleExpr(jstate, expr);
+ currentExprIdx++;
+
+ if (currentExprIdx == const_merge_threshold -1)
+ {
+ /*
+ * The next expression will be eligible for merging check.
+ * For it to happen correctly remember the number of
+ * constants in the previous expression.
+ */
+ List *constants = NIL;
+ find_const_walker(expr, &constants);
+
+ /* The expression we work with here could be anything, so
+ * no constants found is a possible outcome. */
+ if (constants != NIL)
+ lastExprLength = constants->length;
+ else
+ lastExprLength = 1;
+ }
+ }
+ break;
+
+ default:
+ JumbleExpr(jstate, node);
+ break;
+ }
+
+ return merged;
+}
+
/*
* Jumble an expression tree
*
@@ -390,7 +613,7 @@ JumbleExpr(JumbleState *jstate, Node *node)
/* We jumble only the constant's type, not its value */
APP_JUMB(c->consttype);
/* Also, record its parse location for query normalization */
- RecordConstLocation(jstate, c->location);
+ RecordConstLocation(jstate, c->location, false);
}
break;
case T_Param:
@@ -579,7 +802,7 @@ JumbleExpr(JumbleState *jstate, Node *node)
}
break;
case T_ArrayExpr:
- JumbleExpr(jstate, (Node *) ((ArrayExpr *) node)->elements);
+ JumbleExprList(jstate, (Node *) ((ArrayExpr *) node)->elements);
break;
case T_RowExpr:
JumbleExpr(jstate, (Node *) ((RowExpr *) node)->args);
@@ -832,11 +1055,11 @@ JumbleExpr(JumbleState *jstate, Node *node)
}
/*
- * Record location of constant within query string of query tree
+ * Record location of constant or a parameter within query string of query tree
* that is currently being walked.
*/
static void
-RecordConstLocation(JumbleState *jstate, int location)
+RecordConstLocation(JumbleState *jstate, int location, bool merged)
{
/* -1 indicates unknown or undefined location */
if (location >= 0)
@@ -851,6 +1074,7 @@ RecordConstLocation(JumbleState *jstate, int location)
sizeof(LocationLen));
}
jstate->clocations[jstate->clocations_count].location = location;
+ jstate->clocations[jstate->clocations_count].merged = merged;
/* initialize lengths to -1 to simplify third-party module usage */
jstate->clocations[jstate->clocations_count].length = -1;
jstate->clocations_count++;
diff --git a/src/include/utils/queryjumble.h b/src/include/utils/queryjumble.h
index a4c277269e..157e3f777a 100644
--- a/src/include/utils/queryjumble.h
+++ b/src/include/utils/queryjumble.h
@@ -15,6 +15,7 @@
#define QUERYJUBLE_H
#include "nodes/parsenodes.h"
+#include "nodes/nodeFuncs.h"
#define JUMBLE_SIZE 1024 /* query serialization buffer size */
@@ -25,6 +26,8 @@ typedef struct LocationLen
{
int location; /* start offset in query text */
int length; /* length in bytes, or -1 to ignore */
+ bool merged; /* whether or not the location was marked as
+ duplicate */
} LocationLen;
/*
@@ -39,7 +42,10 @@ typedef struct JumbleState
/* Number of bytes used in jumble[] */
Size jumble_len;
- /* Array of locations of constants that should be removed */
+ /*
+ * Array of locations of constants that should be removed, or parameters
+ * that are already replaced, but could be also processed to be merged
+ */
LocationLen *clocations;
/* Allocated length of clocations array */
@@ -62,7 +68,7 @@ enum ComputeQueryIdType
/* GUC parameters */
extern int compute_query_id;
-
+extern int const_merge_threshold;
extern const char *CleanQuerytext(const char *query, int *location, int *len);
extern JumbleState *JumbleQuery(Query *query, const char *querytext);
--
2.32.0
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-01-04 23:02 Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-01-05 21:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 16:38 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 17:11 ` Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-03-12 14:10 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2022-03-14 14:17 ` Robert Haas <[email protected]>
2022-03-14 14:57 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Robert Haas @ 2022-03-14 14:17 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Tom Lane <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
On Sat, Mar 12, 2022 at 9:11 AM Dmitry Dolgov <[email protected]> wrote:
> Here is the limited version of list collapsing functionality, which
> doesn't utilize eval_const_expressions and ignores most of the stuff
> except ArrayExprs. Any thoughts/more suggestions?
The proposed commit message says this commit intends to "Make Consts
contribute nothing to the jumble hash if they're part of a series and
at position further that specified threshold." I'm not sure whether
that's what the patch actually implements because I can't immediately
understand the new logic you've added, but I think if we did what that
sentence said then, supposing the threshold is set to 1, it would
result in producing the same hash for "x in (1,2)" that we do for "x
in (1,3)" but a different hash for "x in (2,3)" which does not sound
like what we want. What I would have thought we'd do is: if the list
is all constants and long enough to satisfy the threshold then nothing
in the list gets jumbled.
I'm a little surprised that there's not more context-awareness in this
code. It seems that it applies to every ArrayExpr found in the query,
which I think would extend to cases beyond something = IN(whatever).
In particular, any use of ARRAY[] in the query would be impacted. Now,
the comments seem to imply that's pretty intentional, but from the
user's point of view, WHERE x in (1,3) and x = any(array[1,3]) are two
different things. If anything like this is to be adopted, we certainly
need to be precise about exactly what it is doing and which cases are
covered. I thought of looking at the documentation to see whether
you'd tried to clarify this there, and found that you hadn't written
any.
In short, I think this patch is not really very close to being in
committable shape even if nobody were objecting to the concept.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-01-04 23:02 Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-01-05 21:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 16:38 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 17:11 ` Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-03-12 14:10 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-14 14:17 ` Re: pg_stat_statements and "IN" conditions Robert Haas <[email protected]>
@ 2022-03-14 14:57 ` Dmitry Dolgov <[email protected]>
0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Dolgov @ 2022-03-14 14:57 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Mon, Mar 14, 2022 at 10:17:57AM -0400, Robert Haas wrote:
> On Sat, Mar 12, 2022 at 9:11 AM Dmitry Dolgov <[email protected]> wrote:
> > Here is the limited version of list collapsing functionality, which
> > doesn't utilize eval_const_expressions and ignores most of the stuff
> > except ArrayExprs. Any thoughts/more suggestions?
>
> The proposed commit message says this commit intends to "Make Consts
> contribute nothing to the jumble hash if they're part of a series and
> at position further that specified threshold." I'm not sure whether
> that's what the patch actually implements because I can't immediately
> understand the new logic you've added, but I think if we did what that
> sentence said then, supposing the threshold is set to 1, it would
> result in producing the same hash for "x in (1,2)" that we do for "x
> in (1,3)" but a different hash for "x in (2,3)" which does not sound
> like what we want. What I would have thought we'd do is: if the list
> is all constants and long enough to satisfy the threshold then nothing
> in the list gets jumbled.
Well, yeah, the commit message is somewhat clumsy in this regard. It
works almost in the way you've described, except if the list is all
constants and long enough to satisfy the threshold then *first N
elements (where N == threshold) will be jumbled -- to leave at least
some traces of it in pgss.
> I'm a little surprised that there's not more context-awareness in this
> code. It seems that it applies to every ArrayExpr found in the query,
> which I think would extend to cases beyond something = IN(whatever).
> In particular, any use of ARRAY[] in the query would be impacted. Now,
> the comments seem to imply that's pretty intentional, but from the
> user's point of view, WHERE x in (1,3) and x = any(array[1,3]) are two
> different things. If anything like this is to be adopted, we certainly
> need to be precise about exactly what it is doing and which cases are
> covered.
I'm not sure if I follow the last point. WHERE x in (1,3) and x =
any(array[1,3]) are two different things for sure, but in which way are
they going to be mixed together because of this change? My goal was to
make only the following transformation, without leaving any uncertainty:
WHERE x in (1, 2, 3, 4, 5) -> WHERE x in (1, 2, ...)
WHERE x = any(array[1, 2, 3, 4, 5]) -> WHERE x = any(array[1, 2, ...])
> I thought of looking at the documentation to see whether you'd tried
> to clarify this there, and found that you hadn't written any.
>
> In short, I think this patch is not really very close to being in
> committable shape even if nobody were objecting to the concept.
Sure, I'll add documentation. To be honest I'm not targeting PG15 with
this, just want to make some progress. Thanks for the feedback, I'm glad
to see it coming!
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2022-03-14 14:57 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-06-06 22:42 [PATCH v4 2/3] Allow CREATE INDEX CONCURRENTLY on partitioned table Justin Pryzby <[email protected]>
2022-01-04 23:02 Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-01-05 04:37 ` Re: pg_stat_statements and "IN" conditions Andrey V. Lepikhov <[email protected]>
2022-01-05 05:13 ` Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-01-05 21:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 16:38 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-10 17:11 ` Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2022-03-10 17:32 ` Re: pg_stat_statements and "IN" conditions Robert Haas <[email protected]>
2022-03-10 19:06 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-12 14:10 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-03-14 14:17 ` Re: pg_stat_statements and "IN" conditions Robert Haas <[email protected]>
2022-03-14 14:57 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox