On Thu, Jan 30, 2025 at 8:37 PM Michael Paquier <michael@paquier.xyz> 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?
