WAIT FOR command should do some query jumbling

Hi Peter,

On Fri, Aug 28, 2026 at 2:48 PM Peter Eisentraut <peter@eisentraut.org>
wrote:

Right now, WAIT FOR commands are recorded literally in
pg_stat_statements. I think the actual LSN value should be replaced by
some placeholder, like

WAIT FOR LSN $1

Otherwise, a client library or framework that runs this command
repeatedly would needlessly spam pg_stat_statements.

The query jumbling facilities should be used to handle this.

+1 to your suggestion. Please find the attached patch to address this issue.

Regards,
Sirisha

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)

Hello,

Thanks for the patch. I tested it on with a
clean meson build (cassert enabled). It applies cleanly to master
(92819e57945) and does what it says:

WAIT FOR LSN '0/0' WITH (MODE 'primary_flush');
WAIT FOR LSN '0/1' WITH (MODE 'primary_flush');
WAIT FOR LSN '0/2' WITH (MODE 'primary_flush');

SELECT calls, query FROM pg_stat_statements WHERE query LIKE 'WAIT FOR%';
calls | query
-------+---------------------------------------------
3 | WAIT FOR LSN $1 WITH (MODE 'primary_flush')

Same result with real LSNs from pg_current_wal_lsn(), and the queryid is
stable across resets. Jumbling of other statements is unaffected.

Full test suite passes: 362 OK, 0 failures (the 40 skips are all
PG_TEST_EXTRA opt-ins). No new compiler warnings from the patched files.

One question: the LSN is normalized, but option values still aren't.
Varying TIMEOUT produces separate entries:

calls | query
-------+-----------------------------------------------------------
1 | WAIT FOR LSN $1 WITH (MODE 'primary_flush', TIMEOUT 1000)
1 | WAIT FOR LSN $1 WITH (MODE 'primary_flush', TIMEOUT 2000)
1 | WAIT FOR LSN $1 WITH (MODE 'primary_flush', TIMEOUT 3000)

A client computing a dynamic timeout (remaining budget, backoff) would
still spam pg_stat_statements, which seems close to the original
complaint. I realize DefElem isn't jumble-annotated and this is
consistent with other utility statements, so it may be out of scope
here, but wanted to raise it in case TIMEOUT is expected to vary in
practice while MODE isn't.

Regards,
Rithvika Devisetti

On Fri, Aug 28, 2026 at 4:07 PM sirisha chamarthi <sirichamarthi22@gmail.com>
wrote:

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.pg@gmail.com> wrote:

Hi Sami

On Sat, Aug 29, 2026 at 6:03 PM Sami Imseih <samimseih.pg@gmail.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.

Thanks Sami for the v2 patch! This patch applied cleanly and the tests
passed.
One observation with this approach is at times option can have different
meanings
and that may be lost during jumbling. For example, in the WAL FOR LSN case,
4 different MODEs are defined with different semantics and this patch does
not differentiate.

WAIT FOR LSN $1 WITH (MODE $2, TIMEOUT $3, NO_THROW)

Regards,
Sirisha

Hi,

One observation with this approach is at times option can have different

meanings
and that may be lost during jumbling. For example, in the WAL FOR LSN case,
4 different MODEs are defined with different semantics and this patch does
not differentiate.

WAIT FOR LSN $1 WITH (MODE $2, TIMEOUT $3, NO_THROW)

I thought about this, but did not think it will be useful to keep MODE
without normalization, since we are normalizing the LSN. But, perhaps
others have a different opinion?

--
Sami Imseih

On Sun, Aug 30, 2026 at 05:27:59PM -0500, Sami Imseih wrote:

I thought about this, but did not think it will be useful to keep MODE
without normalization, since we are normalizing the LSN. But, perhaps
others have a different opinion?

I can see the lack of normalization for the LSN as a bug, but the
option patterns in a WITH clause is IMO more debatable. I think that
we should collect more data from the field before setting down on a
behavior we may regret. I can partially get behind a normalization of
the timeout, but I am less clear about the MODE and NO_THROW, as it
could be useful to split them.

