Le mar. 12 nov. 2024 à 16:35, Guillaume Lelarge <guilla...@lelarge.info> a
écrit :
> Le mar. 12 nov. 2024 à 16:21, Robert Haas <robertmh...@gmail.com> a
> écrit :
>
>> On Mon, Nov 11, 2024 at 3:59 PM Guillaume Lelarge
>> <guilla...@lelarge.info> wrote:
>> > Agreed. Having an "EXPLAIN (ALL)" would be a great addition. I could
>> tell a customer to do an "EXPLAIN (ALL)", rather than first asking the
>> PostgreSQL release installed on the server and after that, giving the
>> correct options for EXPLAIN.
>>
>> I realize that you're probably going to hate my guts -- or hate them
>> even more than you do already -- but I doubt that a proposal to add
>> EXPLAIN (ALL) will go anywhere.
>
>
> I don't hate your guts :) and...
>
>
>> The definitional problem is that it is
>> not clear what to do with non-Boolean valued options, such as
>> SERIALIZE. People who think that we were wrong not to make SERIALIZE
>> TEXT the default in v17 will argue that EXPLAIN (ALL) should turn it
>> on; after all, the backward-compatibility argument carries no water in
>> that case. But people who do not like the behavior of SERIALIZE TEXT
>> will not be happy about that. They might directly make that argument,
>> or they might instead make the argument that ALL should do nothing
>> about a non-Boolean valued option. But that position is really quite
>> difficult to justify. Let's suppose that the current BUFFERS option,
>> which is Boolean, got replaced with BUFFERS { detailed | on | off }.
>> Well, then, by the principle that ALL only affects Boolean-valued
>> options, it's no longer included in EXPLAIN (ALL). Nobody will be
>> happy with that. Practically speaking, I think it will be very
>> difficult to get agreement on what EXPLAIN (ALL) should do, and I
>> think it is unlikely that anything will get committed no matter how
>> much time we spend arguing about it.
>>
>>
> ... I kinda agree with you. It would have been nice to have an "EXPLAIN
> (ALL)" but I completely understand the issue.
>
>
>> But I think we would get most of the same benefit from just doing what
>> David Rowley proposed and turning on EXPLAIN (BUFFERS) by default. I'd
>> suggest that we decide that, without ANALYZE, the option would not do
>> anything; that is already how TIMING works. So this would be a very
>> small patch and would probably get a lot of support from a lot of
>> people. It also wouldn't require users to change their habits or learn
>> any new syntax -- they could just keep typing EXPLAIN ANALYZE or
>> EXPLAIN ANALYZE VERBOSE and all would be well.
>>
>>
> That would be a nice enhancement.
>
>
>> And the same principle could be applied to other EXPLAIN options if
>> there is sufficient consensus. We could default to WAL ON, SERIALIZE
>> TEXT, and MEMORY ON, if we wanted to do that. However, the more we try
>> to change at once, the less likely it is that anything will happen at
>> all. For example, I personally believe that EXPLAIN (MEMORY) should be
>> ripped out of the server as both badly-named and mostly useless, so
>> I'm not going to vote in favor of turning it on by default; and I
>> wouldn't vote for enabling WAL by default because I have no experience
>> with it to suggest that it's routinely valuable and thus worth the
>> overhead. I would vote for SERIALIZE TEXT because I've seen that cause
>> gross distortion of EXPLAIN ANALYZE results on many occasions. But the
>> point is that other people will vote differently, so tying all the
>> proposals together just increases the chances of agreeing on nothing
>> at all.
>>
>>
> Agreed.
>
>
>> So to recap: everyone is free to propose whatever they like, and I am
>> not in charge here, but if you want to get something committed, the
>> proposal which I think has the highest chance of success is: propose
>> to make BUFFERS ON the default (but a noop without ANALYZE, similar to
>> how TIMING already works).
>>
>>
> Sounds like a plan.
>
>
Sure looks easy enough to do (though it still lacks doc and tests changes).
See patch attached.
--
Guillaume.
From 173a6103d8b4727ed626c4fbbdab5feeb5aa7280 Mon Sep 17 00:00:00 2001
From: Guillaume Lelarge <guillaume.lela...@dalibo.com>
Date: Tue, 12 Nov 2024 21:59:14 +0100
Subject: [PATCH] BUFFERS is ON by default when ANALYZE is ON
---
src/backend/commands/explain.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 7c0fd63b2f..d6815e01d1 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -198,6 +198,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
List *rewritten;
ListCell *lc;
bool timing_set = false;
+ bool buffers_set = false;
bool summary_set = false;
/* Parse options list. */
@@ -212,7 +213,10 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
else if (strcmp(opt->defname, "costs") == 0)
es->costs = defGetBoolean(opt);
else if (strcmp(opt->defname, "buffers") == 0)
+ {
+ buffers_set = true;
es->buffers = defGetBoolean(opt);
+ }
else if (strcmp(opt->defname, "wal") == 0)
es->wal = defGetBoolean(opt);
else if (strcmp(opt->defname, "settings") == 0)
@@ -292,6 +296,9 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
/* if the timing was not set explicitly, set default value */
es->timing = (timing_set) ? es->timing : es->analyze;
+ /* if the buffers was not set explicitly, set default value */
+ es->buffers = (buffers_set) ? es->buffers : es->analyze;
+
/* check that timing is used with EXPLAIN ANALYZE */
if (es->timing && !es->analyze)
ereport(ERROR,
--
2.47.0
