| From: | Rithvika Devisetti <devisettirithvika(at)gmail(dot)com> |
|---|---|
| To: | Sami Imseih <samimseih(dot)pg(at)gmail(dot)com> |
| Cc: | sirisha chamarthi <sirichamarthi22(at)gmail(dot)com>, Peter Eisentraut <peter(at)eisentraut(dot)org>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Alexander Korotkov <akorotkov(at)postgresql(dot)org>, Michael Paquier <michael(at)paquier(dot)xyz> |
| Subject: | Re: WAIT FOR command should do some query jumbling |
| Date: | 2026-08-30 20:35:08 |
| Message-ID: | CA+HR5vinS4cFSgM0pABedU2iDWsHiBTYJMAUux2-6cLP-PpkLQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hello Sami and Sirisha,
Thank You very much for the patches.
I tested the v2 series on with a clean meson build (cassert enabled). Both
patches apply cleanly to master
(92819e57945) and the full test suite passes: 362 OK, 0 failures.
Your example normalizes as described:
WAIT FOR LSN 'FFFFFFFF/FFFFFFFF' WITH (MODE 'primary_flush', TIMEOUT
'1ms', NO_THROW);
WAIT FOR LSN 'FFFFFFFE/FFFFFFFF' WITH (MODE 'primary_flush', TIMEOUT
'2ms', NO_THROW);
calls | query
-------+------------------------------------------------------
2 | WAIT FOR LSN $1 WITH (MODE $2, TIMEOUT $3, NO_THROW)
A few other cases I checked, all behaving sensibly:
- TIMEOUT 1000 and TIMEOUT '1ms' normalize to the same entry despite
the different literal types.
- Different option order or a different number of options produce
separate entries, which seems right since the query text differs.
- Argument-less options (NO_THROW) are fine: arg_location stays -1 and
RecordConstLocation() already ignores negative locations.
- VACUUM/EXPLAIN/COPY still record their option values literally, as
expected since 0001 only tracks arg_location without changing
jumbling for statements that haven't opted in.
Agreed the helper looks reusable for VACUUM and ALTER ROLE later.
Regards,
Rithvika Devisetti
On Sat, Aug 29, 2026 at 6:03 PM Sami Imseih <samimseih(dot)pg(at)gmail(dot)com> wrote:
> Hi,
>
> > The query jumbling facilities should be used to handle this.
>
> +1 for this. I think we need to do a bit more than what is suggested
> in v1-0001, which only normalizes the target LSN. I think we should
> also look at the rest of the rest of the WAIT FOR syntax and normalize
> option values, for example
>
> ```
> WAIT FOR LSN 'FFFFFFFF/FFFFFFFF'
> WITH (MODE 'primary_flush', TIMEOUT '1ms', NO_THROW);
>
> WAIT FOR LSN 'FFFFFFFE/FFFFFFFF'
> WITH (MODE 'primary_flush', TIMEOUT '2ms', NO_THROW);
> ```
>
> These should normalize to one pg_stat_statements entry
>
> ```
> WAIT FOR LSN $1 WITH (MODE $2, TIMEOUT $3, NO_THROW)
> ```
>
> Because these options are carried as DefElem, I think we should also
> track DefElem arg_location, and then statement parse nodes with such
> DefElem option lists can use pg_node_attr(custom_query_jumble) to
> traverse those lists and normalize the option values.
>
> WAIT FOR is one case, but I think the same approach could also be
> useful for other utility statements such as VACUUM and ALTER ROLE. For
> example, ALTER ROLE could normalize PASSWORD and VALID UNTIL. I have
> kept this series focused on WAIT FOR for now, though.
>
> So, attached in v2, v2-0001 adds the DefElem arg_location tracking,
> and v2-0002 adds the WAIT FOR jumbling changes.
> JumbleDefElemOptions() is a small helper in queryjumblefuncs.c that
> other statements can use to implement the same kind of option
> jumbling.
>
> CC'ing Michael also to get his thoughts on the approach.
>
> --
> Sami Imseih
> Amazon Web Services (AWS)
>
| From | Date | Subject | |
|---|---|---|---|
| Next Message | shihao zhong | 2026-08-30 20:45:50 | Re: Commitfest manager for September 2026 |
| Previous Message | Daniel Gustafsson | 2026-08-30 20:15:27 | Re: Changing the state of data checksums in a running cluster |