On 28/3/2025 14:09, Robert Haas wrote:
On Thu, Mar 27, 2025 at 5:34 PM Andrei Lepikhov wrote:
I’m afraid to sound like a bore, but I think pg_overexplain should
include a call into the hook call chain (see attachment). Who knows,
maybe this extension will be used someday in production?
Oh, bo
On Mon, Mar 17, 2025 at 11:54 PM Sami Imseih wrote:
> +1
>
> I did not think of adding a new hook, because there must be a really good
> reason to add a new hook. I think it's justified for this case. It's better
> than
> my approach since the extension author can just put all their checks in one
On Thu, Mar 20, 2025 at 3:04 AM Andrei Lepikhov wrote:
> I'm sorry, I was confused; previously, the difficulties faced by
> extension developers were always attributed to him (refer to the
> discussion on the selectivity hook). However, now you're introducing a
> hook for a trivial operation that
Sami Imseih writes:
>> It would be better to add the parameter "type: EXPLAIN_ONLY |
>> ANALYZE_ONLY | BOTH" to the RegisterExtensionExplainOption() routine.
>> This value will be saved inside the ExplainExtensionOption structure and
>> processed by the core inside the ParseExplainOptionList.
> h
> On Tue, Mar 18, 2025 at 11:21 PM Sami Imseih wrote:
> > > > Do you want to propose a patch?
> > >
> > > yes, will attach a patch shortly.
> >
> > Attached is a patch to add a hook to allow extensions
> > to add additional option validations. The hook takes
> > in the ExplainState as an argument
On Fri, Mar 28, 2025 at 5:39 AM Pavel Luzanov wrote:
> One more suggestion to improve the documentation.
> It lacks installations actions, something like in auto_explain:
>
> To use pg_overexplain, simply load it into the server.
> You can load it into an individual session:
>
> LOAD 'pg_overexpla
On Fri, Mar 28, 2025 at 12:13 AM Man Zeng wrote:
> // PG_MODULE_MAGIC;
> PG_MODULE_MAGIC_EXT(
> .name = "pg_overexplain",
> .version = PG_VERSION
> );
Good catch, committed.
--
Robert Haas
EDB: http://www.enterprise
On Thu, Mar 27, 2025 at 5:34 PM Andrei Lepikhov wrote:
> I’m afraid to sound like a bore, but I think pg_overexplain should
> include a call into the hook call chain (see attachment). Who knows,
> maybe this extension will be used someday in production?
Oh, bother, that was a silly mistake on my
One more suggestion to improve the documentation.
It lacks installations actions, something like in auto_explain:
To use pg_overexplain, simply load it into the server.
You can load it into an individual session:
LOAD 'pg_overexplain';
(You must be superuser to do that.)
Another way is to prelo
I tried it out and felt good about this feature.
But I found that the following information is not very complete.
postgres=# select * from pg_get_loaded_modules();
module_name | version | file_name
--+-+---
| | pg_overexplain.so
On Wed, Mar 19, 2025 at 11:38 AM Sami Imseih wrote:
> ... as I made the hook signature match that of
> ParseExplainOptionList, so both pstate and the options list
> are now available to the hook.
This version LGTM, except it's not pgindent-clean.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Thu, Mar 20, 2025 at 8:37 PM Sami Imseih wrote:
> 1/ provides a good example for future extensions on how to
> use the new hooks. it definitely would be nice to add an example
> for the hook added to check options against each other as mentioned
> by Andrei here [0] but I could not find a good
On 3/26/25 19:09, Robert Haas wrote:
On Sat, Mar 22, 2025 at 12:10 PM Robert Haas wrote:
I'm not going
to insist on shipping this if I'm the ONLY one who would ever get any
use out of it, but I doubt that's the case.
Hearing no other votes against this, I have committed it, but now I
wonder i
On Wed, Mar 26, 2025 at 10:40 PM Tom Lane wrote:
> Robert Haas writes:
> > - -> Index Scan using daucus_id_idx on daucus v2_2 (actual
> > rows=0.12 loops=8)
> > + -> Index Scan using daucus_id_idx on daucus v2_2 (actual
> > rows=0.13 loops=8)
>
> Even if this had not failed in t
Robert Haas writes:
> - -> Index Scan using daucus_id_idx on daucus v2_2 (actual
> rows=0.12 loops=8)
> + -> Index Scan using daucus_id_idx on daucus v2_2 (actual
> rows=0.13 loops=8)
Even if this had not failed in the buildfarm, it would certainly
have caused problems for someb
On Wed, Mar 26, 2025 at 02:09:32PM -0400, Robert Haas wrote:
> On Sat, Mar 22, 2025 at 12:10 PM Robert Haas wrote:
> > I'm not going
> > to insist on shipping this if I'm the ONLY one who would ever get any
> > use out of it, but I doubt that's the case.
>
> Hearing no other votes against this, I
On Wed, Mar 26, 2025 at 2:09 PM Robert Haas wrote:
> Hearing no other votes against this, I have committed it, but now I
> wonder if that is going to break the buildfarm, because I just noticed
> that the changes I made in v9 seem not to have resolved the problem
> with debug_parallel_query, for r
On Sat, Mar 22, 2025 at 12:10 PM Robert Haas wrote:
> I'm not going
> to insist on shipping this if I'm the ONLY one who would ever get any
> use out of it, but I doubt that's the case.
Hearing no other votes against this, I have committed it, but now I
wonder if that is going to break the buildf
On Sat, Mar 22, 2025 at 4:46 AM Andrei Lepikhov wrote:
> I skimmed through the code and tested how it works.
> It looks good and has no apparent architectural dependencies.
> But I haven't scrutinised it line-by-line and do not intend to do so.
> I wanna say I hate the design of this module. Havin
On 3/21/25 20:54, Robert Haas wrote:
On Fri, Mar 21, 2025 at 2:37 PM Tom Lane wrote:
Here's v9, which also adds 'SET debug_parallel_query = off' to the
pg_overexplain tests, per CI, because the test results are not (and
cannot realistically be made) stable under under that option.
I skimmed thr
Sami Imseih writes:
> overall this LGTM, and I don't really have other comments.
I took a look through v8, and have just a couple of trivial nits:
+static void overexplain_debug_handler(ExplainState *, DefElem *,
+ ParseState *);
+static void overexplain_rang
On Fri, Mar 21, 2025 at 2:37 PM Tom Lane wrote:
> I took a look through v8, and have just a couple of trivial nits:
Thank you!
> +static void overexplain_debug_handler(ExplainState *, DefElem *,
> + ParseState *);
> +static void overexplain_range_table_handle
> > explain (query_tree_show range_table) select ..
> > explain (query_tree_show qualification) select ..
> > explain (query_tree_show target_list) select ..
>
> But why would those options be exclusive of each other? Surely you
> could want more than one of those things, which suggests that separa
> It sounds like we're sufficiently in agreement so I've committed the
> patch.
Thanks!
> I've rebased my pg_overexplain patch and attach that here.
I spent some time playing around with this extension, and I can
see the value.
1/ provides a good example for future extensions on how to
use the
>
> On 19/3/2025 21:51, Sami Imseih wrote:
> >> Why do you think this hook is not redundant?
> > what is it redundant with?
> >
> >> It would be better to add the parameter "type: EXPLAIN_ONLY |
> >> ANALYZE_ONLY | BOTH" to the RegisterExtensionExplainOption() routine.
> >> This value will be saved
On 19/3/2025 21:51, Sami Imseih wrote:
Why do you think this hook is not redundant?
what is it redundant with?
It would be better to add the parameter "type: EXPLAIN_ONLY |
ANALYZE_ONLY | BOTH" to the RegisterExtensionExplainOption() routine.
This value will be saved inside the ExplainExtensio
On 19/3/2025 18:41, Sami Imseih wrote:
On Wed, Mar 19, 2025 at 11:38 AM Sami Imseih wrote:
... as I made the hook signature match that of
ParseExplainOptionList, so both pstate and the options list
are now available to the hook.
This version LGTM, except it's not pgindent-clean.
ugh, sorry
> Why do you think this hook is not redundant?
what is it redundant with?
> It would be better to add the parameter "type: EXPLAIN_ONLY |
> ANALYZE_ONLY | BOTH" to the RegisterExtensionExplainOption() routine.
> This value will be saved inside the ExplainExtensionOption structure and
> processed b
On Wed, Mar 19, 2025 at 1:41 PM Sami Imseih wrote:
> ugh, sorry about that. ran it for both modified files and it found one
> indentation correction from v2.
Yeah, that's the one I noticed. I think this should be uncontroversial
but I'll give it a day or two in case anyone wants to complain.
--
> On Wed, Mar 19, 2025 at 11:38 AM Sami Imseih wrote:
> > ... as I made the hook signature match that of
> > ParseExplainOptionList, so both pstate and the options list
> > are now available to the hook.
>
> This version LGTM, except it's not pgindent-clean.
ugh, sorry about that. ran it for both
On Tue, Mar 18, 2025 at 11:21 PM Sami Imseih wrote:
> > > Do you want to propose a patch?
> >
> > yes, will attach a patch shortly.
>
> Attached is a patch to add a hook to allow extensions
> to add additional option validations. The hook takes
> in the ExplainState as an argument and returns void
On Tue, Mar 18, 2025 at 8:02 AM Andrei Lepikhov wrote:
> Some questions:
> 1. I think, hooks ExplainOneQuery_hook_type, explain_per_plan_hook_type,
> explain_per_node_hook_type deserve to be moved to explain_format.h
> At least, inside the hook, we usually use functions like ExplainProperty.
-1,
On Tue, Mar 18, 2025 at 2:40 PM Tom Lane wrote:
> FWIW, I am fairly strongly against that. Every extension author is
> going to feel that their information is so important it should come
> first. Other people might have a different opinion about that, and
> in any case they can't all be first.
> > Do you want to propose a patch?
>
> yes, will attach a patch shortly.
Attached is a patch to add a hook to allow extensions
to add additional option validations. The hook takes
in the ExplainState as an argument and returns void.
It is expected the extension will raise an error if the
validati
Robert Haas writes:
> If you mean just after, that would amount to deciding that information
> coming from extensions goes before information from core rather than
> after. I thought about that and it's defensible, but in the end I
> thought it made more sense for core info to come first. We could
> Do you want to propose a patch?
yes, will attach a patch shortly.
--
Sami
On Tue, Mar 18, 2025 at 9:58 AM Andrei Lepikhov wrote:
> I agree with him, too. But, as you can see, I proposed not changing the
> first string or adding something there but just putting extension data
> under that line. Extra information about workers' state (not so
> important most of the time,
On 3/18/25 14:33, Robert Haas wrote:
On Tue, Mar 18, 2025 at 8:02 AM Andrei Lepikhov wrote:
Some questions:
1. I think, hooks ExplainOneQuery_hook_type, explain_per_plan_hook_type,
explain_per_node_hook_type deserve to be moved to explain_format.h
At least, inside the hook, we usually use funct
On 3/7/25 16:05, Robert Haas wrote:
I have attempted to use hooks, proposed in 0002, in my extensions.
At first, it worked great. My patch reduced a lot, and the only things
that I need in the planner to improve its predictions are the
selectivity hook and the create_plan hook - the last one nee
> You only
> need the second hook if you want to check the values of options
> against the values of other options.
+1
I did not think of adding a new hook, because there must be a really good
reason to add a new hook. I think it's justified for this case. It's better than
my approach since the ex
On Thu, Mar 13, 2025 at 5:52 PM Sami Imseih wrote:
> > The validation point is an interesting one. I agree that we don't
> > want the behavior to depend on the order in which options are
> > written.
>
> Here is what I applied on top of v6-0001 to correct this issue. Attaching it
> as a text file
On 3/12/25 20:58, Sami Imseih wrote:
I think this is a seriously bad idea. The first line is already
overloaded; we don't need several different extensions adding more
stuff to it.
Fair enough.
Plus, this doesn't consider what to do in non-text
output formats.
the hook will be a no-op for
> The validation point is an interesting one. I agree that we don't
> want the behavior to depend on the order in which options are
> written.
Here is what I applied on top of v6-0001 to correct this issue. Attaching it
as a text file only as Robert may have a different opinion on how to fix
this
> I think this is a seriously bad idea. The first line is already
> overloaded; we don't need several different extensions adding more
> stuff to it.
Fair enough.
> Plus, this doesn't consider what to do in non-text
> output formats.
the hook will be a no-op for non-text formats, which is not
d
Sami Imseih writes:
> 1/ As you can see form the output above, I used explain_per_node_hook
> to append a "Plan Node ID" to the explain output. I really don't like having
> it
> there, and prefer that it gets added to the top line of the node.
> i.e.
>-> Foreign Scan on t_r1 (cost=100.00..
Hi,
>>> EXPLAIN output. It wouldn't make sense for core to have an EXPLAIN
>>> option whose whole purpose is to cater to the needs of some extension,
>>> so that made me think of providing some extensibility infrastructure.
>> Making EXPLAIN extensible soun
On Thu, Mar 6, 2025 at 4:16 PM Tom Lane wrote:
> I find a good deal of attraction in getting rid of the IDs and
> just using names. Nor do I believe we need a hash table.
> (1) Surely there will not be so many extensions using this within a
> single EXPLAIN that a simple loop with strcmp()'s isn'
Robert Haas writes:
> If there is, maybe we should discard the integer IDs and just use
> strings directly. I could make GetExplainExtensionState() and
> SetExplainExtensionState() take const char *extension_name arguments
> instead of int extension_id, and just get rid of GetExplainExtensionId
>
Robert Haas writes:
> Here's v6, doing it that way. I found that the simplest thing to do
> was just push the call to GetExplainExtensionId() inside
> Get/SetExplainExtensionState().
Fair enough.
> Tom, what do you think?
The header comments for explain_state.c could use a spellcheck.
I noticed
Robert Haas writes:
> On Fri, Mar 7, 2025 at 9:38 AM Peter Eisentraut wrote:
>> Also, benign typedef redefinitions are a C11 feature. In practice, all
>> compilers currently in play support it, and the only problem you'll get
>> is from the buildfarm members that are explicitly set up to warn ab
On Fri, Mar 7, 2025 at 9:38 AM Peter Eisentraut wrote:
> Just to clarify this: Nobody has gone through and used IWYU to clean up
> indirect includes, as you appear to imagine here. My recent IWYU work
> was, besides putting some infrastructure in place, to clean up includes
> that are completely
On Thu, Mar 6, 2025 at 4:24 PM Robert Haas wrote:
> On Thu, Mar 6, 2025 at 4:16 PM Tom Lane wrote:
> > I find a good deal of attraction in getting rid of the IDs and
> > just using names. Nor do I believe we need a hash table.
> > (1) Surely there will not be so many extensions using this within
On 06.03.25 21:23, Robert Haas wrote:
On Wed, Mar 5, 2025 at 4:38 PM Tom Lane wrote:
v4 has addressed most of my nitpicks, but you still have typedefs
for ExplainState in both header files. My bet is that at least
one buildfarm animal will complain about that. I could be wrong
though, maybe a
On Wed, Mar 5, 2025 at 4:38 PM Tom Lane wrote:
> v4 has addressed most of my nitpicks, but you still have typedefs
> for ExplainState in both header files. My bet is that at least
> one buildfarm animal will complain about that. I could be wrong
> though, maybe all such compilers are in disuse n
Robert Haas writes:
> On Wed, Mar 5, 2025 at 4:00 PM Tom Lane wrote:
>> Got it. So does that mean we can remove any #include's from explain.h
>> after moving the struct definition?
> Good question. It looks like "lib/stringinfo.h" can come out but the
> other two are still needed, so there is n
On Wed, Mar 5, 2025 at 4:00 PM Tom Lane wrote:
> Ah. I was mistakenly assuming that 0001 would compile on its own ;-)
Oopsie.
> Got it. So does that mean we can remove any #include's from explain.h
> after moving the struct definition?
Good question. It looks like "lib/stringinfo.h" can come
Robert Haas writes:
> On Wed, Mar 5, 2025 at 1:53 PM Tom Lane wrote:
>> I'm quite confused by the #include additions in auto_explain.c and
>> file_fdw.c, and I strongly object to the ones in explain_state.h.
>> Surely those are unnecessary?
> They are necessary but they should have been part of
On Wed, Mar 5, 2025 at 1:53 PM Tom Lane wrote:
> Here's some comments on 0001 and 0002; I didn't have time for
> 0003 today. But in any case, I think you should move forward
> with committing 0001/0002 soon so other people can play around
> in this space. 0003 can be left for later.
Cool. Thank
Robert Haas writes:
> OK. It sounds to me like there is a good amount of support for going
> forward with something like what I have, even though some people might
> also like other things. What I feel is currently lacking is some
> review of the actual code. Would anyone like to do that?
Here's
On Wed, Mar 5, 2025 at 12:52 PM Jeff Davis wrote:
> It would be good to clarify whether this is for (a) experimenting with
> explain options that might be useful in core some day; or (b) special
> developer-only options that would never be useful in core; or (c)
> production-grade explain options
On Wed, Mar 5, 2025 at 12:20 PM Jeff Davis wrote:
> I didn't look into the technical details to see what might be required
> to allow that kind of collaboration, and I am not suggesting you
> redesign the entire feature around that idea.
OK. It sounds to me like there is a good amount of support
It would be good to clarify whether this is for (a) experimenting with
explain options that might be useful in core some day; or (b) special
developer-only options that would never be useful in core; or (c)
production-grade explain options specific to an extension.
On Tue, 2025-03-04 at 16:23 -050
On Tue, 2025-03-04 at 16:23 -0500, Robert Haas wrote:
> But, I'm doubtful that
> letting unrelated extensions try to share the same option name is
> that
> thing.
This sub-discussion started because we were wondering whether to prefix
the options. I'm just pointing out that, even if there is a co
On 4/3/2025 22:23, Robert Haas wrote:
On Tue, Mar 4, 2025 at 1:53 PM Jeff Davis wrote:
I don't expect there to be zillions of extensions that only add new and
exciting explain options. Instead, it seems more likely that all
TableAM and CustomScan extensions will have 1-3 new explain options,
an
+1 to the general idea, I didn't look at the patches yet.
On Fri, 2025-02-28 at 15:32 -0500, Robert Haas wrote:
> 1. It is much more verbose. I theorize that people will be unhappy
> about having to type EXPLAIN (pg_overexplain.range_table) rather than
> just EXPLAIN (range_table).
That was my fi
On Tue, Mar 4, 2025 at 1:53 PM Jeff Davis wrote:
> I don't expect there to be zillions of extensions that only add new and
> exciting explain options. Instead, it seems more likely that all
> TableAM and CustomScan extensions will have 1-3 new explain options,
> and that some of those might collid
On Tue, Mar 4, 2025 at 10:26 AM Andrei Lepikhov wrote:
> I wouldn't say there is a thread in the mailing list. I mentioned this
> direction of extensibility multiple times (for example, [1,2]) with no
> reaction. However, letting extensions show data in explan gives this
> idea additional impulse.
On 4/3/2025 16:14, Robert Haas wrote:
On Tue, Mar 4, 2025 at 10:12 AM Andrei Lepikhov wrote:
Also, I think this feature is quite close to the discussion on the
possibility of adding an extensible list field into Query, PlanState,
Plan, etc. nodes to let extensions gather and transfer some addit
On Tue, Mar 4, 2025 at 10:12 AM Andrei Lepikhov wrote:
> Also, I think this feature is quite close to the discussion on the
> possibility of adding an extensible list field into Query, PlanState,
> Plan, etc. nodes to let extensions gather and transfer some additional
> data starting with the firs
On 4/3/2025 15:23, Robert Haas wrote:
On Tue, Mar 4, 2025 at 8:56 AM Andrei Lepikhov wrote:
Passing through the patches, I would say that changing the order of 0001
and 0002 would make them more independent.
Hmm, I thought this order made sense, but I could reorder them if
there's some compel
On Tue, Mar 4, 2025 at 8:56 AM Andrei Lepikhov wrote:
> Passing through the patches, I would say that changing the order of 0001
> and 0002 would make them more independent.
Hmm, I thought this order made sense, but I could reorder them if
there's some compelling reason to do so. If there's no pa
On 28/2/2025 20:26, Robert Haas wrote:
So here are some patches.
Yes, this is a big pain for extension developers. As I remember, it was
discussed multiple times in the hackers' mailing list.
Because there is no explain node hook, I use a patch in almost each of
my extensions: I write optimisat
hed all of that stuff; this is all quite preliminary and
I'm not committed to the details. The only thing that would disappoint
me is if somebody said "this whole idea of making EXPLAIN extensible
is stupid and pointless and we shouldn't ever do it." I will argue
against
On Mon, Mar 3, 2025 at 9:14 AM Matthias van de Meent
wrote:
> I think you meant "some time prior to PostgreSQL 10".
> PostgreSQL 9.0 had 5 options, of which COSTS, BUFFERS, and FORMAT were
> newly added, so only before 9.0 we had 2 options.
> PostgreSQL 9.2 then added TIMING on top of that, for a
First, thanks to all who have said they like this idea. I wasn't
expecting this much enthusiasm, to be honest. Woohoo!
On Mon, Mar 3, 2025 at 8:27 AM Matheus Alcantara
wrote:
> It would make sense (or possible) to have some kind of validation that returns
> an error when using an option that is a
On Fri, 28 Feb 2025 at 20:26, Robert Haas wrote:
>
> Prior to PostgreSQL 10, EXPLAIN had just 2 options: VACUUM and
> ANALYZE.
I think you meant "some time prior to PostgreSQL 10".
PostgreSQL 9.0 had 5 options, of which COSTS, BUFFERS, and FORMAT were
newly added, so only before 9.0 we had 2 opti
Hi, +1 for the idea. I Haven't reviewed the patches yet, but I would like to
share some thoughts.
On Fri, Feb 28, 2025 at 5:32 PM Robert Haas wrote:
>
> > One thing I am wondering is whether extensions should be required to
> > prefix their EXPLAIN option with the extension name to avoid
> > coll
ails. The only thing that would disappoint
me is if somebody said "this whole idea of making EXPLAIN extensible
is stupid and pointless and we shouldn't ever do it." I will argue
against that vociferously. I think even what I have here is enough to
disprove that hypothesis, but I ha
Hi Robert,
thanks for working on this and +1 for the idea.
i have reviewed 1,2 patches using 3rd patch(pg_overexplain module) and they
LGTM,will review more the 3rd patch.
Regards,
Srinath Reddy Sadipiralla
EDB:http://www.enterprisedb.com
On Fri, Feb 28, 2025 at 3:09 PM Thom Brown wrote:
> "pg_overexplain"? I love this name! And the idea sounds like a natural
> evolution, so +1.
Thanks. I thought about things like pg_hyperexplain or
pg_explain_debug, but in the end I didn't like any of them better than
overexplain. :-)
> One thin
es. The hook design in 0002 is a bit
> simplistic and could be made more complex; there's lots of stuff that
> could be added to or removed from 0003, much of which comes down to
> what somebody hacking on the planner would actually want to see. I'm
> happy to bikeshed al
> EXPLAIN output. It wouldn't make sense for core to have an EXPLAIN
> option whose whole purpose is to cater to the needs of some extension,
> so that made me think of providing some extensibility infrastructure.
Making EXPLAIN extensible sounds like a good idea.. FWIW, There is a
On Fri, 28 Feb 2025 at 15:09, Thom Brown wrote:
> One thing I am wondering is whether extensions should be required to
> prefix their EXPLAIN option with the extension name to avoid
> collisions.
>
> If two extensions happen to choose the same name, it won't be possible
> to use both simultaneou
eliminary and
I'm not committed to the details. The only thing that would disappoint
me is if somebody said "this whole idea of making EXPLAIN extensible
is stupid and pointless and we shouldn't ever do it." I will argue
against that vociferously. I think even what I have here
84 matches
Mail list logo
