On Thu, Feb 06, 2025 at 07:52:53PM -0600, Sami Imseih wrote: > This fixes the long comments in plannodes.h to make it easier to add the > attribute annotation. It made the most sense to make this the first patch > in the set.
A commit that happened last Friday made also this to have conflict. > Done. Also rewrote the header comment in jumblefuncs.c to describe > a more generic node jumbling mechanism that this file now offers. > > Yes, after getting my hands on this, I agree with you. It made more sense > to keep all the jumbling work in jumblefuncs.c -static void AppendJumble(JumbleState *jstate, - const unsigned char *item, Size size I don't understand why there is a need for publishing AppendJumble() while it remains statis in jumblefuncs.c. This is not needed in 0003 and 0004, either. Should we use more generic names for the existing custom_query_jumble, no_query_jumble, query_jumble_ignore and query_jumble_location? Last time I've talked about that with Peter E, "jumble" felt too generic, so perhaps we're looking for a completely new term? This impacts as well the naming of the existing queryjumblefuncs.c. The simplest term that may be possible here is "hash", actually, because that's what we are doing with all these node structures? That's also a very generic term. The concept of location does not apply to plans, based on the current proposal, so perhaps we should talk about "query normalization location"? Point is that query_jumble_ignore is used in the planner nodes, which feels inconsistent, so perhaps we could rename query_jumble_ignore and no_query_jumble to "hash_ignore" and/or "no_hash", or something like that? This may point towards the need of a split, not sure, still the picture is incomplete. > v5-0003 and v5-0004 introduce the planId in core and pg_stat_plans. These > needed rebasing only; but I have not yet looked at this thoroughly. > > We should aim to get 0001 and 0002 committed next. Yeah. I didn't see any reasons why 0001 should not happen now, as it makes the whole easier while making the header styles a bit more consistent. Perhaps also if somebody forks the code and adds some pg_node_attr() properties? v5-0003 and v5-0004, not sure yet. The intrisincs of the planner make putting a strict definition of what a hash means hard to set down, we should work towards studying that more first. I don't see this happen until the next release freeze. -- Michael
signature.asc
Description: PGP signature
