On Fri, Apr 3, 2026 at 10:09 PM Lukas Fittl <[email protected]> wrote: > v2/0002: > > > +bool > > +GUCCheckExplainExtensionOption(const char *option_name, > > + const char *option_value, > > + NodeTag option_type) > > +{ > > It feels a bit odd to not use an actual node here as the input > argument (replacing option_value and option_type), or even pass a > DefElem. > > But, if I followed your reasoning correctly, you're avoiding using > DefElems here, because you don't want to keep nodes in auto_explain's > GUC derived struct, to ensure we use guc_malloc for derived > information. And presumably you're also modeling this particular > method after GUCs in general, which don't have values represented as > nodes.
Right, exactly. Although we sometimes cheat, a GUC's "extra" information is really supposed to consist of a single allocated chunk. I think that's a super-awkward constraint that we should really try to find a way to lift (and there was a thread about that earlier this release cycle which failed to reach a successful conclusion). But in this case, it seemed relatively straightforward to abide by that constraint, so I did. I'm actually pretty happy with the result: as we also discussed regarding pg_stash_advice and persistence, there's much to be said for fully validating the input first and only doing real work (such as allocating) afterward. I think I would like it better if the interface didn't need to refer to node types at all, but that seemed hard to avoid. What I'm more worried about with this patch is that the new infrastructure is too special-purpose. I think what I've learned here is that designing an interface around a single ExplainOptionHandler callback was a bad idea, because it had too specific an idea about how the callback was to be used and didn't leave future callers enough room to make their own decisions. This patch tries to paper around that mistake by adding ExplainOptionGUCCheckHandler, but I think there is a good chance that this will turn out to have exactly the same problem of being too specific to a particular use case. In other words, instead of fixing my earlier mistake, I'm making it a second time, which is usually not what you want to do. However, I still don't really know what I should be doing instead. If at some point in the future we figure out a way to separate EXPLAIN option validation and EXPLAIN option application in a cleaner way, I think we can refactor this for a future major release and just accept that extensions that provide EXPLAIN options will need updating. In the meantime, I think committing this is better than admitting defeat, so I have done that. > With that in mind, 0002 looks good as well. Thanks for looking! -- Robert Haas EDB: http://www.enterprisedb.com
