Thanks for the updated patch set! On Fri, 2022-10-28 at 17:59 -0500, Justin Pryzby wrote: > > > 0004, 0005, 0006, 0007: EXPLAIN (MACHINE) > > > > I think it is confusing that these are included in this patch set. > > EXPLAIN (MACHINE OFF) is similar to "explain_regress = on", only it goes > > even further: > > no query ID, no Heap Fetches, no Sort details, ... > > Why not add this functionality to the GUC? > > Good question, and one I've asked myself. > > explain_regress affects pre-existing uses of explain in the regression > tests, aiming to simplify current (or maybe only future) uses. And > is obviously used to allow enabling BUFFERS by default. > > explain(MACHINE off) is mostly about allowing "explain ANALYZE" to be > used in regression tests. Which, right now, is rare. Maybe I shouldn't > include those patches until the earlier patches progress (or, maybe we > should just defer their discussion).
Essentially, "explain_regress" is to simplify the current regression tests, and MACHINE OFF is to simplify future regression tests. That does not seem like a meaningful distinction to me. Both are only useful for the regression tests, and I see no need for two different knobs. My opinion is now like this: +1 on enabling BUFFERS by default for EXPLAIN (ANALYZE) +0.2 on "explain_regress" -1 on EXPLAIN (MACHINE) in addition to "explain_regress" -0.1 on adding the MACHINE OFF functionality to "explain_regress" "explain_regress" reduces the EXPLAIN options you need for regression tests. This is somewhat useful, but not a big win. Also, it will make backpatching regression tests slightly harder for the next 5 years. Hence the +0.2 for "explain_regress". For me, an important question is: do we really want more EXPLAIN (ANALYZE) in the regression tests? It will probably slow the tests down, and I wonder if there is a lot of added value, if we have to remove all the machine-specific output anyway. Hence the -0.1. > > 0005 suppresses "rows removed by filter", but how is that machine > > dependent? > > It can vary with the number of parallel workers (see 13e8b2ee8), which > may not be "machine dependent" but the point of that patch is to allow > predictable output of EXPLAIN ANALYZE. Maybe it needs a better name (or > maybe it should be folded into the first patch). Now it makes sense to me. I just didn't think of that. > I'm not actually sure if this patch should try to address parallel > workers at all, or (if so) if what it's doing now is adequate. > > > > BTW, I think it may be that the GUC should be marked PGDLLIMPORT ? > > > > I think it is project policy to apply this mark wherever it is needed. Do > > you think > > that third-party extensions might need to use this in C code? > > Since v15 (8ec569479), the policy is: > > On Windows, export all the server's global variables using PGDLLIMPORT > > markers (Robert Haas) > > I'm convinced now that's what's intended here. You convinced me too. > > This is not enough. The patch would have to update all the examples > > that use EXPLAIN ANALYZE. > > Fair enough. I've left a comment to handle this later if the patch > gains any traction and 001 is progressed. I agree with that. I would really like to see 0003 go in, but it currently depends on 0001, which I am not so sure about. I understand that you did that so that "explain_regress" can turn off BUFFERS and there is no extra churn in the regression tests. Still, it would be a shame if resistance against "explain_regress" would be a show-stopper for 0003. If I could get my way, I'd want two separate patches: first, one to turn BUFFERS on, and second one for "explain_regress" with its current functionality on top of that. Yours, Laurenz Albe