public inbox for [email protected]  
help / color / mirror / Atom feed
From: Zsolt Parragi <[email protected]>
To: Lukas Fittl <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Peter Smith <[email protected]>
Subject: Re: Stack-based tracking of per-node WAL/buffer usage
Date: Mon, 9 Mar 2026 21:55:22 +0000
Message-ID: <CAN4CZFMon7XcTv+PKXB_ANUJ86k9VmJnx7BoW2q1m5Z2QQVZQw@mail.gmail.com> (raw)
In-Reply-To: <CAP53PkxrmpECzVFpeeEEHDGe6u625s+YkmVv5-gw3L_NDSfbiA@mail.gmail.com>
References: <CAP53PkzdBK8VJ1fS4AZ481LgMN8f9mJiC39ZRHqkFUSYq6KWmg@mail.gmail.com>
	<h7zq426fhzt3oqkps53qxfwl3dlz3ifcwqmybuvxntlcaqwb33@m6huopxovgle>
	<CAP53Pkw1RpyAVsvJwUOMgGnVoQUibMk+EB+w-8WqY8kzsC4WjA@mail.gmail.com>
	<CAP53Pkz-BmkLFJ+sAcr8H1sBPtWp8WYGzorH9yw+W5Tn0yH0sg@mail.gmail.com>
	<mflexfbmbkmql5snr322ldu72dfnjr2dntxx3ippaz4tcso7vl@zmxxgiws3qux>
	<CAP53PkzZ3UotnRrrnXWAv=F4avRq9MQ8zU+bxoN9tpovEu6fGQ@mail.gmail.com>
	<k2uxwancbi5blb2duny3kbuz667m47hqc4jv7xstlx3yyzcd3a@fjqww6avalt3>
	<CAHut+Put94CTpjQsqOJHdHkgJ2ZXq+qVSfMEcmDKLiWLW-hPfA@mail.gmail.com>
	<CAP53PkyOvXC7pWAiamvWth_JNeb=isrxX+PJT0pw_Hw5Czzf+Q@mail.gmail.com>
	<CAP53PkzGbyeJMLDAcvMRgzXPXYsYXZr3SBg0UwhfkYjqu8oK_g@mail.gmail.com>
	<CAP53PkxrmpECzVFpeeEEHDGe6u625s+YkmVv5-gw3L_NDSfbiA@mail.gmail.com>

Hello

+ if (queryDesc->totaltime && estate->es_instrument && !IsParallelWorker())
+ {
+ ExecFinalizeNodeInstrumentation(queryDesc->planstate);
+
+ ExecFinalizeTriggerInstrumentation(estate);
+ }
+
  if (queryDesc->totaltime)
- InstrStop(queryDesc->totaltime);
+ queryDesc->totaltime = InstrQueryStopFinalize(queryDesc->totaltime);

In ExecFinalizeNodeInstrumentation InstrFinalizeNode pfrees the
original instrumentation, but doesn't remove it from the
unfinalized_children list. In normal execution in
InstrQueryStopFinalize ResourceOwnerForgetInstrumentation handles
this, but what about the error path, if something happens between the
two?
Won't we end up in ResOwnerReleaseInstrumentation and do use after
free reads and then a double free on the now invalid pointer?



+ if (myState->es->timing || myState->es->buffers)
+ instr = InstrQueryStopFinalize(instr);
+

Is it okay to leak 1 instrumentation copy per tuple in the query
context? This freshly palloced object will be out of scope a few lines
after this.


@@ -128,8 +130,22 @@ IndexNext(IndexScanState *node)

IndexNextWithReorder doesn't need the same handling?


+ if (scandesc->xs_heap_continue)
+ elog(ERROR, "non-MVCC snapshots are not supported in index-only scans");

Shouldn't this say index scans? (there's another preexisting
indexonylscan mention in this file, but that also seems wrong)

+#if HAVE_INSTR_STACK
+ usage = &instr_top.bufusage;
+#else
+ usage = &pgBufferUsage;
+#endif

I don't see pgBufferUsage anywhere else in the current code, it was
probably removed between rebases?


+ char    *prefix = title ? psprintf("%s ", title) : pstrdup("");
+
+ ExplainPropertyInteger(psprintf("%sShared Hit Blocks", prefix), NULL,
     usage->shared_blks_hit, es);

(And many similar ExplainPropery after this)

title is NULL most of the time, and this results in 16 allocations for
that common case - isn't there a better solution like using
ExplainOpenGroup or something?

+ * Callers must ensure that no intermediate stack entries are skipped, to
+ * handle aborts correctly. If you're thinking of calling this in a PG_FINALLY
+ * block, instead call InstrPopAndFinalizeStack which can skip intermediate
+ * stack entries, or instead use InstrStart/InstrStop.

InstrPopAndFinalizeStack doesn't exists in the latest patch version





view thread (44+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Stack-based tracking of per-node WAL/buffer usage
  In-Reply-To: <CAN4CZFMon7XcTv+PKXB_ANUJ86k9VmJnx7BoW2q1m5Z2QQVZQw@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox