MIME-Version: 1.0
References: 
 <CAP53PkznofNg+ii363QQGoje30nhssuSz_hV5U4YANAt-Yr_Yg@mail.gmail.com>
 <a1edb578-8a54-4f7a-ad74-11ce9cef291a@iki.fi>
 <CAP53PkwCk7X_ryOak_3x6ek2f+4kCgGjWe8aVy39D59Q8y9wTg@mail.gmail.com>
 <CAP53Pky2=31B1AS9vg=Ca9308_hb0H4g968cSKxiFgT0moJfYg@mail.gmail.com>
 <CAP53PkybGzJTAo7V07ssUae5047pBY3jc_F07tGY13cNhqe+AQ@mail.gmail.com>
 <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>
In-Reply-To: 
 <CAN4CZFM11mo-Bo3nhdcvVQ_ue7w3u_p8AL6xyDs054Q8kSv-xQ@mail.gmail.com>
From: Lukas Fittl <lukas@fittl.com>
Date: Mon, 6 Apr 2026 17:39:15 -0700
Message-ID: 
 <CAP53PkzfeTzoC0WTL-ftVsn00ZV9wTe2_6g71txE5dN3Zq1y5g@mail.gmail.com>
Subject: Re: Stack-based tracking of per-node WAL/buffer usage
To: Zsolt Parragi <zsolt.parragi@percona.com>
Cc: Andres Freund <andres@anarazel.de>, Heikki Linnakangas <hlinnaka@iki.fi>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Tomas Vondra <tomas@vondra.me>,
	Peter Smith <smithpb2250@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAP53PkzfeTzoC0WTL-ftVsn00ZV9wTe2_6g71txE5dN3Zq1y5g%40mail.gmail.com>
Precedence: bulk

On Mon, Apr 6, 2026 at 3:46=E2=80=AFPM Zsolt Parragi <zsolt.parragi@percona=
.com> wrote:
>
> I couldn't find any issues with v15, all comments are stylistic/minor,
> except maybe the first one.

Thanks for reviewing!

>
> + /* Abort handling: link in parent QueryInstrumentation's unfinalized li=
st */
> + dlist_node unfinalized_entry;
>
> Is it okay to store a pointer in shared memory, even if it seems to be
> always NULL there?

Its not ideal, mainly because a caller might interpret it incorrectly,
but as long as we don't read from it, its safe in practice. In the
parallel instrumentation we just use the Instrumentation struct as a
way to transport data (with the 0006 patch applied), and we
copy/accumulate from it before it gets used elsewhere.

I've previously avoided putting the unfinalized_entry value in the
Instrumentation struct for that reason, but I don't think there is a
good way to avoid that without complicating the design.

>
>  #ifndef INSTRUMENT_NODE_H
>  #define INSTRUMENT_NODE_H
>
> +
> +#include "executor/tuptable.h"
> +#include "nodes/execnodes.h"
> +
>
> Is it okay to incude files in the middle of the file, is there a good
> reason why these can't be at the top of the file?

Yeah, those need to be on the top of the file, good catch.

>
> + * Recurse into children first (bottom-up accumulation), and accummulate
> + * to this nodes instrumentation as the parent context.
>
> Two typos (accumulate / this node's)

Good catch, agreed those are typos.

>
>  #define RELEASE_PRIO_FILES 600
>  #define RELEASE_PRIO_WAITEVENTSETS 700
> +#define RELEASE_PRIO_INSTRUMENTATION 800
>
> This is mainly a generic observation, not strictly related to this
> patch, but this list could use some explanation which of these
> priorities are actually required by dependencies, and which are just
> "put the new entry at the end of the list".

Agreed, that would be helpful. It'll require more investigation to
confirm particular ordering reasons that exist today, but it seems
worth explaining more clearly.

I'll hold off on posting another patch round since what you raised
were just small stylistic issues, and they don't apply to the
remaining prep patches before the stack patch itself.

Thanks,
Lukas

--=20
Lukas Fittl