Обсуждение: BUFFERS enabled by default in EXPLAIN (ANALYZE)
Re-reading the thread [1] (cannot answer there – don't have those emails in my box anymore), I see that there was strong support for enabling BUFFERS in EXPLAIN ANALYZE by default. And there were patches. Commitfest entry [2] was marked Rejected because there were questions to the implementation based on GUCs.
Attached is a simple patch enabling BUFFERS by default, without involving GUCs.
Why it is important?
In many cases, people forget about the BUFFERS option in EXPLAIN ANALYZE and share execution plans without it – sending it via regular communication channels for work or publishing to visualization systems. Meanwhile, the option has a lower overhead compared to TIMING (enabled by default for EXPLAIN ANALYZE) and it is extremely useful for query
optimization. This patch doesn't enable BUFFERS for EXPLAIN executed
without ANALYZE.
optimization. This patch doesn't enable BUFFERS for EXPLAIN executed
without ANALYZE.
Open questions:
1. Should BUFFERS be enabled for regular (w/o ANALYZE) EXPLAIN? Now it may make sense because of the buffer numbers the planner uses. This patch doesn't do that, but it definitely may make sense because it can help people understand why planning time is so big, in some cases.
2. How to adjust documentation? Should EXPLAIN ANALYZE examples be adjusted to use BUFFERS OFF (easier change) or show some example buffer numbers – like it is done for timing and cost numbers? I tend to think that the latter makes more sense.
3. How to adjust regression tests? Of course, now many tests fail. Same question as for documentation. Excluding buffer, numbers would be an easier fix, of course – but at least some tests should
check the behavior of BUFFERS (such as both default options – with and without ANALYZE).
On any given platform, the buffer numbers are pretty stable, so we could rely on it, but I'm not sure about all possible options being tested and would appreciate advice here (of course, if the patch makes it thru the discussion in general).
Вложения
On 11/12/21 23:58, Nikolay Samokhvalov wrote: > Re-reading the thread [1] (cannot answer there – don't have those emails > in my box anymore), You can download the message as mbox and import it into your client (pretty much any client supports that, I think). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 11/12/21 11:58 PM, Nikolay Samokhvalov wrote: > Re-reading the thread [1] (cannot answer there – don't have those emails in > my box anymore), I see that there was strong support for enabling BUFFERS > in EXPLAIN ANALYZE by default. And there were patches. Commitfest entry [2] > was marked Rejected because there were questions to the implementation > based on GUCs. > > Attached is a simple patch enabling BUFFERS by default, without involving > GUCs. I obviously agree with this (although I still wish we had gucs as we keep adding more and more options to EXPLAIN). The patch looks good to me, too. +1 -- Vik Fearing
On Sat, Nov 13, 2021 at 8:18 AM Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: > > On 11/12/21 23:58, Nikolay Samokhvalov wrote: > > Re-reading the thread [1] (cannot answer there – don't have those emails > > in my box anymore), > > You can download the message as mbox and import it into your client > (pretty much any client supports that, I think). There is also a "resend mail" link available for each mail in the archive which should be even more straightforward.
On Fri, Nov 12, 2021 at 02:58:07PM -0800, Nikolay Samokhvalov wrote: > Re-reading the thread [1] (cannot answer there – don't have those emails in > my box anymore), I see that there was strong support for enabling BUFFERS > in EXPLAIN ANALYZE by default. And there were patches. Commitfest entry [2] > was marked Rejected because there were questions to the implementation > based on GUCs. I think the only reason this isn't already the default is because of (3): > 3. How to adjust regression tests? Of course, now many tests fail. Same > question as for documentation. Excluding buffer, numbers would be an easier > fix, of course – but at least some tests should > check the behavior of BUFFERS (such as both default options – with and > without ANALYZE). Some time ago, I had a few relevant patches: 1) add explain(REGRESS) which is shorthand for (BUFFERS OFF, TIMING OFF, COSTS OFF, SUMMARY OFF) 2) add explain(MACHINE) which elides machine-specific output from explain; for example, Heap Fetches, sort spaceUsed, hash nbuckets, and tidbitmap stuff. https://www.postgresql.org/message-id/flat/20200306213310.GM684@telsasoft.com > 1. Should BUFFERS be enabled for regular (w/o ANALYZE) EXPLAIN? Now it may > make sense because of the buffer numbers the planner uses. This patch > doesn't do that, but it definitely may make sense because it can help > people understand why planning time is so big, in some cases. I think it *should* be enabled for planning, since that makes the default easier to understand and document, and it makes a user's use of "explain" easier. However, that makes this patch series more complicated: explain(ANALYZE) is rare in the regression tests, but this would affect the output of bare "explain", which affects almost every test. I think the answer to that is to add a GUC (as Tom suggested in an old thread) like explain_regress=on, which causes explain to omit these details. This would be instead of my explain(REGRESS), and would change the defaults of various params in a central place, to avoid the need to update many regression tests. In the future, this might also handle the stuff that my "explain(MACHINE)" attempted to handle. I've rebased my patches like this. I think the explain(REGRESS) should be re-written as a GUC which is set by regress.c. I'm interested to hear from a reviewer if this is worth pursing like this. -- Justin
Вложения
I think it *should* be enabled for planning, since that makes the default
easier to understand and document, and it makes a user's use of "explain"
easier.
I’d be keen to see BUFFERS off by default with EXPLAIN, and on by default with EXPLAIN ANALYZE.
The SUMMARY flag was implemented that way, which I think has been easy enough for folks to understand and document. In fact, I think the only BUFFERS information goes in the “summary” section for EXPLAIN (BUFFERS), so maybe it makes perfect sense? If it would be great if that made implementation easier, too.
In any case, thank you all, I’m so glad that this is being discussed again. It’d be so good to start seeing buffers in more plans.
—
Michael
On Mon, Nov 15, 2021 at 01:09:54PM -0600, Justin Pryzby wrote: > Some time ago, I had a few relevant patches: > 1) add explain(REGRESS) which is shorthand for (BUFFERS OFF, TIMING OFF, COSTS OFF, SUMMARY OFF) > 2) add explain(MACHINE) which elides machine-specific output from explain; > for example, Heap Fetches, sort spaceUsed, hash nbuckets, and tidbitmap stuff. > > https://www.postgresql.org/message-id/flat/20200306213310.GM684@telsasoft.com The attached patch series now looks like this (some minor patches are not included in this list): 1. add GUC explain_regress, which disables TIMING, SUMMARY, COSTS; 2. enable explain(BUFFERS) by default (but disabled by explain_regress); 3. Add explain(MACHINE) - which is disabled by explain_regress. This elides various machine-specific output like Memory and Disk use. Maybe it should be called something else like "QUIET" or "VERBOSE_MINUS_1" or ?? The regression tests now automatically run with explain_regress=on, which is shorthand for TIMING OFF, SUMMARY OFF, COSTS OFF, and then BUFFERS OFF. There's a further option called explain(MACHINE) which can be disabled to hide portions of the output that are unstable, like Memory/Disk/Batches/ Heap Fetches/Heap Blocks. This allows "explain analyze" to be used more easily in regression tests, and simplifies some existing tests that currently use plpgsql functions to filter the output. But it doesn't handle all the variations from parallel workers. (3) is optional, but simplifies some regression tests. The patch series could be rephrased with (3) first. Unfortunately, "COSTS OFF" breaks postgres_fdw remote_estimate. If specifying "COSTS ON" in postgres_fdw.c is considered to be a poor fix , then I suppose this patch series could do as suggested and enable buffers by default only when ANALYZE is specified. Then postgres_fdw is not affected, and the explain_regress GUC is optional: instead, we'd need to specify BUFFERS OFF in ~100 regression tests which use EXPLAIN ANALYZE. (3) still seems useful on its own.
Вложения
I'm renaming this thread for better visibility, since buffers is a small, optional part of the patches I sent. I made a CF entry here. https://commitfest.postgresql.org/36/3409/ On Wed, Dec 01, 2021 at 06:58:20PM -0600, Justin Pryzby wrote: > On Mon, Nov 15, 2021 at 01:09:54PM -0600, Justin Pryzby wrote: > > Some time ago, I had a few relevant patches: > > 1) add explain(REGRESS) which is shorthand for (BUFFERS OFF, TIMING OFF, COSTS OFF, SUMMARY OFF) > > 2) add explain(MACHINE) which elides machine-specific output from explain; > > for example, Heap Fetches, sort spaceUsed, hash nbuckets, and tidbitmap stuff. > > > > https://www.postgresql.org/message-id/flat/20200306213310.GM684@telsasoft.com > > The attached patch series now looks like this (some minor patches are not > included in this list): > > 1. add GUC explain_regress, which disables TIMING, SUMMARY, COSTS; > 2. enable explain(BUFFERS) by default (but disabled by explain_regress); > 3. Add explain(MACHINE) - which is disabled by explain_regress. > This elides various machine-specific output like Memory and Disk use. > Maybe it should be called something else like "QUIET" or "VERBOSE_MINUS_1" > or ?? > > The regression tests now automatically run with explain_regress=on, which is > shorthand for TIMING OFF, SUMMARY OFF, COSTS OFF, and then BUFFERS OFF. > > There's a further option called explain(MACHINE) which can be disabled to hide > portions of the output that are unstable, like Memory/Disk/Batches/ > Heap Fetches/Heap Blocks. This allows "explain analyze" to be used more easily > in regression tests, and simplifies some existing tests that currently use > plpgsql functions to filter the output. But it doesn't handle all the > variations from parallel workers. > > (3) is optional, but simplifies some regression tests. The patch series could > be rephrased with (3) first. > > Unfortunately, "COSTS OFF" breaks postgres_fdw remote_estimate. If specifying > "COSTS ON" in postgres_fdw.c is considered to be a poor fix , then I suppose > this patch series could do as suggested and enable buffers by default only when > ANALYZE is specified. Then postgres_fdw is not affected, and the > explain_regress GUC is optional: instead, we'd need to specify BUFFERS OFF in > ~100 regression tests which use EXPLAIN ANALYZE. (3) still seems useful on its > own.
Вложения
Rebased over ebf6c5249b7db525e59563fb149642665c88f747. It looks like that patch handles only query_id, and this patch also tries to handle a bunch of other stuff. If it's helpful, feel free to kick this patch to a future CF.
Вложения
On Sat, Feb 26, 2022 at 03:07:20PM -0600, Justin Pryzby wrote: > Rebased over ebf6c5249b7db525e59563fb149642665c88f747. > It looks like that patch handles only query_id, and this patch also tries to > handle a bunch of other stuff. > > If it's helpful, feel free to kick this patch to a future CF. Rebased over MERGE.
Вложения
@cfbot: rebased
Вложения
On Mon, Jan 24, 2022 at 9:54 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
I'm renaming this thread for better visibility, since buffers is a small,
optional part of the patches I sent.
I made a CF entry here.
https://commitfest.postgresql.org/36/3409/
On Wed, Dec 01, 2021 at 06:58:20PM -0600, Justin Pryzby wrote:
> On Mon, Nov 15, 2021 at 01:09:54PM -0600, Justin Pryzby wrote:
> > Some time ago, I had a few relevant patches:
> > 1) add explain(REGRESS) which is shorthand for (BUFFERS OFF, TIMING OFF, COSTS OFF, SUMMARY OFF)
> > 2) add explain(MACHINE) which elides machine-specific output from explain;
> > for example, Heap Fetches, sort spaceUsed, hash nbuckets, and tidbitmap stuff.
> >
> > https://www.postgresql.org/message-id/flat/20200306213310.GM684@telsasoft.com
>
> The attached patch series now looks like this (some minor patches are not
> included in this list):
>
> 1. add GUC explain_regress, which disables TIMING, SUMMARY, COSTS;
> 2. enable explain(BUFFERS) by default (but disabled by explain_regress);
+1
> 3. Add explain(MACHINE) - which is disabled by explain_regress.
> This elides various machine-specific output like Memory and Disk use.
> Maybe it should be called something else like "QUIET" or "VERBOSE_MINUS_1"
> or ??
INCIDENTALS ?
Aside from being 4 syllables and, for me at least, a pain to remember how to spell, it seems to fit.
RUNTIME is probably easier, and we just need to point out the difficulty in naming things since actual rows is still shown, and timings have their own independent option.
>
> The regression tests now automatically run with explain_regress=on, which is
> shorthand for TIMING OFF, SUMMARY OFF, COSTS OFF, and then BUFFERS OFF.
>
> There's a further option called explain(MACHINE) which can be disabled to hide
> portions of the output that are unstable, like Memory/Disk/Batches/
> Heap Fetches/Heap Blocks. This allows "explain analyze" to be used more easily
> in regression tests, and simplifies some existing tests that currently use
> plpgsql functions to filter the output. But it doesn't handle all the
> variations from parallel workers.
>
> (3) is optional, but simplifies some regression tests. The patch series could
> be rephrased with (3) first.
I like the idea of encapsulating the logic about what to show into the generation code instead of a post-processing routine.
I don't see any particular ordering of the three changes being most clear.
>
> Unfortunately, "COSTS OFF" breaks postgres_fdw remote_estimate. If specifying
> "COSTS ON" in postgres_fdw.c is considered to be a poor fix
Given that we have version tests scattered throughout the postgres_fdw.c code I'd say protect it with version >= 9.0 and call it good.
But I'm assuming that we are somehow also sending over the GUC to the remote server (which will be v16+) but I am unsure why that would be the case. I'm done code reading for now so I'm leaving this an open request/question.
, then I suppose
> this patch series could do as suggested and enable buffers by default only when
> ANALYZE is specified. Then postgres_fdw is not affected, and the
> explain_regress GUC is optional: instead, we'd need to specify BUFFERS OFF in
> ~100 regression tests which use EXPLAIN ANALYZE.
I'm not following the transition from the prior sentences about COSTS to this one regarding BUFFERS.
(3) still seems useful on its
> own.
It is an orthogonal feature with a different section of our user base being catered to, so I'd agree by default.
Patch Review Comments:
The following change in the machine commit seems out-of-place; are we fixing a bug here?
explain.c:
/* Show buffer usage in planning */
- if (bufusage)+ if (bufusage && es->buffers)
Typo (trailing comment // without comment):
ExplainPropertyFloat("Actual Rows", NULL, rows, 0, es); //
I did a pretty thorough look-through of the locations of the various changes and everything seems consistent with the established code in this area (took me a bit to get to the level of comprehension though). Whether there may be something missing I'm less able to judge.
From reading the other main thread I'm not finding this particular choice of GUC usage to be a problem anyone has raised and it does seem the cleanest solution, both for us and for anyone out there with a similar testing framework.
The machine output option covers an obvious need since we've already written ad-hoc parsing code to deal with the problem.
And, as someone who sees first-hand the kinds of conversations that occur surrounding beginner's questions, and getting more into performance questions specifically, I'm sympathetic with the desire to have BUFFERS something that is output by default. Given existing buy-in for that idea I'd hope that at minimum the "update 100 .sql and expected output files, then change the default" commit happens even if the rest take some further discussion. Though with even just a bit of attention I don't see any real problems getting the whole thing resolved one way or another.
David J.
On Tue, Jul 26, 2022 at 03:38:53PM -0700, David G. Johnston wrote: > On Mon, Jan 24, 2022 at 9:54 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > Unfortunately, "COSTS OFF" breaks postgres_fdw remote_estimate. If specifying > > > "COSTS ON" in postgres_fdw.c is considered to be a poor fix , then I suppose > > > this patch series could do as suggested and enable buffers by default only when > > > ANALYZE is specified. Then postgres_fdw is not affected, and the > > > explain_regress GUC is optional: instead, we'd need to specify BUFFERS OFF in > > > ~100 regression tests which use EXPLAIN ANALYZE. > > I'm not following the transition from the prior sentences about COSTS to > this one regarding BUFFERS. In this patch, SET explain_regress=on changes to defaults to explain(COSTS OFF), so postgres_fdw now explicitly uses COSTS ON. The patch also changes default to (BUFFERS OFF), which is what allows enabling buffers by default. I think I was just saying that the enabling buffers by default depends on somehow disabling it in regression tests (preferably not by adding 100 instances of "BUFFERS OFF"). > The following change in the machine commit seems out-of-place; are we > fixing a bug here? > > explain.c: > /* Show buffer usage in planning */ > - if (bufusage) > + if (bufusage && es->buffers) I think that was an extraneous change, maybe from a conflict while re-ordering the patches. Removed it and rebased. Thanks for looking. -- Justin
Вложения
Rebased. BTW, I think it may be that the GUC should be marked PGDLLIMPORT ?
Вложения
On Thu, 2022-10-20 at 21:09 -0500, Justin Pryzby wrote:
> Rebased.
I had a look at the patch set.
It applies and builds cleanly and passes the regression tests.
0001: Add GUC: explain_regress
I like the idea of the "explain_regress" GUC. That should simplify
the regression tests.
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -625,7 +625,7 @@ initialize_environment(void)
* user's ability to set other variables through that.
*/
{
- const char *my_pgoptions = "-c intervalstyle=postgres_verbose";
+ const char *my_pgoptions = "-c intervalstyle=postgres_verbose -c explain_regress=on";
const char *old_pgoptions = getenv("PGOPTIONS");
char *new_pgoptions;
@@ -2288,6 +2288,7 @@ regression_main(int argc, char *argv[],
fputs("log_lock_waits = on\n", pg_conf);
fputs("log_temp_files = 128kB\n", pg_conf);
fputs("max_prepared_transactions = 2\n", pg_conf);
+ // fputs("explain_regress = yes\n", pg_conf);
for (sl = temp_configs; sl != NULL; sl = sl->next)
{
This code comment is a leftover and should go.
0002: exercise explain_regress
This is the promised simplification. Looks good.
0003: Make explain default to BUFFERS TRUE
Yes, please!
This patch is independent from the previous patches.
I'd like this to be applied, even if the GUC is rejected.
(I understand that that would cause more changes in the regression
test output if we changed that without introducing "explain_regress".)
The patch changes the default value of "auto_explain.log_buffers"
to "on", which makes sense. However, the documentation in
doc/src/sgml/auto-explain.sgml should be updated to reflect that.
--- a/doc/src/sgml/perform.sgml
+++ b/doc/src/sgml/perform.sgml
@@ -731,8 +731,8 @@ EXPLAIN ANALYZE SELECT * FROM polygon_tbl WHERE f1 @> polygon '(0.5,2.0)';
</para>
<para>
- <command>EXPLAIN</command> has a <literal>BUFFERS</literal> option that can be used with
- <literal>ANALYZE</literal> to get even more run time statistics:
+ <command>EXPLAIN ANALYZE</command> has a <literal>BUFFERS</literal> option which
+ provides even more run time statistics:
<screen>
EXPLAIN (ANALYZE, BUFFERS) SELECT * FROM tenk1 WHERE unique1 < 100 AND unique2 > 9000;
This is not enough. The patch would have to update all the examples that use EXPLAIN ANALYZE.
I see two options:
1. Change the output of all the examples and move this explanation to the first example
with EXPLAIN ANALYZE:
The numbers in the <literal>Buffers:</literal> line help to identify which parts
of the query are the most I/O-intensive.
2. Change all examples that currently do *not* use BUFFERS to EXPLAIN (BUFFERS OFF).
Then you could change the example that shows BUFFERS to something like
If you do not suppress the <literal>BUFFERS</literal> option that can be used with
<command>EXPLAIN (ANALYZE)</command>, you get even more run time statistics:
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?
0005 suppresses "rows removed by filter", but how is that machine dependent?
> 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?
Yours,
Laurenz Albe
On Tue, Oct 25, 2022 at 03:49:14PM +0200, Laurenz Albe wrote:
> On Thu, 2022-10-20 at 21:09 -0500, Justin Pryzby wrote:
> > Rebased.
>
> I had a look at the patch set.
Thanks for looking
> @@ -2288,6 +2288,7 @@ regression_main(int argc, char *argv[],
> fputs("log_lock_waits = on\n", pg_conf);
> fputs("log_temp_files = 128kB\n", pg_conf);
> fputs("max_prepared_transactions = 2\n", pg_conf);
> + // fputs("explain_regress = yes\n", pg_conf);
>
> for (sl = temp_configs; sl != NULL; sl = sl->next)
> {
>
> This code comment is a leftover and should go.
No - I was wondering whether the GUC should be specified by the
environment or by the file; I think it's better by file. Then it also
needs to be in Cluster.pm.
> 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).
> 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).
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.
> 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.
--
Justin
Вложения
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
On Fri, Nov 04, 2022 at 11:46:03AM +0100, Laurenz Albe wrote: > "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. But it doesn't cause backpatching to be harder; if a patch is meant to be backpatched, its author can still choose to write the old way, with (COSTS OFF, ...) > 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. I don't intend to encourage putting lots of "explain analyze" in the tests. Rather, this makes that a reasonable possibility rather than something that people are discouraged from doing by its difficulty. Using "explain analyze" still needs to be evaluated (for speed and otherwise), same as always. In any case, I suggested to defer review of patches 004 and beyond, which are optional, and it distracts from the essential discussion about 001. > 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. You set the patch to "waiting on author", which indicates that there's no need for further input or review. But, I think that's precisely what's needed - without input from more people, what could I do to progress the patch ? I don't think it's reasonable to put 001 first and change thousands (actually, 1338) of regression results. If nobody wants to discuss 001, then this patch series won't progress. -- Justin
Justin Pryzby <pryzby@telsasoft.com> writes:
> You set the patch to "waiting on author", which indicates that there's
> no need for further input or review. But, I think that's precisely
> what's needed - without input from more people, what could I do to
> progress the patch ? I don't think it's reasonable to put 001 first and
> change thousands (actually, 1338) of regression results. If nobody
> wants to discuss 001, then this patch series won't progress.
Well ...
1. 0001 invents a new GUC but provides no documentation for it.
That certainly isn't committable, and it's discouraging the
discussion you seek because people have to read the whole patch
in detail to understand what is being proposed.
2. The same complaint for 0004, which labors under the additional
problem that MACHINE is one of the worst option names I've ever
seen proposed around here. It conveys *nothing* about what it does
or is good for. The fact that it's got no relationship to the GUC
name, and yet they're intimately connected, doesn't improve my
opinion of it.
3. I'm not really on board with the entire approach. Making
EXPLAIN work significantly differently for developers and test
critters than it does for everyone else seems likely to promote
confusion and hide bugs. I don't think getting rid of the need
for filter functions in test scripts is worth that.
4. I don't see how the current patches could pass under "make
installcheck" --- it looks to me like it only takes care of
applying the GUC change in installations built by pg_regress
or Cluster.pm. To make this actually work, you'd have to
add "explain_regress = on" to the ALTER DATABASE SET commands
issued for regression databases. That'd substantially increase
the problem of "it works differently than what users see", at
least for me --- I do a lot of stuff interactively in the
regression database.
5. The change in postgres_fdw.c in 0001 concerns me too.
Yeah, it will fix things if an updated postgres_fdw talks to
an updated server, but what about an older postgres_fdw?
Don't really want to see that case break; communication with
servers of different major versions is one of postgres_fdw's
main goals.
As a side note, 0001 also creates a hard break in postgres_fdw's
ability to work with pre-9.0 remote servers, which won't accept
"EXPLAIN (COSTS)". Maybe we don't care about that anymore, given
the policy we've adopted elsewhere that we won't test or support
interoperability with versions before 9.2. But we should either
make the command spelling conditional on remote version, or
officially move that goalpost.
regards, tom lane
On Sat, Nov 05, 2022 at 10:43:07AM -0400, Tom Lane wrote: > Justin Pryzby <pryzby@telsasoft.com> writes: > > You set the patch to "waiting on author", which indicates that there's > > no need for further input or review. But, I think that's precisely > > what's needed - without input from more people, what could I do to > > progress the patch ? I don't think it's reasonable to put 001 first and > > change thousands (actually, 1338) of regression results. If nobody > > wants to discuss 001, then this patch series won't progress. > > Well ... > > 1. 0001 invents a new GUC but provides no documentation for it. > That certainly isn't committable, and it's discouraging the > discussion you seek because people have to read the whole patch > in detail to understand what is being proposed. > > 2. The same complaint for 0004, which labors under the additional I suggested that the discussion be limited to the early patches (004 is an optional, possible idea that I had, but it's evidently still causing confusion). > 3. I'm not really on board with the entire approach. Making > EXPLAIN work significantly differently for developers and test > critters than it does for everyone else seems likely to promote > confusion and hide bugs. I don't think getting rid of the need > for filter functions in test scripts is worth that. I'm open to suggestions how else to move towards the goal. Note that there's a few other things which have vaguely-similar behavior: HIDE_TABLEAM=on HIDE_TOAST_COMPRESSION=on compute_query_id = regress Another possibility is to make a new "explain" format, like | explain(FORMAT REGRESS, ...). ...but I don't see how that's would be less objectionable than what I've written. The patch record should probably be closed until someone proposes another way to implement what's necessary to enable explain(BUFFERS) by default. -- Justin