In terms of v19, it seems to me that just ignoring the LSN part is
enough as a LSN is a moving target, so I would agree about using v1
for now, backpatch it down to v19. We should consider some of the
patterns covered by v2 once we know more about how users use this
feature.
--
Michael

I can see the lack of normalization for the LSN as a bug, but the
option patterns in a WITH clause is IMO more debatable. I think that
we should collect more data from the field before setting down on a
behavior we may regret. I can partially get behind a normalization of
the timeout, but I am less clear about the MODE and NO_THROW, as it
could be useful to split them.

IMO, LSN and timeout are both the minimum we should normalize here.

I am also not sure how tools will use this feature, but what makes timeout
safe IMO is the fact that the "timeout" values across many executions that
collapse into a single entry can be understood by the *_exec_time fields.

--
Sami

On Sun, Aug 30, 2026 at 09:56:29PM -0500, Sami Imseih wrote:

IMO, LSN and timeout are both the minimum we should normalize here.

I am also not sure how tools will use this feature, but what makes timeout
safe IMO is the fact that the "timeout" values across many executions that
collapse into a single entry can be understood by the *_exec_time fields.

I see the traction here, but I cannot really get fully convinced by a
grouping of the timeouts without more input. As a whole, I think that
Sirisha's patch is fine enough on its own.

Adding Nathan with his RMT hat on for comments regarding a potential
backpatch to v19, so as we have a fixed query ID with v20.
--
Michael

I see the traction here, but I cannot really get fully convinced by a
grouping of the timeouts without more input. As a whole, I think that
Sirisha's patch is fine enough on its own.

Perhaps LSN is enough for now as it will be the value that will vary the
most with heavy usage. I don't know how often TIMEOUT values will
vary however, so I'll drop the options normalization idea for now.

--
Sami

I'm still digging into the details here, but what are folks' thoughts on
how critical this patch is for v19? IOW is it a big deal if we leave it
out of v19 and put it in v20? Or should the pg_stat_statements spam be a
blocker for its release?

--
nathan

I'm still digging into the details here, but what are folks' thoughts on
how critical this patch is for v19? IOW is it a big deal if we leave it
out of v19 and put it in v20? Or should the pg_stat_statements spam be a
blocker for its release?

I am not sure if I would call it a blocker. The pg_stat_statements spam will
be confined to a standby, unless the MODE used is primary_flush, which
ISTM will be the lesser used case. But for both cases, the workaround is
to disable utility statement tracking. So, there is a workaround.

This is very much similar to how lack of SAVEPOINT normalization with
many unique savepoint names would cause pg_stat_statements before we
fixed that.

But if we go with v1-, this seems like a simple enough fix to get it, if
there
is still a chance to do so.

--
Sami

On Mon, Aug 31, 2026 at 10:08:54AM -0500, Sami Imseih wrote:

But if we go with v1-, this seems like a simple enough fix to get it, if
there is still a chance to do so.

Yeah, after staring at this some more, v1 seems reasonable to proceed with
for v19 from an RMT perspective. I've alerted Melanie and Heikki in case
they want to share opinions, too.

--
nathan

On Mon, Aug 31, 2026 at 11:28:13AM -0500, Nathan Bossart wrote:

Yeah, after staring at this some more, v1 seems reasonable to proceed with
for v19 from an RMT perspective. I've alerted Melanie and Heikki in case
they want to share opinions, too.

Hearing nothing, I have expanded the tests in v1 with more option
patterns, and applied it down to v19.
--
Michael

On Wed, Sep 2, 2026 at 8:16 AM Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Aug 31, 2026 at 11:28:13AM -0500, Nathan Bossart wrote:

Yeah, after staring at this some more, v1 seems reasonable to proceed with
for v19 from an RMT perspective. I've alerted Melanie and Heikki in case
they want to share opinions, too.

