MIME-Version: 1.0
References: <57biou6l65r7gr4nunoe6lignz2x6m3w45gihoypaez4pc46di@txj3bakhj66l>
 <CAP53PkxDupm5U6TV0LF_YWHQ2wfcSiLsgnDBcO6b5AchLJhp=A@mail.gmail.com>
 <mtjyijvuv7xavvcdy3rosg43ycy3t5sluioxkef46se25ajtxp@e3e2xvesqhzu>
 <CAP53Pky4zHCueAyPkWE-cH6SqE_m+Kppmy=zE5K36X3HvCFZLw@mail.gmail.com>
 <pgvy7lsoa5jldwgyay2g757xivzbmgyan547wealc7cwtduvra@dysyw6dtqdgs>
 <3xbje45m5knff52mye5dfnrjdnwv7it2bzmqac3jqe66fvop4a@xvhy6zx7n6sb>
 <CAP53Pkzec5L=PDvF+zrPei2kM1FZH6pD2aD=zFWXwzW8oKXJBg@mail.gmail.com>
 <CAN4CZFM11mo-Bo3nhdcvVQ_ue7w3u_p8AL6xyDs054Q8kSv-xQ@mail.gmail.com>
 <CAP53PkzfeTzoC0WTL-ftVsn00ZV9wTe2_6g71txE5dN3Zq1y5g@mail.gmail.com>
 <CAP53Pkyqsht+exJQYRsjhSWYKu+vFGHhPub7m6PmFD6Or0=p1g@mail.gmail.com>
 <rhah5kx5ftfztu532zck4mhc4hxtfis4z552laisd53sf6ycrn@wnbste4hleyg>
In-Reply-To: <rhah5kx5ftfztu532zck4mhc4hxtfis4z552laisd53sf6ycrn@wnbste4hleyg>
From: Lukas Fittl <lukas@fittl.com>
Date: Tue, 7 Apr 2026 15:27:45 -0700
Message-ID: 
 <CAP53PkzpJ39HFRGaMR7n5kSsNuqui3TJ3Gw_FU7k_qjdBvT6hg@mail.gmail.com>
Subject: Re: Stack-based tracking of per-node WAL/buffer usage
To: Andres Freund <andres@anarazel.de>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Tomas Vondra <tomas@vondra.me>,
	Peter Smith <smithpb2250@gmail.com>,
 Zsolt Parragi <zsolt.parragi@percona.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAP53PkzpJ39HFRGaMR7n5kSsNuqui3TJ3Gw_FU7k_qjdBvT6hg%40mail.gmail.com>
Precedence: bulk

On Tue, Apr 7, 2026 at 3:19=E2=80=AFPM Andres Freund <andres@anarazel.de> w=
rote:
>
> Hi,
>
> On 2026-04-07 13:30:11 -0700, Lukas Fittl wrote:
> > 0001 is the change to make queryDesc->totaltime be allocated by
> > ExecutorStart instead of plugins themselves, and adds a
> > queryDesc->totaltime_options to have plugins request which level of
> > summary instrumentation they need. This change is pretty simple, and
> > could still make sense to get into 19. Because of the earlier
> > Instrumentation refactoring that was pushed (thanks!) we're already
> > asking extensions allocating queryDesc->totaltime to modify their use
> > of InstrAlloc, so I think we might as well clean this up now.
>
> Hm.  That's a fair argument. They indeed would have to again change next
> release
>
>  It's not a complicated change and removes more lines than it adds.
>
> I guess one thing I'm not sure is whether the fields shouldn't be renamed=
 at
> the same time:
>
> a) To prevent extensions from continuing to set it, most of them do not t=
est
>    against assert enabled builds. With a different name they would get a
>    compiler error.
>
> b) "totaltime" and "totaltime_options" are pretty poor descriptors of tra=
cking
>    query level statistics.  If everyone has to change anyway, this is a g=
ood
>    occasion.
>
> 'query_instr[_options]'?
>
>
> Any opinions?

