> The test additions done in v7-0002 look sensible here. > > --- In the following two queries the operator expressions (+) and (@) have > --- different oppno, and will be given different query_id if squashed, even > though > --- the normalized query will be the same > > In v7-0002, this comment is removed, but it still applies, isn't it?
No, the comment is wrong/misleading even in HEAD. The output looks
like the below so, the normalized query will not be the same and the
queries will also not be squashed.
```
SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
query
| calls
----------------------------------------------------------------------------------------------------+-------
SELECT * FROM test_squash WHERE id IN
+| 2
($1 + $2, $3 + $4, $5 + $6, $7 + $8, $9 + $10, $11 + $12, $13
+ $14, $15 + $16, $17 + $18) |
SELECT * FROM test_squash WHERE id IN
+| 2
(@ $1, @ $2, @ $3, @ $4, @ $5, @ $6, @ $7, @ $8, @ $9)
|
SELECT pg_stat_statements_reset() IS NOT NULL AS t
| 1
(3 rows)
```
so I felt a much simpler and more appropriate comment is
```
-- No constants squashing for OpExpr
```
> --- Bigint, explicit cast is not squashed
> +-- Bigint, explicit cast is squashed
>
> Seems incorrect with 0002 taken in isolation. The last cast is still
> present in the normalization. It's not after v7-0003.
in 0002, this is still a squashed string even if the "::bigint" still appears at
the end of the string. Squashing is replacing the elements with
"$1 /*, ... */"
```
----------------------------------------------------+-------
SELECT * FROM test_squash_bigint WHERE data IN +| 2
($1 /*, ... */::bigint)
```
0003 improves/fixes this by truly squashing between the entire
boundary of the list.
> Already mentioned upthread, but applying only v7-0003 on top of
> v7-0002 (not v7-0004) leads to various regression failures in dml.sql
> and squashing.sql. The failures persist with v7-0004 applied. Please
> see these as per the attached, the IN lists do not get squashed, the
> array elements are. Just to make sure that I am not missing
> something, I've rebuilt from scratch with no success.
I cannot reproduce this. I applied each patch (v7-0002, 0003)
in order and ran "make check" on pg_stat_statements for every apply,
and I could not reproduce. Not sure what you and I are doing different?
I also could not reproduce with the v8 patch series either.
> IsSquashableExpressionList() includes this comment, which is outdated,
> probably because squashing was originally optional behind a GUC and
> the parameter has been removed while the comment has not been
> refreshed:
> /*
> * If squashing is disabled, or the list is too short, we don't try to
> * squash it.
> */
Thanks for reminding me about this. I remember seeing it, but missed
fixing it. Corrected now.
> RecordExpressionLocation()'s top comment needs a refresh, talking
> about constants. The simplifications gained in pgss.c's normalization
> are pretty cool.
Yes, missed this also. Done.
> + bool has_squashed_lists;
> [...]
> + if (jstate->has_squashed_lists)
> + jstate->highest_extern_param_id = 0;
>
> This new flag in JumbleState needs to be documented, explaining why
> it needs to be here.
Done. I felt that combining highest_extern_param_id and
has_squashed_lists in the
same comment made the most sense, as they are closely related.
- /* highest Param id we've seen, in order to start normalization
correctly */
+ /*
+ * Highest Param id we've seen, in order to start normalization correctly.
+ * However, if the jumble contains at least one squashed list, we
+ * disregard the highest_extern_param_id value because parameters can
+ * exist within the squashed list and are no longer considered for
+ * normalization.
+ */
int highest_extern_param_id;
+ bool has_squashed_lists;
> I have to admit that it is strange to see
> highest_extern_param_id, one value in JumbleState be forced to zero in
> the PGSS normalization code if has_squashed_lists is set to true.
> This seems like a layer violation to me
Yeah, that's silly of me. This should be done in DoJumble after
_jumbleNode. Fixed.
I also reorganized the tests in extended.out to make them more readable,
namely I wanted to show separate outputs for what is tested
for "-- Unique query IDs with parameter numbers switched." and what is tested
for "-- Two groups of two queries with the same query ID."
I also added a comment for
``
bool extern_param;
```
v8 addresses the above.
--
Sami Imseih
Amazon Web Services (AWS)
v8-0003-Support-Squashing-of-External-Parameters.patch
Description: Binary data
v8-0002-Fix-Normalization-for-squashed-query-texts.patch
Description: Binary data
v8-0001-Enhanced-query-jumbling-squashing-tests.patch
Description: Binary data
