Re: BUG: pg_stat_statements query normalization issues with combined queries

Hello Craig,

>>> [...] It's touching every single utility statement type, which is not
>>> only pretty invasive in itself, but will break any pending patches
>>> that add more utility statement types.
>>
>> Yep, but it is limited to headers and the break is trivial...
>
> I'm not sure how else that part can be done.
>
> We can add a common parent for all utility statement types, but that
> also touches all utility statements, and since we're working with pure
> C, it means 'type' won't be accessible as e.g. someCreateStmt.type, but
> only as something like ((UtilityStatement)someCreateStmt).type or
> someCreateStmt.hdr.type. [...] I don't think that's so bad, but it's not
> exactly less invasive.

I thought of this way of implementing that on the submitted version, I
decided against because then it is less obvious what is directly in the
structure, existing code may reference the tag field, and the number of
header changes is the same, if a little less verbose.

>> [...] For fixing the information in pg_stat_statement, the location
>> data must be transported from the parsed node to the query to the
>> planned node, because the later two nodes types are passed to different
>> hooks.
>
> Yep.
>
>> Now the detail is that utility statements, which seems to be nearly all of
>> them but select/update/delete/insert, do not have plans: The statement
>> itself is its own plan... so there is no place to store the location &
>> length.
>
> Yeah. I don't see any way around adding location info for utility
> statements one way or the other.

If utility statements are done this way, that's 95% of all statements, so
the point of doing the remaining 5% with Tom's neat intermediate node
trick seems void, I think that it is better to have all of them the same
way.

> TBH I think that for utility statements, adding a member struct with
> location info is the least-bad option. Something like:

> typedef struct StmtLocation
> {
> int offset;
> int length;
> }

It is possible, but:

> typedef struct CreateStmt
> {
> NodeTag type;
> StmtLocation location;
> ....
> }

Name "location" is already used meaning the offset or the directory
destination for create table space, that would create a third option.

For homogeneity, ISTM that keeping location & length directly and renaming
the table space location is the simplest & most homogeneous option.

--
Fabien.