public inbox for [email protected]  

help / color / mirror / Atom feed

From: Lukas Fittl <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Sami Imseih <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Marko M <[email protected]>
Subject: Re: [PATCH] Optionally record Plan IDs to track plan changes for a query
Date: Thu, 30 Jan 2025 21:19:49 -0800
Message-ID: <CAP53Pkw1QJHH9UDjkArS=XdAxtK7XWMpZLGHnVMDmhRTp_HYYw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAP53Pkyow59ajFMHGpmb1BK9WHDypaWtUsS_5DoYUEfsa_Hktg@mail.gmail.com>
	<[email protected]>
	<CAA5RZ0sUPPOpkRZD=Za83op2ngcPC7dp249vcHA-X5YS7p3n8Q@mail.gmail.com>
	<CAP53PkwuFbo3NkwZgxwNRMjMfqPEqidD-SggaoQ4ijotBVLJAA@mail.gmail.com>
	<[email protected]>
	<[email protected]>

On Thu, Jan 30, 2025 at 8:37 PM Michael Paquier <[email protected]> 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

-- 
Lukas Fittl

view thread (34+ 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: [PATCH] Optionally record Plan IDs to track plan changes for a query
  In-Reply-To: <CAP53Pkw1QJHH9UDjkArS=XdAxtK7XWMpZLGHnVMDmhRTp_HYYw@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