Hearing nothing, I have expanded the tests in v1 with more option
patterns, and applied it down to v19.

I was about to deal with this issue. Thank you for pushing v1,
I'll take a look at v2 for v20.

------
Regards,
Alexander Korotkov
Supabase

On Wed, Sep 02, 2026 at 03:53:20PM +0300, Alexander Korotkov wrote:

I was about to deal with this issue. Thank you for pushing v1,

No problem.

I'll take a look at v2 for v20.

Sami has also mentioned to me off-list that we may want something
smarter in terms of the management of the DefElems that include
strings, because we have other commands with contents that we may want
to normalize in some contexts, like an ALTER TABLE with a password as
one example. Perhaps it would be better to not rush that part.
--
Michael

Hi!

On Thu, Sep 3, 2026 at 6:38 AM Michael Paquier <michael@paquier.xyz> wrote:

Sami has also mentioned to me off-list that we may want something
smarter in terms of the management of the DefElems that include
strings, because we have other commands with contents that we may want
to normalize in some contexts, like an ALTER TABLE with a password as
one example. Perhaps it would be better to not rush that part.

Agreed, so v3 attached drops the generic part and does WAIT only.

Two things today: waits differing only in the timeout get an entry
each, and so does the same wait with its options written in another
order.

1 | WAIT FOR LSN $1 WITH (mode 'primary_flush', timeout '5ms')
1 | WAIT FOR LSN $1 WITH (mode 'primary_flush', timeout '7ms')
1 | WAIT FOR LSN $1 WITH (timeout '5ms', mode 'primary_flush')

With v3 those are one entry:

3 | WAIT FOR LSN $1 WITH (mode 'primary_flush', timeout $2)

while the modes stay apart, as Sirisha asked for:

1 | WAIT FOR LSN $1 WITH (mode 'standby_replay', timeout $2, no_throw)
1 | WAIT FOR LSN $1 WITH (mode 'standby_write', timeout $2, no_throw)

WaitStmt.options gets a custom jumble function that walks the
recognized options in a fixed order instead of the order written,
records TIMEOUT as a constant, and jumbles MODE and NO_THROW as given.
No generic DefElem option normalization: what a value means differs
per option and only the command knows, which is the same problem
VariableSetStmt.jumble_args solves per grammar flavor.

DefElem gains arg_location, since DefElem.location points at the
option name -- recording that one normalizes the wrong token:

WAIT FOR LSN $1 WITH (mode 'primary_flush', $2 '5ms')

and the argument node is a bare String or Integer, which carries no
location at all. Nothing else reads the new field, so no other
statement changes.

One wart left alone: "WITH (no_throw)" and "WITH (no_throw true)" are
still separate entries, as VACUUM (ANALYZE) and VACUUM (ANALYZE true)
are today. Resolving it would mean calling defGetBoolean() while
jumbling, which can throw.

Passwords in pg_stat_statements are untouched by this and can go to
their own thread.

------
Regards,
Alexander Korotkov
Supabase

Hi Alexander,

Two things today: waits differing only in the timeout get an entry
each,

I agree this is not ideal, and it's the point I raised earlier.

and so does the same wait with its options written in another
order.

I am not sure we should add the additional complexity in the custom
jumble to deal with this. I don't anticipate this will be a problem in
the real world. Scripts and drivers will use the same order. Also we
don't really guarantee that in other commands:

```
VACUUM (ANALYZE, VERBOSE) and VACUUM (VERBOSE, ANALYZE)
```

give different queryIds, and the same is true of COPY and every other
command taking an option list.

If we want option order to not affect the queryId, I think we should
teach jumbling to do this for all commands that take options; the
consumers of utility_option_list and copy_generic_opt_list, and
probably some other places.

So my preference is to deal with the TIMEOUT jumbling only, and
keep the ordering aspect out of this for now.

Maybe others have a different opinion.

--
Sami Imseih
Amazon Web Services (AWS)