Re: pg_stat_statements and "IN" conditions

On Tue, Oct 17, 2023 at 10:15:41AM +0200, Dmitry Dolgov wrote:
> In the current patch version I didn't add anything yet to address the
> question of having more parameters to tune constants merging. The main
> obstacle as I see it is that the information for that has to be
> collected when jumbling various query nodes. Anything except information
> about the ArrayExpr itself would have to be acquired when jumbling some
> other parts of the query, not directly related to the ArrayExpr. It
> seems to me this interdependency between otherwise unrelated nodes
> outweigh the value it brings, and would require some more elaborate (and
> more invasive for the purpose of this patch) mechanism to implement.

typedef struct ArrayExpr
{
+ pg_node_attr(custom_query_jumble)
+

Hmm. I am not sure that this is the best approach
implementation-wise. Wouldn't it be better to invent a new
pg_node_attr (these can include parameters as well!), say
query_jumble_merge or query_jumble_agg_location that aggregates all
the parameters of a list to be considered as a single element. To put
it short, we could also apply the same property to other parts of a
parsed tree, and not only an ArrayExpr's list.

/* GUC parameters */
extern PGDLLIMPORT int compute_query_id;
-
+extern PGDLLIMPORT bool query_id_const_merge;

Not much a fan of this addition as well for an in-core GUC. I would
suggest pushing the GUC layer to pg_stat_statements, maintaining the
computation method to use as a field of JumbleState as I suspect that
this is something we should not enforce system-wide, but at
extension-level instead.

+ /*
+ * Indicates the constant represents the beginning or the end of a merged
+ * constants interval.
+ */
+ bool merged;

Not sure that this is the best thing to do either. Instead of this
extra boolean flag, could it be simpler if we switch LocationLen so as
we track the start position and the end position of a constant in a
query string, so as we'd use one LocationLen for a whole set of Const
nodes in an ArrayExpr? Perhaps this could just be a refactoring piece
of its own?

+ /*
+ * If the first expression is a constant, verify if the following elements
+ * are constants as well. If yes, the list is eligible for merging, and the
+ * order of magnitude need to be calculated.
+ */
+ if (IsA(firstExpr, Const))
+ {
+ foreach(temp, elements)
+ if (!IsA(lfirst(temp), Const))
+ return false;

This path should be benchmarked, IMO.
--
Michael