Hi Hannu,
I re-tested v3 on your base (061065e28f) and on today's master HEAD; it
applies cleanly to both. All eight points from the v2 review are
addressed and reproduce here -- plain mode is byte-identical to master
(startup 459 B, 'Z' 66 B), the extra ParameterStatus is gone, P is now
session-local, 'l' decodes to the COMMIT record's end_lsn, compat is
back (unpatched libpq completes in plain, rejects rich with the
documented length error), and check-world is clean under cassert (the
new module's 42 subtests, libpq_pipeline 24/24).
> 5. O(1) With-Hold Cursors ('H')
Confirmed, and it is a real win: the per-'Z' cost in rich went from
+41,703 instructions at 1000 open cursors in v2 to a flat +861 in v3.
Measuring the indicators against SQL in the same session, four edge
cases still report a state that does not match the catalog. Full
sequences are in the attached file; in short:
- T = 0 with a live temp table, two ways: (a) BEGIN; SET LOCAL
ready_for_query_message = plain; CREATE TEMP TABLE; COMMIT -- the
pre-commit check runs while the setting is plain, and the assign hook
rechecks only when IsTransactionState(); (b) switching to rich via
ALTER SYSTEM + pg_reload_conf() -- the SIGHUP is handled between
statements, so the hook skips the check. A plain SET does recheck.
- H = 1 with no cursors, after a procedure that COMMITs inside a FOR
loop: HoldPinnedPortals() -> HoldPortal() increments the counter, but
that portal has no CURSOR_OPT_HOLD, so PortalDrop() never decrements.
- H = 0 with a WITH HOLD cursor still open, if another WITH HOLD cursor
errors while materializing at COMMIT: the failed portal already has a
holdStore, so PortalDrop() decrements for it although HoldPortal()
never counted it.
Three of these (the two T cases and the last H case) are false
negatives -- a pooler would read the session as clean while it still
has state -- so they seem worth closing before the indicators are
relied on.
Two more, not contradicting any claim:
- Rich costs nothing extra when a transaction touches no temp objects,
as you say (-S prepared: within noise). When every transaction does
touch a temp table, rich is ~13-15% slower (two runs, both CIs below
zero), from the pg_depend scan at each pre-commit. Might be worth a
note that the 'T' bookkeeping has a cost under temp-heavy workloads.
- 'l' advances outside a commit. A read-only SELECT after a checkpoint
moved 'l' forward; pg_waldump shows only FPI_FOR_HINT and
PRUNE_ON_ACCESS records (xid 0) in that range. RecordTransactionCommit()
sets XactLastCommitEnd from XactLastRecEnd even for an xid-less
transaction that wrote WAL, so 'l' is never behind the last commit
(fine for read-your-writes) but is not exactly "the last commit LSN"
the docs describe. This is master's behaviour, not v3's.
Attached: the exact reproductions, and the wire client updated to
decode 'l'.
Regards,
Manu
