Re: Improve pg_stat_statements by making jumble handle savepoint names better

On Tue, Jul 25, 2023 at 12:37:18PM -0400, Greg Sabino Mullane wrote:
> Yes, it should. I had some trouble getting it to work that way in the first
> place, but now I realize it was just my unfamiliarity with this part of the
> code. So thanks for the hint: v2 of the patch is much simplified by adding
> two attributes to theTransactionStmt node. I've also added some tests per
> your suggestion.

- char *savepoint_name; /* for savepoint commands */
+ char *savepoint_name pg_node_attr(query_jumble_ignore);
char *gid; /* for two-phase-commit related commands */
bool chain; /* AND CHAIN option */
+ int location pg_node_attr(query_jumble_location);
} TransactionStmt;

You got that right. Applying query_jumble_ignore makes sure that we
don't finish with different query IDs for different savepoint names.

> Unrelated to this patch, I'm struggling with meson testing. Why doesn't
> this update the postgres test binary?:
>
> meson test --suite pg_stat_statements

Yes, it should do so, I assume. I've seen that myself. That's
contrary to what make check does, but perhaps things are better
integrated this way with meson.

I think that I'm OK with your proposal as savepoint names are in
defined places in these queries (contrary to some of the craziness
with BEGIN and the node structure of TransactionStmt, for example).

Has somebody an opinion or a comment to share?
--
Michael