Re: pg_stat_statements and "IN" conditions - Mailing list pgsql-hackers

> This should do it. The last patch for today,

I looked at v27 today and have a few comments.

1/ It looks like the CTE test is missing a check for results.

"""
-- Test constants evaluation in a CTE, which was causing issues in the past
WITH cte AS (
SELECT 'const' as const FROM test_merge
)
SELECT ARRAY['a', 'b', 'c', const::varchar] AS result
FROM cte;

-- Simple array would be merged as well
SELECT pg_stat_statements_reset() IS NOT NULL AS t;
"""

2/ Looking at IsMergeableConst, I am not sure why we care about
things like function volatility, implicit cast or funcid > FirstGenbkiObjectId?

+               if (func->funcid > FirstGenbkiObjectId)
+                       return false;
+
+               if (func->funcformat != COERCE_IMPLICIT_CAST)
+                       return false;
+
+               if (!list_member_oid(*known_immutable_funcs, func->funcid))
+               {
+                       /* Not found in the cache, verify and add if needed */
+                       if(func_volatile(func->funcid) != PROVOLATILE_IMMUTABLE)
+                               return false;
+
+                       *known_immutable_funcs =
lappend_oid(*known_immutable_funcs,
+
                          func->funcid);
+               }

Shouldn't we just be looking for Constants (or PARAM_EXTERNAL) and if so
record the location, and for all other conditions, simply call _jumbleNode? Why
wouldn't that be enough?

3/ Here, this looks wrong as we could end up traversing an elements list
twice. Once inside IsMergeableConstList and if that call returns false, we end
up traversing through the elements list again in _jumbleNode.

+       Node *first, *last;
+       if (IsMergeableConstList(elements, &first, &last))
+       {
+               /*
+                * Both first and last constants have to be recorded.
The first one
+                * will indicate the merged interval, the last one
will tell us the
+                * length of the interval within the query text.
+                *
+                * Note that for the last expression we actually need
not the expression
+                * location (which is the leftmost expression), but
where it ends. For
+                * the limited set of supported cases now (implicit coerce via
+                * FuncExpr, Const) it's fine to use exprLocation, but
if more complex
+                * composite expressions will be supported, e.g.
OpExpr or FuncExpr as
+                * an explicit call, the rightmost expression will be needed.
+                */
+               RecordConstLocation(jstate, exprLocation(first), true);
+               RecordConstLocation(jstate, exprLocation(last), true);
+       }
+       else
+       {
+               _jumbleNode(jstate, (Node *) elements);
+       }
+}


Regards,

Sami