I think renaming makes sense - both to make sure extensions reconsider
how they use it, and because "totaltime" is a bad name anyway, because
its not just about timing (and hasn't been for many releases).

"query_instr[_options]" seems reasonable to me, although we could drop
the "query_" since it'd be "queryDesc->query_instr" vs
"queryDesc->instr".

>
> > 0002 is just ExecProcNodeInstr moved to instrument.c, as Andres had
> > suggested previously. We still get some quick performance wins from
> > doing that (see end of email), and again, its a simple change, so
> > could be considered if someone has bandwidth remaining. I've added a
> > later patch that then does the more complex inlining and gets us the
> > full speed up.
>
> Here it needs a few more inlines to get the full performance, otherwise i=
t
> doesn't inline all the helpers.  I think on balance I didn't like the
> prototype in instrument.h, that's too widely included, and it might even =
cause
> some circularity issues.  It seems better to do the somewhat ugly thing a=
nd
> have the prototype be in executor.h.

Yeah, that makes sense.

>
> > 0002 measurements (with current master and TSC clock source used for
> > timing, best of three):
> >
> > CREATE TABLE lotsarows(key int not null);
> > INSERT INTO lotsarows SELECT generate_series(1, 50000000);
> > VACUUM FREEZE lotsarows;
>
> With the somewhat more extreme benchmark I used in the rdtsc thread and t=
he
> added inline mentioned above I see a bit bigger wins. See the attached
> explainbench.sql - it doesn't quite cover all the combinations, but I thi=
nk it
> gives a good enough overview.
>
> c=3D1 pgbench -f ~/tmp/explainbench.sql -P5 -r -t 10
>
> master:
> statement latencies in milliseconds and failures:
>        200.800           0 SELECT pg_prewarm('pgbench_accounts');
>          0.098           0 PREPARE query AS SELECT * FROM pgbench_account=
s OFFSET 5000000 LIMIT 1;
>        212.010           0 EXPLAIN (ANALYZE, BUFFERS OFF, WAL OFF, TIMING=
 OFF)
>        268.648           0 EXPLAIN (ANALYZE, BUFFERS OFF, WAL OFF, TIMING=
 ON)
>        232.421           0 EXPLAIN (ANALYZE, BUFFERS ON, WAL ON, TIMING O=
FF)
>        283.531           0 EXPLAIN (ANALYZE, BUFFERS ON, WAL ON, TIMING O=
N)
>          0.030           0 DEALLOCATE query;
>
>
> 0002:
>
> statement latencies in milliseconds and failures:
>        201.558           0 SELECT pg_prewarm('pgbench_accounts');
>          0.103           0 PREPARE query AS SELECT * FROM pgbench_account=
s OFFSET 5000000 LIMIT 1;
>        188.696           0 EXPLAIN (ANALYZE, BUFFERS OFF, WAL OFF, TIMING=
 OFF)
>        244.479           0 EXPLAIN (ANALYZE, BUFFERS OFF, WAL OFF, TIMING=
 ON)
>        223.773           0 EXPLAIN (ANALYZE, BUFFERS ON, WAL ON, TIMING O=
FF)
>        266.947           0 EXPLAIN (ANALYZE, BUFFERS ON, WAL ON, TIMING O=
N)
>          0.034           0 DEALLOCATE query;
>
> That's something like 4-12%.
>
> Pretty nice for a patch that just adds a few lines around and adds a few
> inlines.

Agreed.


> > @@ -334,6 +334,9 @@ explain_ExecutorStart(QueryDesc *queryDesc, int efl=
ags)
> >
> >       if (auto_explain_enabled())
> >       {
> > +             /* We're always interested in runtime */
> > +             queryDesc->totaltime_options |=3D INSTRUMENT_TIMER;
>
> > -                     queryDesc->totaltime =3D InstrAlloc(INSTRUMENT_AL=
L);
>
> Not that it's going to make a significant difference, but it is nice that=
 this
> now would need to track less.

Yup.

>
> Kinda wonder about having
>   EXPLAIN (ANALYZE BUFFERS totals_only, WAL totals_only) ...;
>
> in plenty cases that'd be all one needs, at substantially lower cost.

True. I don't like the name "totals_only", but I like the concept.
Today someone has to go to pg_stat_statements to get just the total
numbers, without running them for all nodes with EXPLAIN ANALYZE (and
incurring its overhead).

Thanks,
Lukas

--=20
Lukas Fittl