Thanks a lot for review, very useful! On Thu, Oct 8, 2026 at 1:48 PM Manu <[email protected]> wrote: > > Hi Hannu, > > I measured v2 on current master (it applies cleanly). Behaviourally > it is v1 plus the postgresql.conf.sample entry, so the observations > below hold on both versions; the attached scripts reproduce every > point. I looked at the behaviour rather than the design; some of this > may well be intended. > > The good news first: in plain mode the cost is negligible -- +2 > instructions per ReadyForQuery (+0.01%), tps within noise over two > runs, and the 'Z' message is byte-identical to master. > > Two of the stated claims did not hold in my tests. > > > libpq (fe-protocol3.c) is updated so that clients ignore any trailing > > extension bytes in ReadyForQuery messages if they receive them > > An unpatched client does not tolerate a rich 'Z'.
Sorry for being unclear - the protocol change is for the distant future. The main protection is that one should not set it to rich when client libarary can not handle this. > With master's own > libpq and with PG18's libpq, a rich session fails to connect: > message contents do not agree with length in message type "Z" > And since ready_for_query_message is PGC_USERSET, a client that > connects in plain and later runs SET ready_for_query_message = rich > hits the same error mid-session and hangs (I had to kill psql after > 8s, twice). This is indeed the current behaviour. But as there are plenty of other ways to make the connection unresponsive I am not too worried about such abuse. > > Unless you set ready_for_query_message=rich the protocol does not > > change at all > > The 'Z' itself is unchanged, but because the GUC is GUC_REPORT the > patched server sends one extra ParameterStatus > (ready_for_query_message=plain) per connection that master does not. > Harmless, but not strictly no change. Agreed, and there is really no need to be GUC_REPORT. Will remove > On the session indicators, comparing the rich values with SQL in the > same session: > - T stays 1 after the temp objects are gone -- DROP, DISCARD TEMP and > DISCARD ALL all leave T=1 with an empty pg_class. A pooler would > read the connection as dirty permanently. Ouch. Thanks! > - P reflects the whole cluster, not the session: a session that never > prepared a transaction reports P=1 while another session has one > prepared. Will check. I need to figure out how it even knows what other sessions have prepared. That should be backend-local info... > - L matches pg_current_wal_insert_lsn() numerically, but it is printed > with %X where pg_lsn uses %08X, so the text differs only while the > low word is under 8 hex digits, e.g. on a fresh cluster (0/17F5830 > vs 0/017F5830). Hmm, this is what I get on PostgreSQL 16.4 hannuk=# select '0/017F5830'::pg_lsn; pg_lsn ─────────── 0/17F5830 (1 row) Maybe there should also be 'l' which just returns the binary uint64 ? > - H was correct in every case. > > Cost of rich grows with open cursors. The H check walks the full > portal list on every 'Z', so it is O(cursors), not O(1): +5.9% > instructions per 'Z' on a clean session, +21% with 100 open cursors, > +160% with 1000. As 'Z' is on the per-query path, that may matter for > the pooling case the patch targets. Yup, at least the default should be boolean 0/1. If someone wants the number of cursors they can write an extension :) > On CI: v2 turns it green by adding the GUC to postgresql.conf.sample > (that was the 003_check_guc failure). The rich path itself is > unchanged -- fe-trace.c is still not updated, so PQtrace() prints > "mismatched message length" on each rich 'Z' and the 7 libpq_pipeline > trace tests fail under rich; they pass in plain, which is what CI > runs. make check is 239/239 in both modes. The new test module > passes but does not exercise any of the cases above, and there is > still an unrelated change to Cluster.pm. Thanks, will add that test as well. > The attached README lists the exact steps for each point above. Thanks Hannu
