MIME-Version: 1.0
References: <CAP53Pkyow59ajFMHGpmb1BK9WHDypaWtUsS_5DoYUEfsa_Hktg@mail.gmail.com>
 <Z5A__Yuy23aXqUad@paquier.xyz> <CAA5RZ0sUPPOpkRZD=Za83op2ngcPC7dp249vcHA-X5YS7p3n8Q@mail.gmail.com>
 <CAP53PkwuFbo3NkwZgxwNRMjMfqPEqidD-SggaoQ4ijotBVLJAA@mail.gmail.com>
 <Z5cDQJM9nnoXuStO@paquier.xyz> <Z5xTb5iBHVGns35R@paquier.xyz>
In-Reply-To: <Z5xTb5iBHVGns35R@paquier.xyz>
From: Lukas Fittl <lukas@fittl.com>
Date: Thu, 30 Jan 2025 21:19:49 -0800
Message-ID: <CAP53Pkw1QJHH9UDjkArS=XdAxtK7XWMpZLGHnVMDmhRTp_HYYw@mail.gmail.com>
Subject: Re: [PATCH] Optionally record Plan IDs to track plan changes for a query
To: Michael Paquier <michael@paquier.xyz>
Cc: Sami Imseih <samimseih@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Marko M <marko@pganalyze.com>
Content-Type: multipart/alternative; boundary="00000000000039f1e0062cf9b3a8"
Archived-At: <https://www.postgresql.org/message-id/CAP53Pkw1QJHH9UDjkArS%3DXdAxtK7XWMpZLGHnVMDmhRTp_HYYw%40mail.gmail.com>
Precedence: bulk

--00000000000039f1e0062cf9b3a8
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 30, 2025 at 8:37=E2=80=AFPM Michael Paquier <michael@paquier.xy=
z> wrote:

> After thinking more about this one, I still want this toy and hearing
> nothing I have applied it, with a second commit for the addition in
> injection_points to avoid multiple bullet points in a single commit.
>

Thanks for committing! I had intended to review/test your patch, but the
earlier parts of the week got way too busy.

I think the API with do_drop makes sense, and whilst I'd think there is
some extra overhead to calling the function vs having an inline check for
kind, it seems unlikely this would be used in a performance critical
context, and the flexibility seems useful.

I have noticed post-commit that I have made a mistake in the credits
> of a632cd354d35 and ce5c620fb625 for your family name.  Really sorry
> about that!  This mistake is on me..
>

No worries regarding the name, happens to me all the time :)

> What do you think?
>
> Attached is a rebased version of the three remaining patches.  While
> looking at this stuff, I have noticed an extra cleanup that would be
> good to have, as a separate change: we could reformat a bit the plan
> header comments so as these do not require a rewrite when adding
> node_attr to them, like d575051b9af9.
>

Yeah, I think that'd be helpful to move the comments before the fields - it
definitely gets hard to read.

Sami's patch set posted at [1] has the same problem, making the
> proposals harder to parse and review, and the devil is in the details
> with these pg_node_attr() properties attached to the structures.  That
> would be something to do on top of the proposed patch sets.  Would any
> of you be interested in that?
>

I'd be happy to tackle that - were you thinking to simply move any comments
before the field, in each case where we're adding an annotation?

Separately I've been thinking how we could best have a discussion/review on
whether the jumbling of specific plan struct fields is correct. I was
thinking maybe a quick wiki page could be helpful, noting why to jumble/not
jumble certain fields?

Thanks,
Lukas

--=20
Lukas Fittl

--00000000000039f1e0062cf9b3a8
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">On Thu, Jan 30, 2025 at 8:37=E2=80=AFPM M=
ichael Paquier &lt;<a href=3D"mailto:michael@paquier.xyz">michael@paquier.x=
yz</a>&gt; wrote:</div><div class=3D"gmail_quote gmail_quote_container"><bl=
ockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-lef=
t:1px solid rgb(204,204,204);padding-left:1ex">After thinking more about th=
is one, I still want this toy and hearing<br>
nothing I have applied it, with a second commit for the addition in<br>
injection_points to avoid multiple bullet points in a single commit.<br></b=
lockquote><div><br></div><div><div>Thanks for committing! I had intended to=
 review/test your patch, but the earlier parts of the week got way too busy=
.</div><div><br></div><div>I
 think the API with do_drop makes sense, and whilst I&#39;d think there is
 some extra overhead to calling the function vs having an inline check for =
kind, it=20
seems unlikely this would be used in a performance critical context, and th=
e flexibility seems useful.</div><div><br></div></div><blockquote class=3D"=
gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(20=
4,204,204);padding-left:1ex">
I have noticed post-commit that I have made a mistake in the credits<br>
of a632cd354d35 and ce5c620fb625 for your family name.=C2=A0 Really sorry<b=
r>
about that!=C2=A0 This mistake is on me..<br></blockquote><br><div>No worri=
es regarding the name, happens to me all the time :)</div><div><br></div><b=
lockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-le=
ft:1px solid rgb(204,204,204);padding-left:1ex">
&gt; What do you think?<br>
<br>
Attached is a rebased version of the three remaining patches.=C2=A0 While<b=
r>
looking at this stuff, I have noticed an extra cleanup that would be<br>
good to have, as a separate change: we could reformat a bit the plan<br>
header comments so as these do not require a rewrite when adding<br>
node_attr to them, like d575051b9af9.<br></blockquote><div><br></div><div>Y=
eah, I think that&#39;d be helpful to move the comments before the fields -=
 it definitely gets hard to read. <br></div><div><br></div><blockquote clas=
s=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid r=
gb(204,204,204);padding-left:1ex">
Sami&#39;s patch set posted at [1] has the same problem, making the<br>
proposals harder to parse and review, and the devil is in the details<br>
with these pg_node_attr() properties attached to the structures.=C2=A0 That=
<br>
would be something to do on top of the proposed patch sets.=C2=A0 Would any=
<br>
of you be interested in that?<br></blockquote><div><br></div><div>I&#39;d b=
e happy to tackle that - were you thinking to simply move any comments befo=
re the field, in each case where we&#39;re adding an annotation?</div><div>=
<br></div></div><div>Separately I&#39;ve been thinking how we could best ha=
ve a discussion/review on whether the jumbling of specific plan struct fiel=
ds is correct. I was thinking maybe a quick wiki page could be helpful, not=
ing why to jumble/not jumble certain fields?</div><div><br></div><div>Thank=
s,</div><div>Lukas</div><div><br clear=3D"all"></div><span class=3D"gmail_s=
ignature_prefix">-- </span><br><div dir=3D"ltr" class=3D"gmail_signature"><=
div dir=3D"ltr"><div><div dir=3D"ltr"><div><div dir=3D"ltr">Lukas Fittl</di=
v></div></div></div></div></div></div>

--00000000000039f1e0062cf9b3a8--