>> * 0001: > You have mentioned the addition of tests, but v6-0001 includes nothing > of the kind. Am I missing something? How much coverage did you > intend to add here? These seem to be included in squashing.sql in > patch v6-0002, but IMO this should be moved somewhere else to work > with the back-branches and make the whole backpatch story more > consistent.
That's my mistake. I added a new file called normalize.sql to test
specific normalization scenarios. Added in v7
> > * 0002:
> RelabelType perhaps?
Fixed.
> A lot of the tests introduced in v6-0002 are copy-pastes of the
> previous ones for IN clauses introduced for the ARRAY cases, with
> comments explaining the reasons why lists are squashed or not also
> copy-pasted. Perhaps it would make sense to group the ARRAY and IN
> clause cases together. For example, group each of the two CoerceViaIO
> cases together in a single query on pg_stat_statements, with a single
> pg_stat_statements_reset(). That would make more difficult to miss
> the fact that we need to care about IN clauses *and* arrays when
> adding more test patterns, if we add some of course.
>
I agree. I reorganized by grouping both for IN and ARRAY tests
together for a specific test area.
I also clarified some comments in the tests, etc.
> > * 0003:
> I'd suggest to not use "n" for this one, but a different variable
> name, leaving the internals for the SubLink cases minimally touched.
I agree. Fixed.
> Implementation-wise, I would choose a location with a query length
> rather than start and end locations. That's what we do for the nested
> queries in the DMLs, so on consistency grounds..
This is different because the existing location field is tracking
something a bit different than what we want to track.
What the current location field is tracking is to assist in things
like error messages, like below, which wants to place the
caret (^) in the proper location, which is at the location of the
"IN".
```
ERROR: operator does not exist: oid = text
LINE 1: select where 1::oid IN (1::text, 2, 3);
^
HINT: No operator matches the given name and argument types. You
might need to add explicit type casts.
test=#
```
What we need for squashing is to track the start of the outer '(' and ')' of
the expression.
I could do something like fields to track list_start and list_length instead,
Will that be better to be closer in consistency?
> > * 0004: implements external parameter squashing.
> Using a custom implementation for Param nodes means that we are going
> to apply a location record for all external parameters, not only the
> ones in the lists.. Not sure if this is a good idea. Something
> smells a bit wrong with this approach. Sorry, I cannot push my finger
> on what exactly when typing this paragraph.
Actually, only the parameters outside of the squashed lists are
recorded. I added
a comment to make that clear. I would really want to only record parameter
locations if we know we have a squashed list, but it's impossible to
know that in
advance.
Also, the reason for a custom implementation for Param is to avoid having
to change the signature of JUMBLE_LOCATION because we have a
new bool argument to RecordExpressionLocation to set a location as an
external parameter. We will also need special handling in gen_node_support.pl
for Param to set the new argument. I was not too happy with doing that.
> Patches 0002 and 0003 fix bugs in the squashing logic present only on
> HEAD, nothing that impacts older branches already released, right?
That is correct.
--
Sami
v7-0003-Fix-Normalization-for-squashed-query-texts.patch
Description: Binary data
v7-0001-Fix-broken-normalization-due-to-duplicate-constan.patch
Description: Binary data
v7-0002-Enhanced-query-jumbling-squashing-tests.patch
Description: Binary data
v7-0004-Support-Squashing-of-External-Parameters.patch
Description: Binary data
