public inbox for [email protected]
help / color / mirror / Atom feedpg_stat_statements and "IN" conditions
38+ messages / 10 participants
[nested] [flat]
* pg_stat_statements and "IN" conditions
@ 2020-08-12 16:19 Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2020-08-12 16:19 UTC (permalink / raw)
To: pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
Hi
I would like to start another thread to follow up on [1], mostly to bump up the
topic. Just to remind, it's about how pg_stat_statements jumbling ArrayExpr in
queries like:
SELECT something FROM table WHERE col IN (1, 2, 3, ...)
The current implementation produces different jumble hash for every different
number of arguments for essentially the same query. Unfortunately a lot of ORMs
like to generate these types of queries, which in turn leads to
pg_stat_statements pollution. Ideally we want to prevent this and have only one
record for such a query.
As the result of [1] I've identified two highlighted approaches to improve this
situation:
* Reduce the generated ArrayExpr to an array Const immediately, in cases where
all the inputs are Consts.
* Make repeating Const to contribute nothing to the resulting hash.
I've tried to prototype both approaches to find out pros/cons and be more
specific. Attached patches could not be considered a completed piece of work,
but they seem to work, mostly pass the tests and demonstrate the point. I would
like to get some high level input about them and ideally make it clear what is
the preferred solution to continue with.
# Reducing ArrayExpr to an array Const
IIUC this requires producing a Const with ArrayType constvalue in
transformAExprIn for ScalarArrayOpExpr. This could be a general improvement,
since apparently it's being done later anyway. But it deals only with Const,
which leaves more on the table, e.g. Params and other similar types of
duplication we observe when repeating constants are wrapped into VALUES.
Another point here is that it's quite possible this approach will still require
corresponding changes in pg_stat_statements, since just preventing duplicates
to show also loses the information. Ideally we also need to have some
understanding how many elements are actually there and display it, e.g. in
cases when there is just one outlier query that contains a huge IN list.
# Contribute nothing to the hash
I guess there could be multiple ways of doing this, but the first idea I had in
mind is to skip jumbling when necessary. At the same time it can be implemented
more centralized for different types of queries (although in the attached patch
there are only Const & Values). In the simplest case we just identify sequence
of constants of the same type, which just ignores any other cases when stuff is
mixed. But I believe it's something that could be considered a rare corner case
and it's better to start with the simplest solution.
Having said that I believe the second approach of contributing nothing to the
hash sounds more appealing, but would love to hear other opinions.
[1]: https://www.postgresql.org/message-id/flat/CAF42k%3DJCfHMJtkAVXCzBn2XBxDC83xb4VhV7jU7enPnZ0CfEQQ%40m...
Attachments:
[application/octet-stream] 0001-Reduce-ArrayExpr-into-const-array.patch (3.4K, ../../CA+q6zcWtUbT_Sxj0V6HY6EZ89uv5wuG5aefpe_9n0Jr3VwntFg@mail.gmail.com/2-0001-Reduce-ArrayExpr-into-const-array.patch)
download | inline diff:
From 27973d144f592249e2b8d9a548be3d563f58b737 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <[email protected]>
Date: Mon, 10 Aug 2020 18:27:28 +0200
Subject: [PATCH] Reduce ArrayExpr into const array
In case if ArrayExpr generated for ScalarArrayOpExpr contains only
consts, reduce it to a const array. One of the advantages of that is
pg_stat_statements will consider this kind of array as a single entity
no matter how many elements are there, which will partially solve the
duplication problem (when the same query mentioned in pg_stat_statements
multiple times due to different number of elements in e.g. IN condition)
---
src/backend/parser/parse_expr.c | 65 ++++++++++++++++++++++++++++++---
1 file changed, 59 insertions(+), 6 deletions(-)
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index f69976cc8c..6aea674810 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -32,6 +32,7 @@
#include "parser/parse_relation.h"
#include "parser/parse_target.h"
#include "parser/parse_type.h"
+#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/date.h"
#include "utils/lsyscache.h"
@@ -1301,6 +1302,7 @@ transformAExprIn(ParseState *pstate, A_Expr *a)
*/
List *aexprs;
ArrayExpr *newa;
+ bool all_const = true;
aexprs = NIL;
foreach(l, rnonvars)
@@ -1310,6 +1312,10 @@ transformAExprIn(ParseState *pstate, A_Expr *a)
rexpr = coerce_to_common_type(pstate, rexpr,
scalar_type,
"IN");
+
+ if (!IsA(rexpr, Const))
+ all_const = false;
+
aexprs = lappend(aexprs, rexpr);
}
newa = makeNode(ArrayExpr);
@@ -1320,12 +1326,59 @@ transformAExprIn(ParseState *pstate, A_Expr *a)
newa->multidims = false;
newa->location = -1;
- result = (Node *) make_scalar_array_op(pstate,
- a->name,
- useOr,
- lexpr,
- (Node *) newa,
- a->location);
+ /* if all elements are const reduce ArrayExpr to a const as well */
+ if (all_const)
+ {
+ ArrayType *const_array;
+ Datum *elems = (Datum *) palloc(sizeof(Datum) * aexprs->length);
+ bool *nulls = (bool *) palloc(sizeof(bool) * aexprs->length);
+
+ int dims[1];
+ int lbs[1];
+
+ bool elembyval;
+ int16 elemlen;
+ char elemalign;
+ int i = 0;
+
+ foreach(l, aexprs)
+ {
+ Const *expr = (Const *) lfirst(l);
+
+ elems[i] = expr->constvalue;
+ nulls[i] = expr->constisnull;
+ i++;
+ }
+
+ /* ArrayExpr would have only one dimention */
+ dims[0] = aexprs->length;
+ lbs[0] = 1;
+
+ get_typlenbyvalalign(scalar_type, &elemlen,
+ &elembyval, &elemalign);
+ const_array = construct_md_array(elems, nulls, 1, dims, lbs,
+ scalar_type, elemlen,
+ elembyval, elemalign);
+
+ result = (Node *) makeConst(array_type, -1,
+ newa->array_collid, -1,
+ PointerGetDatum(const_array),
+ false, false);
+
+ result = (Node *) make_scalar_array_op(pstate,
+ a->name,
+ useOr,
+ lexpr,
+ (Node *) result,
+ a->location);
+ }
+ else
+ result = (Node *) make_scalar_array_op(pstate,
+ a->name,
+ useOr,
+ lexpr,
+ (Node *) newa,
+ a->location);
/* Consider only the Vars (if any) in the loop below */
rexprs = rvars;
--
2.21.0
[application/octet-stream] 0001-Limit-jumbling-for-repeating-constants.patch (4.6K, ../../CA+q6zcWtUbT_Sxj0V6HY6EZ89uv5wuG5aefpe_9n0Jr3VwntFg@mail.gmail.com/3-0001-Limit-jumbling-for-repeating-constants.patch)
download | inline diff:
From fd8c3139151179615ac01a972bf30970b76cd24a Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <[email protected]>
Date: Mon, 20 Jul 2020 17:11:14 +0200
Subject: [PATCH] Limit jumbling for repeating constants
In case if in pg_stat_statements we found an ArrayExpr or RTE_VALUES
containing only constants, skip jumbling for those with position higher
than a predefined threshold. This helps to avoid problem of duplicated
statements, when the same query is repeated multiple times due to
different number of arguments in the expression, e.g.
WHERE col IN (1, 2, 3, ...)
---
.../pg_stat_statements/pg_stat_statements.c | 110 +++++++++++++++++-
1 file changed, 108 insertions(+), 2 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 14cad19afb..c96bcade81 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -115,6 +115,8 @@ static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100;
#define JUMBLE_SIZE 1024 /* query serialization buffer size */
+#define MERGE_THRESHOLD 5 /* after which number of similar nodes
+ start to merge then if asked */
/*
* Extension version number, for supporting older extension versions' objects
*/
@@ -374,6 +376,7 @@ static void JumbleQuery(pgssJumbleState *jstate, Query *query);
static void JumbleRangeTable(pgssJumbleState *jstate, List *rtable);
static void JumbleRowMarks(pgssJumbleState *jstate, List *rowMarks);
static void JumbleExpr(pgssJumbleState *jstate, Node *node);
+static bool JumbleExprList(pgssJumbleState *jstate, Node *node);
static void RecordConstLocation(pgssJumbleState *jstate, int location);
static char *generate_normalized_query(pgssJumbleState *jstate, const char *query,
int query_loc, int *query_len_p, int encoding);
@@ -2647,7 +2650,7 @@ JumbleRangeTable(pgssJumbleState *jstate, List *rtable)
JumbleExpr(jstate, (Node *) rte->tablefunc);
break;
case RTE_VALUES:
- JumbleExpr(jstate, (Node *) rte->values_lists);
+ JumbleExprList(jstate, (Node *) rte->values_lists);
break;
case RTE_CTE:
@@ -2691,6 +2694,109 @@ JumbleRowMarks(pgssJumbleState *jstate, List *rowMarks)
}
}
+static bool
+JumbleExprList(pgssJumbleState *jstate, Node *node)
+{
+ ListCell *temp;
+ Node *firstExpr = NULL;
+ bool merged = false;
+ bool allConst = true;
+ int currentExprIdx;
+
+ if (node == NULL)
+ return merged;
+
+ Assert(IsA(node, List));
+ firstExpr = (Node *) lfirst(list_head((List *) node));
+
+ /* Guard against stack overflow due to overly complex expressions */
+ check_stack_depth();
+
+ /*
+ * 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 *) firstExpr)
+ {
+ Node * subExpr = (Node *) lfirst(temp);
+
+ if (!IsA(subExpr, Const))
+ {
+ allConst = false;
+ break;
+ }
+ }
+
+ foreach(temp, (List *) node)
+ {
+ Node *expr = (Node *) lfirst(temp);
+ ListCell *lc;
+
+ foreach(lc, (List *) expr)
+ {
+ Node * subExpr = (Node *) lfirst(lc);
+
+ if (!IsA(subExpr, Const))
+ {
+ allConst = false;
+ break;
+ }
+ }
+
+ if (!equal(expr, firstExpr) && allConst &&
+ currentExprIdx > MERGE_THRESHOLD)
+ {
+ merged = true;
+ continue;
+ }
+
+ JumbleExpr(jstate, expr);
+ currentExprIdx++;
+ }
+ break;
+
+ case T_Const:
+ currentExprIdx = 0;
+
+ foreach(temp, (List *) node)
+ {
+ Node *expr = (Node *) lfirst(temp);
+
+ if (!equal(expr, firstExpr) && IsA(expr, Const) &&
+ currentExprIdx > MERGE_THRESHOLD)
+ {
+ merged = true;
+ continue;
+ }
+
+ JumbleExpr(jstate, expr);
+ currentExprIdx++;
+ }
+ break;
+
+ default:
+ foreach(temp, (List *) node)
+ {
+ JumbleExpr(jstate, (Node *) lfirst(temp));
+ }
+ break;
+ }
+
+ return merged;
+}
+
/*
* Jumble an expression tree
*
@@ -2928,7 +3034,7 @@ JumbleExpr(pgssJumbleState *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);
--
2.21.0
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2020-11-18 16:04 ` Dmitry Dolgov <[email protected]>
2020-12-09 03:37 ` Re: pg_stat_statements and "IN" conditions Chengxi Sun <[email protected]>
2020-12-26 10:46 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 2 replies; 38+ messages in thread
From: Dmitry Dolgov @ 2020-11-18 16:04 UTC (permalink / raw)
To: pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Wed, Aug 12, 2020 at 06:19:02PM +0200, Dmitry Dolgov wrote:
>
> I would like to start another thread to follow up on [1], mostly to bump up the
> topic. Just to remind, it's about how pg_stat_statements jumbling ArrayExpr in
> queries like:
>
> SELECT something FROM table WHERE col IN (1, 2, 3, ...)
>
> The current implementation produces different jumble hash for every different
> number of arguments for essentially the same query. Unfortunately a lot of ORMs
> like to generate these types of queries, which in turn leads to
> pg_stat_statements pollution. Ideally we want to prevent this and have only one
> record for such a query.
>
> As the result of [1] I've identified two highlighted approaches to improve this
> situation:
>
> * Reduce the generated ArrayExpr to an array Const immediately, in cases where
> all the inputs are Consts.
>
> * Make repeating Const to contribute nothing to the resulting hash.
>
> I've tried to prototype both approaches to find out pros/cons and be more
> specific. Attached patches could not be considered a completed piece of work,
> but they seem to work, mostly pass the tests and demonstrate the point. I would
> like to get some high level input about them and ideally make it clear what is
> the preferred solution to continue with.
I've implemented the second approach mentioned above, this version was
tested on our test clusters for some time without visible issues. Will
create a CF item and would appreciate any feedback.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2020-12-09 03:37 ` Chengxi Sun <[email protected]>
2020-12-09 18:49 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
1 sibling, 1 reply; 38+ messages in thread
From: Chengxi Sun @ 2020-12-09 03:37 UTC (permalink / raw)
To: [email protected]; +Cc: Dmitry Dolgov <[email protected]>
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Hi, I did some test and it works well
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-09 03:37 ` Re: pg_stat_statements and "IN" conditions Chengxi Sun <[email protected]>
@ 2020-12-09 18:49 ` Dmitry Dolgov <[email protected]>
0 siblings, 0 replies; 38+ messages in thread
From: Dmitry Dolgov @ 2020-12-09 18:49 UTC (permalink / raw)
To: Chengxi Sun <[email protected]>; +Cc: [email protected]
> On Wed, Dec 09, 2020 at 03:37:40AM +0000, Chengxi Sun wrote:
>
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: tested, passed
> Spec compliant: not tested
> Documentation: not tested
>
> Hi, I did some test and it works well
Thanks for testing!
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2020-12-26 10:46 ` Dmitry Dolgov <[email protected]>
2020-12-26 16:53 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
1 sibling, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2020-12-26 10:46 UTC (permalink / raw)
To: pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Wed, Nov 18, 2020 at 05:04:32PM +0100, Dmitry Dolgov wrote:
> > On Wed, Aug 12, 2020 at 06:19:02PM +0200, Dmitry Dolgov wrote:
> >
> > I would like to start another thread to follow up on [1], mostly to bump up the
> > topic. Just to remind, it's about how pg_stat_statements jumbling ArrayExpr in
> > queries like:
> >
> > SELECT something FROM table WHERE col IN (1, 2, 3, ...)
> >
> > The current implementation produces different jumble hash for every different
> > number of arguments for essentially the same query. Unfortunately a lot of ORMs
> > like to generate these types of queries, which in turn leads to
> > pg_stat_statements pollution. Ideally we want to prevent this and have only one
> > record for such a query.
> >
> > As the result of [1] I've identified two highlighted approaches to improve this
> > situation:
> >
> > * Reduce the generated ArrayExpr to an array Const immediately, in cases where
> > all the inputs are Consts.
> >
> > * Make repeating Const to contribute nothing to the resulting hash.
> >
> > I've tried to prototype both approaches to find out pros/cons and be more
> > specific. Attached patches could not be considered a completed piece of work,
> > but they seem to work, mostly pass the tests and demonstrate the point. I would
> > like to get some high level input about them and ideally make it clear what is
> > the preferred solution to continue with.
>
> I've implemented the second approach mentioned above, this version was
> tested on our test clusters for some time without visible issues. Will
> create a CF item and would appreciate any feedback.
After more testing I found couple of things that could be improved,
namely in the presence of non-reducible constants one part of the query
was not copied into the normalized version, and this approach also could
be extended for Params. These are incorporated in the attached patch.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 10:46 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2020-12-26 16:53 ` Zhihong Yu <[email protected]>
2021-01-05 12:52 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Zhihong Yu @ 2020-12-26 16:53 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
Hi,
A few comments.
+ "After this number of duplicating constants
start to merge them.",
duplicating -> duplicate
+ foreach(lc, (List *) expr)
+ {
+ Node * subExpr = (Node *) lfirst(lc);
+
+ if (!IsA(subExpr, Const))
+ {
+ allConst = false;
+ break;
+ }
+ }
It seems the above foreach loop (within foreach(temp, (List *) node)) can
be preceded with a check that allConst is true. Otherwise the loop can be
skipped.
+ if (currentExprIdx == pgss_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 > 0);
+ jstate->clocations[jstate->clocations_count -
1].merged = true;
+ currentExprIdx++;
+ }
The above snippet occurs a few times. Maybe extract into a helper method.
Cheers
On Sat, Dec 26, 2020 at 2:45 AM Dmitry Dolgov <[email protected]> wrote:
> > On Wed, Nov 18, 2020 at 05:04:32PM +0100, Dmitry Dolgov wrote:
> > > On Wed, Aug 12, 2020 at 06:19:02PM +0200, Dmitry Dolgov wrote:
> > >
> > > I would like to start another thread to follow up on [1], mostly to
> bump up the
> > > topic. Just to remind, it's about how pg_stat_statements jumbling
> ArrayExpr in
> > > queries like:
> > >
> > > SELECT something FROM table WHERE col IN (1, 2, 3, ...)
> > >
> > > The current implementation produces different jumble hash for every
> different
> > > number of arguments for essentially the same query. Unfortunately a
> lot of ORMs
> > > like to generate these types of queries, which in turn leads to
> > > pg_stat_statements pollution. Ideally we want to prevent this and have
> only one
> > > record for such a query.
> > >
> > > As the result of [1] I've identified two highlighted approaches to
> improve this
> > > situation:
> > >
> > > * Reduce the generated ArrayExpr to an array Const immediately, in
> cases where
> > > all the inputs are Consts.
> > >
> > > * Make repeating Const to contribute nothing to the resulting hash.
> > >
> > > I've tried to prototype both approaches to find out pros/cons and be
> more
> > > specific. Attached patches could not be considered a completed piece
> of work,
> > > but they seem to work, mostly pass the tests and demonstrate the
> point. I would
> > > like to get some high level input about them and ideally make it clear
> what is
> > > the preferred solution to continue with.
> >
> > I've implemented the second approach mentioned above, this version was
> > tested on our test clusters for some time without visible issues. Will
> > create a CF item and would appreciate any feedback.
>
> After more testing I found couple of things that could be improved,
> namely in the presence of non-reducible constants one part of the query
> was not copied into the normalized version, and this approach also could
> be extended for Params. These are incorporated in the attached patch.
>
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 10:46 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 16:53 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
@ 2021-01-05 12:52 ` Dmitry Dolgov <[email protected]>
2021-01-05 15:51 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2021-01-05 12:52 UTC (permalink / raw)
To: Zhihong Yu <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Sat, Dec 26, 2020 at 08:53:28AM -0800, Zhihong Yu wrote:
> Hi,
> A few comments.
>
> + foreach(lc, (List *) expr)
> + {
> + Node * subExpr = (Node *) lfirst(lc);
> +
> + if (!IsA(subExpr, Const))
> + {
> + allConst = false;
> + break;
> + }
> + }
>
> It seems the above foreach loop (within foreach(temp, (List *) node)) can
> be preceded with a check that allConst is true. Otherwise the loop can be
> skipped.
Thanks for noticing. Now that I look at it closer I think it's the other
way around, the loop above checking constants for the first expression
is not really necessary.
> + if (currentExprIdx == pgss_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 > 0);
> + jstate->clocations[jstate->clocations_count -
> 1].merged = true;
> + currentExprIdx++;
> + }
>
> The above snippet occurs a few times. Maybe extract into a helper method.
Originally I was hesitant to extract it was because it's quite small
part of the code. But now I've realized that the part relevant to lists
is not really correct, which makes those bits even more different, so I
think it makes sense to leave it like that. What do you think?
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 10:46 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 16:53 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-01-05 12:52 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2021-01-05 15:51 ` Zhihong Yu <[email protected]>
2021-03-18 13:38 ` Re: pg_stat_statements and "IN" conditions David Steele <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Zhihong Yu @ 2021-01-05 15:51 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
Hi, Dmitry:
+ int lastExprLenght = 0;
Did you mean to name the variable lastExprLenghth ?
w.r.t. extracting to helper method, the second and third if (currentExprIdx
== pgss_merge_threshold - 1) blocks are similar.
It is up to you whether to create the helper method.
I am fine with the current formation.
Cheers
On Tue, Jan 5, 2021 at 4:51 AM Dmitry Dolgov <[email protected]> wrote:
> > On Sat, Dec 26, 2020 at 08:53:28AM -0800, Zhihong Yu wrote:
> > Hi,
> > A few comments.
> >
> > + foreach(lc, (List *) expr)
> > + {
> > + Node * subExpr = (Node *) lfirst(lc);
> > +
> > + if (!IsA(subExpr, Const))
> > + {
> > + allConst = false;
> > + break;
> > + }
> > + }
> >
> > It seems the above foreach loop (within foreach(temp, (List *) node)) can
> > be preceded with a check that allConst is true. Otherwise the loop can be
> > skipped.
>
> Thanks for noticing. Now that I look at it closer I think it's the other
> way around, the loop above checking constants for the first expression
> is not really necessary.
>
> > + if (currentExprIdx == pgss_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 > 0);
> > + jstate->clocations[jstate->clocations_count -
> > 1].merged = true;
> > + currentExprIdx++;
> > + }
> >
> > The above snippet occurs a few times. Maybe extract into a helper method.
>
> Originally I was hesitant to extract it was because it's quite small
> part of the code. But now I've realized that the part relevant to lists
> is not really correct, which makes those bits even more different, so I
> think it makes sense to leave it like that. What do you think?
>
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 10:46 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 16:53 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-01-05 12:52 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-01-05 15:51 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
@ 2021-03-18 13:38 ` David Steele <[email protected]>
2021-03-18 15:50 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: David Steele @ 2021-03-18 13:38 UTC (permalink / raw)
To: Zhihong Yu <[email protected]>; Dmitry Dolgov <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
On 1/5/21 10:51 AM, Zhihong Yu wrote:
>
> + int lastExprLenght = 0;
>
> Did you mean to name the variable lastExprLenghth ?
>
> w.r.t. extracting to helper method, the second and third
> if (currentExprIdx == pgss_merge_threshold - 1) blocks are similar.
> It is up to you whether to create the helper method.
> I am fine with the current formation.
Dmitry, thoughts on this review?
Regards,
--
-David
[email protected]
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 10:46 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 16:53 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-01-05 12:52 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-01-05 15:51 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-03-18 13:38 ` Re: pg_stat_statements and "IN" conditions David Steele <[email protected]>
@ 2021-03-18 15:50 ` Dmitry Dolgov <[email protected]>
2021-06-15 15:18 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2021-03-18 15:50 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Zhihong Yu <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Thu, Mar 18, 2021 at 09:38:09AM -0400, David Steele wrote:
> On 1/5/21 10:51 AM, Zhihong Yu wrote:
> >
> > + int lastExprLenght = 0;
> >
> > Did you mean to name the variable lastExprLenghth ?
> >
> > w.r.t. extracting to helper method, the second and third
> > if (currentExprIdx == pgss_merge_threshold - 1) blocks are similar.
> > It is up to you whether to create the helper method.
> > I am fine with the current formation.
>
> Dmitry, thoughts on this review?
Oh, right. lastExprLenghth is obviously a typo, and as we agreed that
the helper is not strictly necessary I wanted to wait a bit hoping for
more feedback and eventually to post an accumulated patch. Doesn't make
sense to post another version only to fix one typo :)
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 10:46 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 16:53 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-01-05 12:52 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-01-05 15:51 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-03-18 13:38 ` Re: pg_stat_statements and "IN" conditions David Steele <[email protected]>
2021-03-18 15:50 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2021-06-15 15:18 ` Dmitry Dolgov <[email protected]>
2021-06-16 14:02 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2021-06-15 15:18 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Zhihong Yu <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Thu, Mar 18, 2021 at 04:50:02PM +0100, Dmitry Dolgov wrote:
> > On Thu, Mar 18, 2021 at 09:38:09AM -0400, David Steele wrote:
> > On 1/5/21 10:51 AM, Zhihong Yu wrote:
> > >
> > > + int lastExprLenght = 0;
> > >
> > > Did you mean to name the variable lastExprLenghth ?
> > >
> > > w.r.t. extracting to helper method, the second and third
> > > if (currentExprIdx == pgss_merge_threshold - 1) blocks are similar.
> > > It is up to you whether to create the helper method.
> > > I am fine with the current formation.
> >
> > Dmitry, thoughts on this review?
>
> Oh, right. lastExprLenghth is obviously a typo, and as we agreed that
> the helper is not strictly necessary I wanted to wait a bit hoping for
> more feedback and eventually to post an accumulated patch. Doesn't make
> sense to post another version only to fix one typo :)
Hi,
I've prepared a new rebased version to deal with the new way of
computing query id, but as always there is one tricky part. From what I
understand, now an external module can provide custom implementation for
query id computation algorithm. It seems natural to think this machinery
could be used instead of patch in the thread, i.e. one could create a
custom logic that will enable constants collapsing as needed, so that
same queries with different number of constants in an array will be
hashed into the same record.
But there is a limitation in how such queries will be normalized
afterwards — to reduce level of surprise it's necessary to display the
fact that a certain query in fact had more constants that are showed in
pgss record. Ideally LocationLen needs to carry some bits of information
on what exactly could be skipped, and generate_normalized_query needs to
understand that, both are not reachable for an external module with
custom query id logic (without replicating significant part of the
existing code). Hence, a new version of the patch.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 10:46 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 16:53 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-01-05 12:52 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-01-05 15:51 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-03-18 13:38 ` Re: pg_stat_statements and "IN" conditions David Steele <[email protected]>
2021-03-18 15:50 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-06-15 15:18 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2021-06-16 14:02 ` Dmitry Dolgov <[email protected]>
2021-09-30 13:49 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2021-06-16 14:02 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Zhihong Yu <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Tue, Jun 15, 2021 at 05:18:50PM +0200, Dmitry Dolgov wrote:
> > On Thu, Mar 18, 2021 at 04:50:02PM +0100, Dmitry Dolgov wrote:
> > > On Thu, Mar 18, 2021 at 09:38:09AM -0400, David Steele wrote:
> > > On 1/5/21 10:51 AM, Zhihong Yu wrote:
> > > >
> > > > + int lastExprLenght = 0;
> > > >
> > > > Did you mean to name the variable lastExprLenghth ?
> > > >
> > > > w.r.t. extracting to helper method, the second and third
> > > > if (currentExprIdx == pgss_merge_threshold - 1) blocks are similar.
> > > > It is up to you whether to create the helper method.
> > > > I am fine with the current formation.
> > >
> > > Dmitry, thoughts on this review?
> >
> > Oh, right. lastExprLenghth is obviously a typo, and as we agreed that
> > the helper is not strictly necessary I wanted to wait a bit hoping for
> > more feedback and eventually to post an accumulated patch. Doesn't make
> > sense to post another version only to fix one typo :)
>
> Hi,
>
> I've prepared a new rebased version to deal with the new way of
> computing query id, but as always there is one tricky part. From what I
> understand, now an external module can provide custom implementation for
> query id computation algorithm. It seems natural to think this machinery
> could be used instead of patch in the thread, i.e. one could create a
> custom logic that will enable constants collapsing as needed, so that
> same queries with different number of constants in an array will be
> hashed into the same record.
>
> But there is a limitation in how such queries will be normalized
> afterwards — to reduce level of surprise it's necessary to display the
> fact that a certain query in fact had more constants that are showed in
> pgss record. Ideally LocationLen needs to carry some bits of information
> on what exactly could be skipped, and generate_normalized_query needs to
> understand that, both are not reachable for an external module with
> custom query id logic (without replicating significant part of the
> existing code). Hence, a new version of the patch.
Forgot to mention a couple of people who already reviewed the patch.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 10:46 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 16:53 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-01-05 12:52 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-01-05 15:51 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-03-18 13:38 ` Re: pg_stat_statements and "IN" conditions David Steele <[email protected]>
2021-03-18 15:50 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-06-15 15:18 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-06-16 14:02 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2021-09-30 13:49 ` Dmitry Dolgov <[email protected]>
2021-09-30 15:03 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2021-09-30 13:49 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Zhihong Yu <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
>On Wed, Jun 16, 2021 at 04:02:12PM +0200, Dmitry Dolgov wrote:
>
> > I've prepared a new rebased version to deal with the new way of
> > computing query id, but as always there is one tricky part. From what I
> > understand, now an external module can provide custom implementation for
> > query id computation algorithm. It seems natural to think this machinery
> > could be used instead of patch in the thread, i.e. one could create a
> > custom logic that will enable constants collapsing as needed, so that
> > same queries with different number of constants in an array will be
> > hashed into the same record.
> >
> > But there is a limitation in how such queries will be normalized
> > afterwards — to reduce level of surprise it's necessary to display the
> > fact that a certain query in fact had more constants that are showed in
> > pgss record. Ideally LocationLen needs to carry some bits of information
> > on what exactly could be skipped, and generate_normalized_query needs to
> > understand that, both are not reachable for an external module with
> > custom query id logic (without replicating significant part of the
> > existing code). Hence, a new version of the patch.
>
> Forgot to mention a couple of people who already reviewed the patch.
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).
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 10:46 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 16:53 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-01-05 12:52 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-01-05 15:51 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-03-18 13:38 ` Re: pg_stat_statements and "IN" conditions David Steele <[email protected]>
2021-03-18 15:50 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-06-15 15:18 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-06-16 14:02 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-09-30 13:49 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2021-09-30 15:03 ` Zhihong Yu <[email protected]>
2021-09-30 15:09 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Zhihong Yu @ 2021-09-30 15:03 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: David Steele <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
On Thu, Sep 30, 2021 at 6:49 AM Dmitry Dolgov <[email protected]> wrote:
> >On Wed, Jun 16, 2021 at 04:02:12PM +0200, Dmitry Dolgov wrote:
> >
> > > I've prepared a new rebased version to deal with the new way of
> > > computing query id, but as always there is one tricky part. From what I
> > > understand, now an external module can provide custom implementation
> for
> > > query id computation algorithm. It seems natural to think this
> machinery
> > > could be used instead of patch in the thread, i.e. one could create a
> > > custom logic that will enable constants collapsing as needed, so that
> > > same queries with different number of constants in an array will be
> > > hashed into the same record.
> > >
> > > But there is a limitation in how such queries will be normalized
> > > afterwards — to reduce level of surprise it's necessary to display the
> > > fact that a certain query in fact had more constants that are showed in
> > > pgss record. Ideally LocationLen needs to carry some bits of
> information
> > > on what exactly could be skipped, and generate_normalized_query needs
> to
> > > understand that, both are not reachable for an external module with
> > > custom query id logic (without replicating significant part of the
> > > existing code). Hence, a new version of the patch.
> >
> > Forgot to mention a couple of people who already reviewed the patch.
>
> 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).
>
Hi,
bq. and at position further that specified threshold.
that specified threshold -> than specified threshold
Cheers
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 10:46 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-12-26 16:53 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-01-05 12:52 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-01-05 15:51 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
2021-03-18 13:38 ` Re: pg_stat_statements and "IN" conditions David Steele <[email protected]>
2021-03-18 15:50 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-06-15 15:18 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-06-16 14:02 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-09-30 13:49 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2021-09-30 15:03 ` Re: pg_stat_statements and "IN" conditions Zhihong Yu <[email protected]>
@ 2021-09-30 15:09 ` Dmitry Dolgov <[email protected]>
0 siblings, 0 replies; 38+ messages in thread
From: Dmitry Dolgov @ 2021-09-30 15:09 UTC (permalink / raw)
To: Zhihong Yu <[email protected]>; +Cc: David Steele <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Thu, Sep 30, 2021 at 08:03:16AM -0700, Zhihong Yu wrote:
> On Thu, Sep 30, 2021 at 6:49 AM Dmitry Dolgov <[email protected]> wrote:
>
> > >On Wed, Jun 16, 2021 at 04:02:12PM +0200, Dmitry Dolgov wrote:
> > >
> > > > I've prepared a new rebased version to deal with the new way of
> > > > computing query id, but as always there is one tricky part. From what I
> > > > understand, now an external module can provide custom implementation
> > for
> > > > query id computation algorithm. It seems natural to think this
> > machinery
> > > > could be used instead of patch in the thread, i.e. one could create a
> > > > custom logic that will enable constants collapsing as needed, so that
> > > > same queries with different number of constants in an array will be
> > > > hashed into the same record.
> > > >
> > > > But there is a limitation in how such queries will be normalized
> > > > afterwards — to reduce level of surprise it's necessary to display the
> > > > fact that a certain query in fact had more constants that are showed in
> > > > pgss record. Ideally LocationLen needs to carry some bits of
> > information
> > > > on what exactly could be skipped, and generate_normalized_query needs
> > to
> > > > understand that, both are not reachable for an external module with
> > > > custom query id logic (without replicating significant part of the
> > > > existing code). Hence, a new version of the patch.
> > >
> > > Forgot to mention a couple of people who already reviewed the patch.
> >
> > 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).
> >
> Hi,
>
> bq. and at position further that specified threshold.
>
> that specified threshold -> than specified threshold
You mean in the patch commit message, nowhere else, right? Yep, my spell
checker didn't catch that, thanks for noticing!
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
@ 2022-03-26 17:40 Dmitry Dolgov <[email protected]>
2022-07-24 10:06 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2022-03-26 17:40 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[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 04:51:50PM +0100, Dmitry Dolgov wrote:
> > On Mon, Mar 14, 2022 at 11:38:23AM -0400, Tom Lane wrote:
> > Dmitry Dolgov <[email protected]> writes:
> > > On Mon, Mar 14, 2022 at 11:23:17AM -0400, Tom Lane wrote:
> > >> I do find it odd that the proposed patch doesn't cause the *entire*
> > >> list to be skipped over. That seems like extra complexity and confusion
> > >> to no benefit.
> >
> > > That's a bit surprising for me, I haven't even thought that folks could
> > > think this is an odd behaviour. As I've mentioned above, the original
> > > idea was to give some clues about what was inside the collapsed array,
> > > but if everyone finds it unnecessary I can of course change it.
> >
> > But if what we're doing is skipping over an all-Consts list, then the
> > individual Consts would be elided from the pg_stat_statements entry
> > anyway, no? All that would remain is information about how many such
> > Consts there were, which is exactly the information you want to drop.
>
> Hm, yes, you're right. I guess I was thinking about this more like about
> shortening some text with ellipsis, but indeed no actual Consts will end
> up in the result anyway. Thanks for clarification, will modify the
> patch!
Here is another iteration. Now the patch doesn't leave any trailing
Consts in the normalized query, and contains more documentation. I hope
it's getting better.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-03-26 17:40 Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2022-07-24 10:06 ` Dmitry Dolgov <[email protected]>
2022-09-16 18:25 ` Re:pg_stat_statements and "IN" conditions Sergei Kornilov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2022-07-24 10:06 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>
> On Sat, Mar 26, 2022 at 06:40:35PM +0100, Dmitry Dolgov wrote:
> > On Mon, Mar 14, 2022 at 04:51:50PM +0100, Dmitry Dolgov wrote:
> > > On Mon, Mar 14, 2022 at 11:38:23AM -0400, Tom Lane wrote:
> > > Dmitry Dolgov <[email protected]> writes:
> > > > On Mon, Mar 14, 2022 at 11:23:17AM -0400, Tom Lane wrote:
> > > >> I do find it odd that the proposed patch doesn't cause the *entire*
> > > >> list to be skipped over. That seems like extra complexity and confusion
> > > >> to no benefit.
> > >
> > > > That's a bit surprising for me, I haven't even thought that folks could
> > > > think this is an odd behaviour. As I've mentioned above, the original
> > > > idea was to give some clues about what was inside the collapsed array,
> > > > but if everyone finds it unnecessary I can of course change it.
> > >
> > > But if what we're doing is skipping over an all-Consts list, then the
> > > individual Consts would be elided from the pg_stat_statements entry
> > > anyway, no? All that would remain is information about how many such
> > > Consts there were, which is exactly the information you want to drop.
> >
> > Hm, yes, you're right. I guess I was thinking about this more like about
> > shortening some text with ellipsis, but indeed no actual Consts will end
> > up in the result anyway. Thanks for clarification, will modify the
> > patch!
>
> Here is another iteration. Now the patch doesn't leave any trailing
> Consts in the normalized query, and contains more documentation. I hope
> it's getting better.
Hi,
Here is the rebased version, with no other changes.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re:pg_stat_statements and "IN" conditions
2022-03-26 17:40 Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-07-24 10:06 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2022-09-16 18:25 ` Sergei Kornilov <[email protected]>
2022-09-24 14:07 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Sergei Kornilov @ 2022-09-16 18:25 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Robert Haas <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>; Tom Lane <[email protected]>
Hello!
Unfortunately the patch needs another rebase due to the recent split of guc.c (0a20ff54f5e66158930d5328f89f087d4e9ab400)
I'm reviewing a patch on top of a previous commit and noticed a failed test:
# Failed test 'no parameters missing from postgresql.conf.sample'
# at t/003_check_guc.pl line 82.
# got: '1'
# expected: '0'
# Looks like you failed 1 test of 3.
t/003_check_guc.pl ..............
The new option has not been added to the postgresql.conf.sample
PS: I would also like to have such a feature. It's hard to increase pg_stat_statements.max or lose some entries just because some ORM sends requests with a different number of parameters.
regards, Sergei
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-03-26 17:40 Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-07-24 10:06 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-09-16 18:25 ` Re:pg_stat_statements and "IN" conditions Sergei Kornilov <[email protected]>
@ 2022-09-24 14:07 ` Dmitry Dolgov <[email protected]>
2022-09-24 23:59 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2022-09-24 14:07 UTC (permalink / raw)
To: Sergei Kornilov <[email protected]>; +Cc: Robert Haas <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>; Tom Lane <[email protected]>
> On Fri, Sep 16, 2022 at 09:25:13PM +0300, Sergei Kornilov wrote:
> Hello!
>
> Unfortunately the patch needs another rebase due to the recent split of guc.c (0a20ff54f5e66158930d5328f89f087d4e9ab400)
>
> I'm reviewing a patch on top of a previous commit and noticed a failed test:
>
> # Failed test 'no parameters missing from postgresql.conf.sample'
> # at t/003_check_guc.pl line 82.
> # got: '1'
> # expected: '0'
> # Looks like you failed 1 test of 3.
> t/003_check_guc.pl ..............
>
> The new option has not been added to the postgresql.conf.sample
>
> PS: I would also like to have such a feature. It's hard to increase pg_stat_statements.max or lose some entries just because some ORM sends requests with a different number of parameters.
Thanks! I'll post the rebased version soon.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2022-03-26 17:40 Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-07-24 10:06 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-09-16 18:25 ` Re:pg_stat_statements and "IN" conditions Sergei Kornilov <[email protected]>
2022-09-24 14:07 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2022-09-24 23:59 ` Dmitry Dolgov <[email protected]>
0 siblings, 0 replies; 38+ messages in thread
From: Dmitry Dolgov @ 2022-09-24 23:59 UTC (permalink / raw)
To: Sergei Kornilov <[email protected]>; +Cc: Robert Haas <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Greg Stark <[email protected]>; Pavel Trukhanov <[email protected]>; Tom Lane <[email protected]>
> On Sat, Sep 24, 2022 at 04:07:14PM +0200, Dmitry Dolgov wrote:
> > On Fri, Sep 16, 2022 at 09:25:13PM +0300, Sergei Kornilov wrote:
> > Hello!
> >
> > Unfortunately the patch needs another rebase due to the recent split of guc.c (0a20ff54f5e66158930d5328f89f087d4e9ab400)
> >
> > I'm reviewing a patch on top of a previous commit and noticed a failed test:
> >
> > # Failed test 'no parameters missing from postgresql.conf.sample'
> > # at t/003_check_guc.pl line 82.
> > # got: '1'
> > # expected: '0'
> > # Looks like you failed 1 test of 3.
> > t/003_check_guc.pl ..............
> >
> > The new option has not been added to the postgresql.conf.sample
> >
> > PS: I would also like to have such a feature. It's hard to increase pg_stat_statements.max or lose some entries just because some ORM sends requests with a different number of parameters.
>
> Thanks! I'll post the rebased version soon.
And here it is.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
@ 2024-01-22 06:33 Peter Smith <[email protected]>
2024-01-22 16:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Peter Smith @ 2024-01-22 06:33 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: vignesh C <[email protected]>; Michael Paquier <[email protected]>; Nathan Bossart <[email protected]>; Gregory Stark (as CFM) <[email protected]>; David Geier <[email protected]>; Sergei Kornilov <[email protected]>; Alvaro Herrera <[email protected]>; Marcos Pegoraro <[email protected]>; Robert Haas <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Pavel Trukhanov <[email protected]>; Tom Lane <[email protected]>
2024-01 Commitfest.
Hi, This patch has a CF status of "Needs Review" [1], but it seems
there was a CFbot test failure last time it was run [2]. Please have a
look and post an updated version if necessary.
======
[1] https://commitfest.postgresql.org/46/2837/
[2] https://cirrus-ci.com/task/6688578378399744
Kind Regards,
Peter Smith.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2024-01-22 06:33 Re: pg_stat_statements and "IN" conditions Peter Smith <[email protected]>
@ 2024-01-22 16:11 ` Dmitry Dolgov <[email protected]>
2024-01-22 16:35 ` Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2024-01-22 16:11 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: vignesh C <[email protected]>; Michael Paquier <[email protected]>; Nathan Bossart <[email protected]>; Gregory Stark (as CFM) <[email protected]>; David Geier <[email protected]>; Sergei Kornilov <[email protected]>; Alvaro Herrera <[email protected]>; Marcos Pegoraro <[email protected]>; Robert Haas <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Pavel Trukhanov <[email protected]>; Tom Lane <[email protected]>
> On Mon, Jan 22, 2024 at 05:33:26PM +1100, Peter Smith wrote:
> 2024-01 Commitfest.
>
> Hi, This patch has a CF status of "Needs Review" [1], but it seems
> there was a CFbot test failure last time it was run [2]. Please have a
> look and post an updated version if necessary.
>
> ======
> [1] https://commitfest.postgresql.org/46/2837/
> [2] https://cirrus-ci.com/task/6688578378399744
It's the same failing pipeline Vignesh C was talking above. I've fixed
the issue in the latest patch version, but looks like it wasn't picked
up yet (from what I understand, the latest build for this CF is 8 weeks
old).
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2024-01-22 06:33 Re: pg_stat_statements and "IN" conditions Peter Smith <[email protected]>
2024-01-22 16:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2024-01-22 16:35 ` Tom Lane <[email protected]>
2024-01-22 17:07 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Tom Lane @ 2024-01-22 16:35 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Peter Smith <[email protected]>; vignesh C <[email protected]>; Michael Paquier <[email protected]>; Nathan Bossart <[email protected]>; Gregory Stark (as CFM) <[email protected]>; David Geier <[email protected]>; Sergei Kornilov <[email protected]>; Alvaro Herrera <[email protected]>; Marcos Pegoraro <[email protected]>; Robert Haas <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Pavel Trukhanov <[email protected]>
Dmitry Dolgov <[email protected]> writes:
>> On Mon, Jan 22, 2024 at 05:33:26PM +1100, Peter Smith wrote:
>> Hi, This patch has a CF status of "Needs Review" [1], but it seems
>> there was a CFbot test failure last time it was run [2]. Please have a
>> look and post an updated version if necessary.
>>
>> ======
>> [1] https://commitfest.postgresql.org/46/2837/
>> [2] https://cirrus-ci.com/task/6688578378399744
> It's the same failing pipeline Vignesh C was talking above. I've fixed
> the issue in the latest patch version, but looks like it wasn't picked
> up yet (from what I understand, the latest build for this CF is 8 weeks
> old).
Please notice that at the moment, it's not being tested at all because
of a patch-apply failure -- that's what the little triangular symbol
means. The rest of the display concerns the test results from the
last successfully-applied patch version. (Perhaps that isn't a
particularly great UI design.)
If you click on the triangle you find out
== Applying patches on top of PostgreSQL commit ID b0f0a9432d0b6f53634a96715f2666f6d4ea25a1 ===
=== applying patch ./v17-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch
patching file contrib/pg_stat_statements/Makefile
Hunk #1 FAILED at 19.
1 out of 1 hunk FAILED -- saving rejects to file contrib/pg_stat_statements/Makefile.rej
patching file contrib/pg_stat_statements/expected/merging.out
patching file contrib/pg_stat_statements/meson.build
...
regards, tom lane
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2024-01-22 06:33 Re: pg_stat_statements and "IN" conditions Peter Smith <[email protected]>
2024-01-22 16:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2024-01-22 16:35 ` Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
@ 2024-01-22 17:07 ` Dmitry Dolgov <[email protected]>
2024-01-22 21:00 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2024-01-22 17:07 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Peter Smith <[email protected]>; vignesh C <[email protected]>; Michael Paquier <[email protected]>; Nathan Bossart <[email protected]>; Gregory Stark (as CFM) <[email protected]>; David Geier <[email protected]>; Sergei Kornilov <[email protected]>; Alvaro Herrera <[email protected]>; Marcos Pegoraro <[email protected]>; Robert Haas <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Pavel Trukhanov <[email protected]>
> On Mon, Jan 22, 2024 at 11:35:22AM -0500, Tom Lane wrote:
> Dmitry Dolgov <[email protected]> writes:
> >> On Mon, Jan 22, 2024 at 05:33:26PM +1100, Peter Smith wrote:
> >> Hi, This patch has a CF status of "Needs Review" [1], but it seems
> >> there was a CFbot test failure last time it was run [2]. Please have a
> >> look and post an updated version if necessary.
> >>
> >> ======
> >> [1] https://commitfest.postgresql.org/46/2837/
> >> [2] https://cirrus-ci.com/task/6688578378399744
>
> > It's the same failing pipeline Vignesh C was talking above. I've fixed
> > the issue in the latest patch version, but looks like it wasn't picked
> > up yet (from what I understand, the latest build for this CF is 8 weeks
> > old).
>
> Please notice that at the moment, it's not being tested at all because
> of a patch-apply failure -- that's what the little triangular symbol
> means. The rest of the display concerns the test results from the
> last successfully-applied patch version. (Perhaps that isn't a
> particularly great UI design.)
>
> If you click on the triangle you find out
>
> == Applying patches on top of PostgreSQL commit ID b0f0a9432d0b6f53634a96715f2666f6d4ea25a1 ===
> === applying patch ./v17-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch
> patching file contrib/pg_stat_statements/Makefile
> Hunk #1 FAILED at 19.
> 1 out of 1 hunk FAILED -- saving rejects to file contrib/pg_stat_statements/Makefile.rej
> patching file contrib/pg_stat_statements/expected/merging.out
> patching file contrib/pg_stat_statements/meson.build
Oh, I see, thanks. Give me a moment, will fix this.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2024-01-22 06:33 Re: pg_stat_statements and "IN" conditions Peter Smith <[email protected]>
2024-01-22 16:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2024-01-22 16:35 ` Re: pg_stat_statements and "IN" conditions Tom Lane <[email protected]>
2024-01-22 17:07 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2024-01-22 21:00 ` Dmitry Dolgov <[email protected]>
0 siblings, 0 replies; 38+ messages in thread
From: Dmitry Dolgov @ 2024-01-22 21:00 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Peter Smith <[email protected]>; vignesh C <[email protected]>; Michael Paquier <[email protected]>; Nathan Bossart <[email protected]>; Gregory Stark (as CFM) <[email protected]>; David Geier <[email protected]>; Sergei Kornilov <[email protected]>; Alvaro Herrera <[email protected]>; Marcos Pegoraro <[email protected]>; Robert Haas <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Pavel Trukhanov <[email protected]>
> On Mon, Jan 22, 2024 at 06:07:27PM +0100, Dmitry Dolgov wrote:
> > Please notice that at the moment, it's not being tested at all because
> > of a patch-apply failure -- that's what the little triangular symbol
> > means. The rest of the display concerns the test results from the
> > last successfully-applied patch version. (Perhaps that isn't a
> > particularly great UI design.)
> >
> > If you click on the triangle you find out
> >
> > == Applying patches on top of PostgreSQL commit ID b0f0a9432d0b6f53634a96715f2666f6d4ea25a1 ===
> > === applying patch ./v17-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch
> > patching file contrib/pg_stat_statements/Makefile
> > Hunk #1 FAILED at 19.
> > 1 out of 1 hunk FAILED -- saving rejects to file contrib/pg_stat_statements/Makefile.rej
> > patching file contrib/pg_stat_statements/expected/merging.out
> > patching file contrib/pg_stat_statements/meson.build
>
> Oh, I see, thanks. Give me a moment, will fix this.
Here is it.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
@ 2025-03-17 06:37 vignesh C <[email protected]>
2025-03-17 08:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: vignesh C @ 2025-03-17 06:37 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Sami Imseih <[email protected]>; Dmitry Dolgov <[email protected]>; Julien Rouhaud <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
On Mon, 3 Mar 2025 at 17:26, Álvaro Herrera <[email protected]> wrote:
>
> On 2025-Feb-18, Sami Imseih wrote:
>
> > > It's not a question about whether it's possible to implement this,
> > > but about whether it makes sense. In case of plain constants it's
> > > straightforward -- they will not change anything meaningfully and
> > > hence could be squashed from the query. Now for a function, that
> > > might return different values for the same set of constant
> > > arguments, it's much less obvious and omitting such expressions
> > > might have unexpected consequences.
> >
> > query jumbling should not care about the behavior of the function. If
> > we take a regular call to a volatile function, we will generate the
> > same queryId for every call regardless of the input to the function.
> > Why does the in-list case need to care about the volatility of the
> > function?
>
> I feel quite insecure about this idea TBH. At least with immutable
> functions I don't expect the system to behave wildly different than with
> actual constants. What non-immutable functions do you have in mind that
> would be useful to fold as if they were constants in the IN list in such
> a query?
>
> In the meantime, here's v28 which is Dmitry's v27 plus pgindent. No
> other changes. Dmitry, were you planning to submit a new version?
I noticed that the feedback from Sami at [1] has not yet been
addressed, I have changed the status to Waiting on Author, kindly
address them and update the status to Needs review.
[1] - https://www.postgresql.org/message-id/CAA5RZ0vt29Om%2BtKFOcUNhXV%2BkKpNnj0yj6OFho3-wngcMHWnAQ%40mail...
Regards,
Vignesh
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-03-17 06:37 Re: pg_stat_statements and "IN" conditions vignesh C <[email protected]>
@ 2025-03-17 08:11 ` Dmitry Dolgov <[email protected]>
2025-03-17 08:28 ` Re: pg_stat_statements and "IN" conditions vignesh C <[email protected]>
2025-03-17 08:44 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
0 siblings, 2 replies; 38+ messages in thread
From: Dmitry Dolgov @ 2025-03-17 08:11 UTC (permalink / raw)
To: vignesh C <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Sami Imseih <[email protected]>; Julien Rouhaud <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
> On Mon, Mar 17, 2025 at 12:07:44PM GMT, vignesh C wrote:
>
> I noticed that the feedback from Sami at [1] has not yet been
> addressed, I have changed the status to Waiting on Author, kindly
> address them and update the status to Needs review.
> [1] - https://www.postgresql.org/message-id/CAA5RZ0vt29Om%2BtKFOcUNhXV%2BkKpNnj0yj6OFho3-wngcMHWnAQ%40mail...
I'm afraid there is a disagreement about this part of the feedback. I'm
not yet convinced about the idea suggested over there (treating mutable
functions in the same way as constants) and not planning to change
anything, at least not in the current version of the patch.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-03-17 06:37 Re: pg_stat_statements and "IN" conditions vignesh C <[email protected]>
2025-03-17 08:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2025-03-17 08:28 ` vignesh C <[email protected]>
1 sibling, 0 replies; 38+ messages in thread
From: vignesh C @ 2025-03-17 08:28 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Sami Imseih <[email protected]>; Julien Rouhaud <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
On Mon, 17 Mar 2025 at 13:42, Dmitry Dolgov <[email protected]> wrote:
>
> > On Mon, Mar 17, 2025 at 12:07:44PM GMT, vignesh C wrote:
> >
> > I noticed that the feedback from Sami at [1] has not yet been
> > addressed, I have changed the status to Waiting on Author, kindly
> > address them and update the status to Needs review.
> > [1] - https://www.postgresql.org/message-id/CAA5RZ0vt29Om%2BtKFOcUNhXV%2BkKpNnj0yj6OFho3-wngcMHWnAQ%40mail...
>
> I'm afraid there is a disagreement about this part of the feedback. I'm
> not yet convinced about the idea suggested over there (treating mutable
> functions in the same way as constants) and not planning to change
> anything, at least not in the current version of the patch.
@Sami Imseih Do you have any other suggestions to solve this?
Others: Any thoughts on which way is better in this case?
Regards,
Vignesh
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-03-17 06:37 Re: pg_stat_statements and "IN" conditions vignesh C <[email protected]>
2025-03-17 08:11 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2025-03-17 08:44 ` Álvaro Herrera <[email protected]>
1 sibling, 0 replies; 38+ messages in thread
From: Álvaro Herrera @ 2025-03-17 08:44 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: vignesh C <[email protected]>; Sami Imseih <[email protected]>; Julien Rouhaud <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
On 2025-Mar-17, Dmitry Dolgov wrote:
> I'm afraid there is a disagreement about this part of the feedback. I'm
> not yet convinced about the idea suggested over there (treating mutable
> functions in the same way as constants) and not planning to change
> anything, at least not in the current version of the patch.
I have to admit that I am leaning towards removing the immutability
constraint. The reason is that we already require the function to be
boostrapped (due to the OID test) and to have implicit cast form, so
that limits which functions are recognized; the only ones there that are
not immutable are:
castsource │ casttarget │ castfunc
─────────────────────────────┼──────────────────────────┼──────────────────────────────────────────
text │ regclass │ regclass(text)
character varying │ regclass │ regclass(text)
date │ timestamp with time zone │ timestamptz(date)
time without time zone │ time with time zone │ timetz(time without time zone)
timestamp without time zone │ timestamp with time zone │ timestamptz(timestamp without time zone)
Looking at this list, it seems rather random to me to say that we should
not squash arrays with types using these casts. Should we really
consider two queries to be different because they run with different
search_path or TimeZone settings?
But kindly do not submit a new version of the patch, as I already have
some changes of my own (mostly on removing the term "merge" from code
and comments to replace with "squash", as well as adding some more
comments). I'll post it soon.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
@ 2025-03-17 19:05 Álvaro Herrera <[email protected]>
2025-03-17 19:14 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Álvaro Herrera @ 2025-03-17 19:05 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; Sami Imseih <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
On 2025-Feb-14, Dmitry Dolgov wrote:
> This should do it. The last patch for today, otherwise I'll probably add
> more bugs than features :)
Thank you. I've spent some time with this patch in the last few days,
and I propose a few changes. I renamed everything from "merge" to
"squash"; apart from that, it's mostly docs and code comments changes,
but I also removed the addition of a boolean argument to JUMBLE_LOCATION
which AFAICS is unnecessary, and did away with the business of checking
for function immutability. I also changed the code layout of
generate_normalized_query(); no functional changes, I just reordered the
code blocks (which caused a couple of lines to appear repeated that
weren't before).
You can see my patch on top of yours here:
https://github.com/alvherre/postgres/commits/query_id_squash_values/
and the CI run here:
https://cirrus-ci.com/build/5660053472018432
In addition, here I attach the complete patch on top of current master.
Unless there's some opposition to this, I intend to push this tomorrow.
I have to admit that looking at this part of the test,
+SELECT * FROM test_squash_cast WHERE data IN
+ (1::int4::casttesttype, 2::int4::casttesttype, 3::int4::casttesttype,
+ 4::int4::casttesttype, 5::int4::casttesttype, 6::int4::casttesttype,
+ 7::int4::casttesttype, 8::int4::casttesttype, 9::int4::casttesttype,
+ 10::int4::casttesttype, 11::int4::casttesttype);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT * FROM test_squash_cast WHERE data IN +| 1
+ ($1 /*, ... */::int4::casttesttype) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
and
+SELECT * FROM test_squash WHERE id IN (1::oid, 2::oid, 3::oid, 4::oid, 5::oid, 6::oid, 7::oid, 8::oid, 9::oid);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------+-------
+ SELECT * FROM test_squash WHERE id IN ($1 /*, ... */::oid) | 1
I am tempted to say that explicit casts should also be considered
squashable (that is, in IsSquashableConst() also allow the case of
func->funcformat == COERCE_EXPLICIT_CAST). That would also squash
queries such as this one:
+SELECT * FROM test_squash_bigint WHERE data IN
+ (1::bigint, 2::bigint, 3::bigint, 4::bigint, 5::bigint, 6::bigint,
+ 7::bigint, 8::bigint, 9::bigint, 10::bigint, 11::bigint);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------------------------------------+-------
+ SELECT * FROM test_squash_bigint WHERE data IN +| 1
+ ($1::bigint, $2::bigint, $3::bigint, $4::bigint, $5::bigint, $6::bigint,+|
+ $7::bigint, $8::bigint, $9::bigint, $10::bigint, $11::bigint) |
I, frankly, see little argument for making a distinction here. We can
still discuss whether we prefer it one way or the other; we don't need
that decision to prevent me from pushing the patch I here attach, I think.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"The saddest aspect of life right now is that science gathers knowledge faster
than society gathers wisdom." (Isaac Asimov)
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-03-17 19:05 Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
@ 2025-03-17 19:14 ` Álvaro Herrera <[email protected]>
2025-03-18 09:45 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Álvaro Herrera @ 2025-03-17 19:14 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; Sami Imseih <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
On 2025-Mar-17, Álvaro Herrera wrote:
> You can see my patch on top of yours here:
> https://github.com/alvherre/postgres/commits/query_id_squash_values/
> and the CI run here:
> https://cirrus-ci.com/build/5660053472018432
Heh, this blew up on bogus SGML markup :-( Fixed and running again:
https://cirrus-ci.com/build/4822893680394240
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL. This is by far the most pleasant management experience of
any database I've worked on." (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-03-17 19:05 Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-17 19:14 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
@ 2025-03-18 09:45 ` Dmitry Dolgov <[email protected]>
2025-03-18 18:33 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2025-03-18 09:45 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; Sami Imseih <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
> On Mon, Mar 17, 2025 at 08:14:16PM GMT, Álvaro Herrera wrote:
> On 2025-Mar-17, Álvaro Herrera wrote:
>
> > You can see my patch on top of yours here:
> > https://github.com/alvherre/postgres/commits/query_id_squash_values/
> > and the CI run here:
> > https://cirrus-ci.com/build/5660053472018432
>
> Heh, this blew up on bogus SGML markup :-( Fixed and running again:
> https://cirrus-ci.com/build/4822893680394240
Thanks, much appreciated! I've inspected the diff between patches and run few
tests, at the first glance everything looks fine.
> I am tempted to say that explicit casts should also be considered
> squashable (that is, in IsSquashableConst() also allow the case of
> func->funcformat == COERCE_EXPLICIT_CAST).
Well, I admit I may have been burned too much by the initial reception of the
patch and handled it too conservatively in this regard. Originally I also had a
concern about normalized queries representation for explicit cast case, but it
was resolved by Julien's suggestion to switch to the /* ... */ format.
> I have to admit that I am leaning towards removing the immutability
> constraint. The reason is that we already require the function to be
> boostrapped (due to the OID test) and to have implicit cast form, so
> that limits which functions are recognized; the only ones there that are
> not immutable are:
>
> castsource │ casttarget │ castfunc
> ─────────────────────────────┼──────────────────────────┼──────────────────────────────────────────
> text │ regclass │ regclass(text)
> character varying │ regclass │ regclass(text)
> date │ timestamp with time zone │ timestamptz(date)
> time without time zone │ time with time zone │ timetz(time without time zone)
> timestamp without time zone │ timestamp with time zone │ timestamptz(timestamp without time zone)
Agree, when put together with the OID limitation it doesn't look so bad.
Somehow I was thinking about the Sami's proposal and the discussion in more
abstract terms, as if we talk about any arbitrary mutable functions to squash
-- I still would be cautious about hiding non-bootstrapped mutable functions.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-03-17 19:05 Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-17 19:14 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-18 09:45 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2025-03-18 18:33 ` Álvaro Herrera <[email protected]>
2025-03-18 18:54 ` Re: pg_stat_statements and "IN" conditions Sami Imseih <[email protected]>
2025-03-19 07:31 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 2 replies; 38+ messages in thread
From: Álvaro Herrera @ 2025-03-18 18:33 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; Sami Imseih <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
On 2025-Mar-18, Dmitry Dolgov wrote:
> Thanks, much appreciated! I've inspected the diff between patches and
> run few tests, at the first glance everything looks fine.
Thanks for looking once more.
> > I am tempted to say that explicit casts should also be considered
> > squashable (that is, in IsSquashableConst() also allow the case of
> > func->funcformat == COERCE_EXPLICIT_CAST).
>
> Well, I admit I may have been burned too much by the initial reception
> of the patch and handled it too conservatively in this regard.
I can totally understand that. I have added this and pushed. Hopefully
nobody will hate this too much, and some people might even like it.
By the way, I'm still open to adding the 'powers' mechanism that was
discussed earlier and other simple additions on top of what's there now,
if you have some spare energy to spend on this. (For full disclosure:
the "powers" thing was discussed in a developer's meeting last year, and
several people said they'd prefer to stick with 0001 and forget about
powers, on the grounds that it produced query texts that were weird and
invalid SQL. But now that we have the commented-out syntax, it's no
longer invalid SQL, and maybe it's not so weird either.)
But there are other proposals such as handling Params and whatnot. At
this point, what we'd need for that is a more extensive test suite to
show that we're not breaking other things ...
> Agree, when put together with the OID limitation it doesn't look so bad.
> Somehow I was thinking about the Sami's proposal and the discussion in more
> abstract terms,
Yeah, that happened to me too, and then I checked again and realized
that my initial impression was wrong.
> as if we talk about any arbitrary mutable functions to squash -- I
> still would be cautious about hiding non-bootstrapped mutable
> functions.
Yeah, absolutely.
One thing I noticed while going over the code, is that we say in some
code comments that the list of elements only contributes the first and
last elements to the jumble. But this is not true -- the list actually
contributes _nothing at all_ to the jumble. I don't think this causes
any terrible problems, but maybe somebody will discover that I'm wrong
on that. This isn't trivial to solve, because if you try to add
anything to the jumble from there, you'd break the first/last location
pair matching. We could maybe fix this by returning the actual
bottommost Const node from IsSquashableConstList() instead of whatever
is wrapping it, and then arrange for _jumbleConst() to receive a boolean
that turns off jumbling of the location.
However, contributing nothing already makes such a query different from
another query that has exactly one element, because that one jumbles
that element. It could only be confused (in the sense of identical
query_ids) with another list that has zero elements.
Anyway, something to play with.
BTW, it's fun to execute a query that's literally
select col from tab where col in (1 /*, ... */);
and then
select col from tab where col in (1, 2);
because now you have two entries in pg_stat_statements with visibly the
same query text, but two different query_ids. I'm not terribly worried
about this, because who uses a literal "/*, ... */" in a query anyway?
And even if they do, it's easily explained. But jesters could probably
get a good laugh messing about with these reports.
Thanks for keeping at this for so long!
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-03-17 19:05 Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-17 19:14 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-18 09:45 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-03-18 18:33 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
@ 2025-03-18 18:54 ` Sami Imseih <[email protected]>
2025-03-18 20:17 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-19 07:32 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
1 sibling, 2 replies; 38+ messages in thread
From: Sami Imseih @ 2025-03-18 18:54 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Dmitry Dolgov <[email protected]>; Julien Rouhaud <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
I want to mention that the current patch does not deal
with external parameters ( prepared statements ) [0] [1]. This could be a
follow-up, as it may need some further discussion. It is important to
address this case, IMO.
[0]
https://www.postgresql.org/message-id/CAA5RZ0uGfxXyzhp9N5nfsS%2BZqF5ngEMC3YtBPtLoeK8EPsjHbw%40mail.g...
[1]
https://www.postgresql.org/message-id/blauoky77sash2qzrbnz6ilfyi7odtvxtdr4ifg4hq4bpqp2uk%406z5yjfsxt...
Regards,
Sami
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-03-17 19:05 Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-17 19:14 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-18 09:45 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-03-18 18:33 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-18 18:54 ` Re: pg_stat_statements and "IN" conditions Sami Imseih <[email protected]>
@ 2025-03-18 20:17 ` Álvaro Herrera <[email protected]>
1 sibling, 0 replies; 38+ messages in thread
From: Álvaro Herrera @ 2025-03-18 20:17 UTC (permalink / raw)
To: Sami Imseih <[email protected]>; +Cc: Dmitry Dolgov <[email protected]>; Julien Rouhaud <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
On 2025-Mar-18, Sami Imseih wrote:
> I want to mention that the current patch does not deal
> with external parameters ( prepared statements ) [0] [1]. This could be a
> follow-up, as it may need some further discussion. It is important to
> address this case, IMO.
Yes, I realize that. Feel free to send a patch.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"E pur si muove" (Galileo Galilei)
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-03-17 19:05 Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-17 19:14 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-18 09:45 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-03-18 18:33 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-18 18:54 ` Re: pg_stat_statements and "IN" conditions Sami Imseih <[email protected]>
@ 2025-03-19 07:32 ` Dmitry Dolgov <[email protected]>
2025-03-22 20:59 ` Re: pg_stat_statements and "IN" conditions Sami Imseih <[email protected]>
1 sibling, 1 reply; 38+ messages in thread
From: Dmitry Dolgov @ 2025-03-19 07:32 UTC (permalink / raw)
To: Sami Imseih <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Julien Rouhaud <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
> On Tue, Mar 18, 2025 at 02:54:18PM GMT, Sami Imseih wrote:
> I want to mention that the current patch does not deal
> with external parameters ( prepared statements ) [0] [1]. This could be a
> follow-up, as it may need some further discussion. It is important to
> address this case, IMO.
Sure, it's important and I'm planning to tackle this next. If you want,
we can collaborate on a patch for that.
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-03-17 19:05 Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-17 19:14 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-18 09:45 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-03-18 18:33 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-18 18:54 ` Re: pg_stat_statements and "IN" conditions Sami Imseih <[email protected]>
2025-03-19 07:32 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2025-03-22 20:59 ` Sami Imseih <[email protected]>
0 siblings, 0 replies; 38+ messages in thread
From: Sami Imseih @ 2025-03-22 20:59 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Julien Rouhaud <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
> Sure, it's important and I'm planning to tackle this next. If you want,
> we can collaborate on a patch for that.
I spent some time looking some more at this, and I believe all that needs
to be done is check for a PRAM node with a type of PARAM_EXTERN.
During planning the planner turns the Param into a Const during
eval_const_expressions_mutator.
If it's as simple as I think it is, I hope we can get this committed for 18.
If not, and a longer discussion is needed, a new thread can be started
for this.
--
Sami Imseih
Amazon Web Services (AWS)
Attachments:
[application/octet-stream] v1-0001-Allow-query-jumble-to-squash-a-list-external-paramet.patch (4.2K, ../../CAA5RZ0upwUPkTFWAYLFUVCCND9kw13U5Dc-EpAZCFTgjgWP2dw@mail.gmail.com/2-v1-0001-Allow-query-jumble-to-squash-a-list-external-paramet.patch)
download | inline diff:
From 76afc3c74f222d3bf5551e7b45cf736453ae91c9 Mon Sep 17 00:00:00 2001
From: "Sami Imseih (AWS)"
<[email protected]>
Date: Fri, 21 Mar 2025 05:37:59 +0000
Subject: [PATCH 1/1] Allow query jumble to squash a list external parameters
62d712ecf now allows query jumbling to squash a list of constants,
but not constants that are passed as external parameters. This patch
now allows the squashing of constant values supplied as external parameters
(e.g., $1, $2), as is the case with prepared statements.
---
.../pg_stat_statements/expected/squashing.out | 38 +++++++++++++++++++
contrib/pg_stat_statements/sql/squashing.sql | 12 ++++++
src/backend/nodes/queryjumblefuncs.c | 20 ++++++++--
3 files changed, 66 insertions(+), 4 deletions(-)
diff --git a/contrib/pg_stat_statements/expected/squashing.out b/contrib/pg_stat_statements/expected/squashing.out
index 55aa5109433..370d91642d4 100644
--- a/contrib/pg_stat_statements/expected/squashing.out
+++ b/contrib/pg_stat_statements/expected/squashing.out
@@ -333,6 +333,44 @@ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
(2 rows)
+-- Test bind parameters
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_squash_bigint WHERE data IN ($1, $2, $3) \bind 1 2 3
+;
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_squash_bigint WHERE data IN ($1, $2, $3, $4) \bind 1 2 3 4
+;
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_squash_bigint WHERE data IN
+ ($1::bigint, $2::bigint, $3::bigint, $4::bigint) \bind 1 2 3 4
+;
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_squash_bigint WHERE data 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_squash_bigint WHERE data IN ($1 /*, ... */) | 4
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
-- CoerceViaIO
-- Create some dummy type to force CoerceViaIO
CREATE TYPE casttesttype;
diff --git a/contrib/pg_stat_statements/sql/squashing.sql b/contrib/pg_stat_statements/sql/squashing.sql
index 56ee8ccb9a1..3ff251a59ee 100644
--- a/contrib/pg_stat_statements/sql/squashing.sql
+++ b/contrib/pg_stat_statements/sql/squashing.sql
@@ -106,6 +106,18 @@ SELECT * FROM test_squash_jsonb WHERE data IN
(SELECT '"10"')::jsonb);
SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+-- Test bind parameters
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_squash_bigint WHERE data IN ($1, $2, $3) \bind 1 2 3
+;
+SELECT * FROM test_squash_bigint WHERE data IN ($1, $2, $3, $4) \bind 1 2 3 4
+;
+SELECT * FROM test_squash_bigint WHERE data IN
+ ($1::bigint, $2::bigint, $3::bigint, $4::bigint) \bind 1 2 3 4
+;
+SELECT * FROM test_squash_bigint WHERE data IN (1, 2, 3, 4);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
-- CoerceViaIO
-- Create some dummy type to force CoerceViaIO
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index 189bfda610a..de405ab08f3 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -244,7 +244,8 @@ RecordConstLocation(JumbleState *jstate, int location, bool squashed)
* - Ignore a possible wrapping RelabelType and CoerceViaIO.
* - If it's a FuncExpr, check that the function is an implicit
* cast and its arguments are Const.
- * - Otherwise test if the expression is a simple Const.
+ * - Otherwise test if the expression is a simple Const or an
+ * external parameter.
*/
static bool
IsSquashableConst(Node *element)
@@ -278,10 +279,21 @@ IsSquashableConst(Node *element)
return true;
}
- if (!IsA(element, Const))
- return false;
+ switch (nodeTag(element))
+ {
+ case T_Const:
+ return true;
+ case T_Param:
+ {
+ Param *param = (Param *) element;
- return true;
+ return param->paramkind == PARAM_EXTERN;
+ }
+ default:
+ break;
+ }
+
+ return false;
}
/*
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-03-17 19:05 Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-17 19:14 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-18 09:45 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-03-18 18:33 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
@ 2025-03-19 07:31 ` Dmitry Dolgov <[email protected]>
1 sibling, 0 replies; 38+ messages in thread
From: Dmitry Dolgov @ 2025-03-19 07:31 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; Sami Imseih <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
> On Tue, Mar 18, 2025 at 07:33:25PM GMT, Álvaro Herrera wrote:
>
> By the way, I'm still open to adding the 'powers' mechanism that was
> discussed earlier and other simple additions on top of what's there now,
> if you have some spare energy to spend on this. (For full disclosure:
> the "powers" thing was discussed in a developer's meeting last year, and
> several people said they'd prefer to stick with 0001 and forget about
> powers, on the grounds that it produced query texts that were weird and
> invalid SQL. But now that we have the commented-out syntax, it's no
> longer invalid SQL, and maybe it's not so weird either.)
>
> But there are other proposals such as handling Params and whatnot. At
> this point, what we'd need for that is a more extensive test suite to
> show that we're not breaking other things ...
Yes, I'm planning to continue working on this topic, there are still
plenty of things that could be improved.
> One thing I noticed while going over the code, is that we say in some
> code comments that the list of elements only contributes the first and
> last elements to the jumble. But this is not true -- the list actually
> contributes _nothing at all_ to the jumble. I don't think this causes
> any terrible problems, but maybe somebody will discover that I'm wrong
> on that. This isn't trivial to solve, because if you try to add
> anything to the jumble from there, you'd break the first/last location
> pair matching. We could maybe fix this by returning the actual
> bottommost Const node from IsSquashableConstList() instead of whatever
> is wrapping it, and then arrange for _jumbleConst() to receive a boolean
> that turns off jumbling of the location.
>
> However, contributing nothing already makes such a query different from
> another query that has exactly one element, because that one jumbles
> that element. It could only be confused (in the sense of identical
> query_ids) with another list that has zero elements.
>
> Anyway, something to play with.
Yep, I don't see this as an immediate problem as well, but will do some
experiments with that.
^ permalink raw reply [nested|flat] 38+ messages in thread
end of thread, other threads:[~2025-03-22 20:59 UTC | newest]
Thread overview: 38+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-08-12 16:19 pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2020-11-18 16:04 ` Dmitry Dolgov <[email protected]>
2020-12-09 03:37 ` Chengxi Sun <[email protected]>
2020-12-09 18:49 ` Dmitry Dolgov <[email protected]>
2020-12-26 10:46 ` Dmitry Dolgov <[email protected]>
2020-12-26 16:53 ` Zhihong Yu <[email protected]>
2021-01-05 12:52 ` Dmitry Dolgov <[email protected]>
2021-01-05 15:51 ` Zhihong Yu <[email protected]>
2021-03-18 13:38 ` David Steele <[email protected]>
2021-03-18 15:50 ` Dmitry Dolgov <[email protected]>
2021-06-15 15:18 ` Dmitry Dolgov <[email protected]>
2021-06-16 14:02 ` Dmitry Dolgov <[email protected]>
2021-09-30 13:49 ` Dmitry Dolgov <[email protected]>
2021-09-30 15:03 ` Zhihong Yu <[email protected]>
2021-09-30 15:09 ` Dmitry Dolgov <[email protected]>
2022-03-26 17:40 Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2022-07-24 10:06 ` Dmitry Dolgov <[email protected]>
2022-09-16 18:25 ` Sergei Kornilov <[email protected]>
2022-09-24 14:07 ` Dmitry Dolgov <[email protected]>
2022-09-24 23:59 ` Dmitry Dolgov <[email protected]>
2024-01-22 06:33 Re: pg_stat_statements and "IN" conditions Peter Smith <[email protected]>
2024-01-22 16:11 ` Dmitry Dolgov <[email protected]>
2024-01-22 16:35 ` Tom Lane <[email protected]>
2024-01-22 17:07 ` Dmitry Dolgov <[email protected]>
2024-01-22 21:00 ` Dmitry Dolgov <[email protected]>
2025-03-17 06:37 Re: pg_stat_statements and "IN" conditions vignesh C <[email protected]>
2025-03-17 08:11 ` Dmitry Dolgov <[email protected]>
2025-03-17 08:28 ` vignesh C <[email protected]>
2025-03-17 08:44 ` Álvaro Herrera <[email protected]>
2025-03-17 19:05 Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-03-17 19:14 ` Álvaro Herrera <[email protected]>
2025-03-18 09:45 ` Dmitry Dolgov <[email protected]>
2025-03-18 18:33 ` Álvaro Herrera <[email protected]>
2025-03-18 18:54 ` Sami Imseih <[email protected]>
2025-03-18 20:17 ` Álvaro Herrera <[email protected]>
2025-03-19 07:32 ` Dmitry Dolgov <[email protected]>
2025-03-22 20:59 ` Sami Imseih <[email protected]>
2025-03-19 07:31 ` 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