Thread: making EXPLAIN extensible

Prior to PostgreSQL 10, EXPLAIN had just 2 options: VACUUM and
ANALYZE. Now, we're up to 12 options, which is already quite a lot,
and there's plenty more things that somebody might like to do.
However, not all of those things necessarily need to be part of the
core code. My original reason for wanting to extend EXPLAIN was that I
was thinking about an extension that would want to do a bunch of
things and one of those things would be to add some information to the
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.

However, there are other use cases, too, basically any of the normal
reasons why extensibility is useful and desirable. You might need to
get some information out a query plan that 99% of people don't care
about. You could come up with your own way of formatting a query plan,
but that's a big pain. It's a lot nicer if you can just add the detail
that you care about to the EXPLAIN output without needing to modify
PostgreSQL itself. Even if you think of something that really ought to
be included in the EXPLAIN output by PostgreSQL, you can roll an
extension out much quicker than you can get a change upstreamed and
released. So I think EXPLAIN extensibility is, as a general concept,
useful.

So here are some patches.

0001 allows a loadable module to register new EXPLAIN options.
Currently, EXPLAIN (FUNGUS) will error out, but if you want to make it
work, this patch is for you. This patch also allows you to stash some
state related to your new option, or options, in the ExplainState.
Core options have hard-coded structure members; e.g. EXPLAIN (BUFFERS)
sets es->buffers. If you add EXPLAIN (FUNGUS), there won't be an
es->fungus, but you can get about the same effect using the new
facilities provided here.

0002 provides hooks that you can use to make your new EXPLAIN options
actually do something. In particular, this adds a new hook that is
called once per PlanState node, and a new nook that is called once per
PlannedStmt. Each is called at an appropriate point for you to tack on
more output after what EXPLAIN would already produce.

0003 adds a new contrib module called pg_overexplain, which adds
EXPLAIN (DEBUG) and EXPLAIN (RANGE_TABLE). I actually think this is
quite useful for planner hacking, and maybe a few more options would
be, too. Right now, if you want to see stuff that EXPLAIN doesn't
clearly show, you have to use SET debug_print_plan = true, and that
output is so verbose that finding the parts you actually want to see
is quite difficult. Assuming it gives you the details you need,
EXPLAIN (RANGE_TABLE) looks way, way better to me, and if we end up
committing these patches I anticipate using this semi-regularly.

There are plenty of debatable things in this patch set, and I mention
some of them in the commit messages. 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 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 that vociferously. I think even what I have here is enough to
disprove that hypothesis, but I have a bunch of ideas about how to do
more. Some of those require additional infrastructure and are best
proposed with that other infrastructure; some can be done with just
this, but I ran out of time to code up examples so here is what I have
got so far.

Hope you like it, sorry if you don't.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, 28 Feb 2025 at 19:26, Robert Haas <robertmhaas@gmail.com> wrote:
>
> Prior to PostgreSQL 10, EXPLAIN had just 2 options: VACUUM and
> ANALYZE. Now, we're up to 12 options, which is already quite a lot,
> and there's plenty more things that somebody might like to do.
> However, not all of those things necessarily need to be part of the
> core code. My original reason for wanting to extend EXPLAIN was that I
> was thinking about an extension that would want to do a bunch of
> things and one of those things would be to add some information to the
> 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.
>
> However, there are other use cases, too, basically any of the normal
> reasons why extensibility is useful and desirable. You might need to
> get some information out a query plan that 99% of people don't care
> about. You could come up with your own way of formatting a query plan,
> but that's a big pain. It's a lot nicer if you can just add the detail
> that you care about to the EXPLAIN output without needing to modify
> PostgreSQL itself. Even if you think of something that really ought to
> be included in the EXPLAIN output by PostgreSQL, you can roll an
> extension out much quicker than you can get a change upstreamed and
> released. So I think EXPLAIN extensibility is, as a general concept,
> useful.
>
> So here are some patches.
>
> 0001 allows a loadable module to register new EXPLAIN options.
> Currently, EXPLAIN (FUNGUS) will error out, but if you want to make it
> work, this patch is for you. This patch also allows you to stash some
> state related to your new option, or options, in the ExplainState.
> Core options have hard-coded structure members; e.g. EXPLAIN (BUFFERS)
> sets es->buffers. If you add EXPLAIN (FUNGUS), there won't be an
> es->fungus, but you can get about the same effect using the new
> facilities provided here.
>
> 0002 provides hooks that you can use to make your new EXPLAIN options
> actually do something. In particular, this adds a new hook that is
> called once per PlanState node, and a new nook that is called once per
> PlannedStmt. Each is called at an appropriate point for you to tack on
> more output after what EXPLAIN would already produce.
>
> 0003 adds a new contrib module called pg_overexplain, which adds
> EXPLAIN (DEBUG) and EXPLAIN (RANGE_TABLE). I actually think this is
> quite useful for planner hacking, and maybe a few more options would
> be, too. Right now, if you want to see stuff that EXPLAIN doesn't
> clearly show, you have to use SET debug_print_plan = true, and that
> output is so verbose that finding the parts you actually want to see
> is quite difficult. Assuming it gives you the details you need,
> EXPLAIN (RANGE_TABLE) looks way, way better to me, and if we end up
> committing these patches I anticipate using this semi-regularly.
>
> There are plenty of debatable things in this patch set, and I mention
> some of them in the commit messages. 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 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 that vociferously. I think even what I have here is enough to
> disprove that hypothesis, but I have a bunch of ideas about how to do
> more. Some of those require additional infrastructure and are best
> proposed with that other infrastructure; some can be done with just
> this, but I ran out of time to code up examples so here is what I have
> got so far.
>
> Hope you like it, sorry if you don't.

"pg_overexplain"? I love this name! And the idea sounds like a natural
evolution, so +1.

Some questions:

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 simultaneously.

In what order would the options be applied? Would it be deterministic,
or weighted within the extension's configuration, or based on the
order of the options in the list?

Would explain extensions be capable of modifying pre-existing core
option output, or just append to output?

Should there be a way of determining which lines are output by which
option? An extension may output similar output to core output, making
it difficult or impossible to discern which is which.

Does there need to be any security considerations so that things like
RLS don't inadvertently become leaky?

Thom

On Fri, Feb 28, 2025 at 3:09 PM Thom Brown <thom@linux.com> 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 thing I am wondering is whether extensions should be required to
> prefix their EXPLAIN option with the extension name to avoid
> collisions.

I considered that. One advantage of doing that is that you could
support autoloading. Right now, you have to LOAD 'pg_overexplain' or
put it in session_preload_libraries or shared_preload_libraries in
order to use it. If you required people to type EXPLAIN
(pg_overexplain.range_table) instead of just EXPLAIN (range_table),
then you could react to not finding any such option by trying to
autoload a .so with the part of the name before the dot.

But you can probably see that this idea has a couple of pretty serious
weaknesses:

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). One could try to address this by renaming
the extension to something shorter, like just 'oe'. Having to type
EXPLAIN (oe.range_table) wouldn't be nearly as annoying. However, this
seems like a pretty clear case of letting the tail wag the dog.

2. autoloading could have security concerns. This is probably fixable,
but we'd need to be sure that providing a new way to trigger loading a
module didn't open up any security holes.

> If two extensions happen to choose the same name, it won't be possible
> to use both simultaneously.

That's true. Of course, a lot depends on whether we end up with 3 or 5
or 8 EXPLAIN extensions or more like 30 or 50 or 80. In the former
case, the people writing those extensions will probably mostly know
about each other and can just use different names. In the latter case
it's a problem. My guess is it's the former case.

> In what order would the options be applied? Would it be deterministic,
> or weighted within the extension's configuration, or based on the
> order of the options in the list?

I'm not entirely sure I know which question you're asking here. If
you're asking what happens if two modules try to register the same
EXPLAIN option name and then a user uses it, one of the registrations
will win and the other will lose. I think the second one wins. As I
say above, I assume we'll find a way to not try to do that. However, I
think more likely you're asking: if you load pg_fingernailexplain and
pg_toenailexplain and then do EXPLAIN (toenail, fingernail) SELECT
..., in what order will the options take effect? For the answer to
that question, see the commit message for 0002.

> Would explain extensions be capable of modifying pre-existing core
> option output, or just append to output?

The interfaces we have are really only going to work for appending.
Modifying would be cool, but I think it's mostly impractical. We have
a framework for emitting stuff into EXPLAIN output in a way that takes
into account whether you're in text mode or json or yaml or whatever,
and this patch just builds on that existing framework to allow you to
make extra calls to those emit-some-output functions at useful places.
As a result, the patch is small and simple. If we had an existing
framework for modifying stuff, then we could perhaps provide suitable
places to call those functions, too. But they don't exist, and it's
not easy to see how they could be created. I think you would need some
kind of major redesign of explain.c, and I don't know how to do that
without making it bloated, slow, and unmaintainable.

If somebody comes up with a way of allowing certain limited types of
modifications to EXPLAIN output with small, elegant-looking code
changes, and if those changes seem like useful things for an extension
to want to do, I'm totally on board. But I currently don't have an
idea like that.

> Should there be a way of determining which lines are output by which
> option? An extension may output similar output to core output, making
> it difficult or impossible to discern which is which.

I don't think this is really going to be a problem.

> Does there need to be any security considerations so that things like
> RLS don't inadvertently become leaky?

It's possible that there may be some security considerations, and
that's worth thinking about. However, RLS disclaims support for
side-channel attacks, so it's already understood to be (very) leaky.

--
Robert Haas
EDB: http://www.enterprisedb.com

> 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
discussion [0]
for showing FDW remote plans ( postgres_fdw specifically), and I think
we  will need to
add some new options to EXPLAIN to make that possible.

Have not looked at your patches, but I will do so now.


Regards,

Sami Imseih
Amazon Web Services (AWS)


[0] https://www.postgresql.org/message-id/CAA5RZ0vXiOiodrNQ-Va4FCAkXMpGA%3DGZDeKjFBRgRvHGuW7s7Q%40mail.gmail.com

0002 provides hooks that you can use to make your new EXPLAIN options
actually do something. In particular, this adds a new hook that is
called once per PlanState node, and a new nook that is called once per
PlannedStmt. Each is called at an appropriate point for you to tack on
more output after what EXPLAIN would already produce.

0003 adds a new contrib module called pg_overexplain, which adds
EXPLAIN (DEBUG) and EXPLAIN (RANGE_TABLE). I actually think this is
quite useful for planner hacking, and maybe a few more options would
be, too. Right now, if you want to see stuff that EXPLAIN doesn't
clearly show, you have to use SET debug_print_plan = true, and that
output is so verbose that finding the parts you actually want to see
is quite difficult. Assuming it gives you the details you need,
EXPLAIN (RANGE_TABLE) looks way, way better to me, and if we end up
committing these patches I anticipate using this semi-regularly.


There are plenty of debatable things in this patch set, and I mention
some of them in the commit messages. 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 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 that vociferously. I think even what I have here is enough to
disprove that hypothesis, but I have a bunch of ideas about how to do
more. Some of those require additional infrastructure and are best
proposed with that other infrastructure; some can be done with just
this, but I ran out of time to code up examples so here is what I have
got so far.

Hope you like it, sorry if you don't.

while writing the AQO extension [0] we were just adding a hook (we called it ExplainOnePlan_hook) similar to this one to add a description for the node used in the plan, in particular its identifier and the cardinality that is used during query planning. We also added a guc that allows user to disable this debug information, since it can only be useful when analyzing a problematic query, but not all the time. Therefore, I think this work is necessary and needed to provide the necessary output of additional information about the plan, which may be necessary for extensions like this.

[0] https://github.com/postgrespro/aqo

-- 
Regards,
Alena Rybakina
Postgres Professional

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 <robertmhaas@gmail.com> wrote:
>
> > One thing I am wondering is whether extensions should be required to
> > prefix their EXPLAIN option with the extension name to avoid
> > collisions.
>
> I considered that. One advantage of doing that is that you could
> support autoloading. Right now, you have to LOAD 'pg_overexplain' or
> put it in session_preload_libraries or shared_preload_libraries in
> order to use it. If you required people to type EXPLAIN
> (pg_overexplain.range_table) instead of just EXPLAIN (range_table),
> then you could react to not finding any such option by trying to
> autoload a .so with the part of the name before the dot.
>
> But you can probably see that this idea has a couple of pretty serious
> weaknesses:
>
> 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). One could try to address this by renaming
> the extension to something shorter, like just 'oe'. Having to type
> EXPLAIN (oe.range_table) wouldn't be nearly as annoying. However, this
> seems like a pretty clear case of letting the tail wag the dog.
>
> 2. autoloading could have security concerns. This is probably fixable,
> but we'd need to be sure that providing a new way to trigger loading a
> module didn't open up any security holes.
>
> > If two extensions happen to choose the same name, it won't be possible
> > to use both simultaneously.
>
> That's true. Of course, a lot depends on whether we end up with 3 or 5
> or 8 EXPLAIN extensions or more like 30 or 50 or 80. In the former
> case, the people writing those extensions will probably mostly know
> about each other and can just use different names. In the latter case
> it's a problem. My guess is it's the former case.
>
It would make sense (or possible) to have some kind of validation that returns
an error when using an option that is ambiguous? If a option is unique for all
options registered via extensions than the extension name prefix is not needed,
otherwise an error is returned forcing the user to specify the extension name.

This is a similar behaviour when, e.g we have a where clause that is
referencing a column that is present in multiple tables used in the query:
ERROR:  42702: column reference "b" is ambiguous LINE 1: select * from t inner
join t2 on t.a = t2.a where b = 10;

--
Matheus Alcantara

On Fri, 28 Feb 2025 at 20:26, Robert Haas <robertmhaas@gmail.com> 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 options.
PostgreSQL 9.2 then added TIMING on top of that, for a total of 6
options prior to PostgreSQL 10.

> 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.

+1, Neon would greatly appreciate infrastructure to allow extending EXPLAIN.

> 0001 allows a loadable module to register new EXPLAIN options.
> Currently, EXPLAIN (FUNGUS) will error out, but if you want to make it
> work, this patch is for you. This patch also allows you to stash some
> state related to your new option, or options, in the ExplainState.
> Core options have hard-coded structure members; e.g. EXPLAIN (BUFFERS)
> sets es->buffers. If you add EXPLAIN (FUNGUS), there won't be an
> es->fungus, but you can get about the same effect using the new
> facilities provided here.

Does this work with parallel workers' stats?
I can't seem to figure out whether or where parallel workers would
pass through their extended explain statistics, mostly because of the
per-backend nature of ID generation making the pointers of
ExplainState->extension_state unshareable.

Kind regards,

Matthias van de Meent

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
<matheusssilv97@gmail.com> wrote:
> It would make sense (or possible) to have some kind of validation that returns
> an error when using an option that is ambiguous? If a option is unique for all
> options registered via extensions than the extension name prefix is not needed,
> otherwise an error is returned forcing the user to specify the extension name.

I'm not saying that this couldn't be done -- it definitely could --
but to me it seems somewhat pointless. I mean, you'd just get the
error when you try to load the second of the two extensions, and then
what are you supposed to do about it at that point? Really, it's
incumbent on EXPLAIN-extension developers to avoid picking names that
conflict with other EXPLAIN extensions that the same users might want
to use. If they don't do that, everything sucks. By jiggering things
around you can cause the result to be either (a) some options are
ignored (current behavior) or (b) errors occur during extension
loading (your proposal) or (c) errors occur at runtime or even (d)
suddenly the conflicting options need to be spelled in a very verbose
way with a module name prefix instead of using them normally. But I
think no matter which of those things happen, life is pretty bad for
the user. The only real solution, AFAICS, is for the extension
developers to be thoughtful about the option names that they pick.

I'm not saying that's definitely going to happen, and I can be
convinced to add something to the patch to cater to it. But only if we
all agree on exactly what to add and exactly why that's going to be
better than doing nothing. My current view - as explained above - is
that anything we add is just going to be increasing the amount of code
without really making the user experience any better. If that's
correct, it's better to just keep it simple, as I have done.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Mar 3, 2025 at 9:14 AM Matthias van de Meent
<boekewurm+postgres@gmail.com> 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 total of 6
> options prior to PostgreSQL 10.

Probably I meant 9 rather than 10, then.

> +1, Neon would greatly appreciate infrastructure to allow extending EXPLAIN.

Cool.

> Does this work with parallel workers' stats?
> I can't seem to figure out whether or where parallel workers would
> pass through their extended explain statistics, mostly because of the
> per-backend nature of ID generation making the pointers of
> ExplainState->extension_state unshareable.

I don't fully understand what your question is. I think there are a
couple of separate points to consider here.

First, I don't think we ever store an ExplainState in DSM. If we do,
then the per-backend nature of ID generation is a fundamental design
issue and needs to be rethought. Otherwise, I don't see why it
matters.

Second, I did not add a hook to allow an extension to add data to a
"Worker N" section. I'm open to suggestions.

Third, regardless of parallel query, there is a general problem with
this infrastructure if what you want to do is print out some
instrumentation data. Sure, the hooks might allow you to get control
at a point where you can print some stuff, but how are you supposed to
get the stuff you want to print? planduration, bufusage, and
mem_counters are passed down to ExplainOnePlan(); and there's other
stuff in struct Instrumentation that is used in ExplainNode(), but
those approaches don't seem to scale nicely to arbitrary new things
that somebody might want to measure. While I welcome ideas about how
to fix that, my current view is that it's a job for a separate patch
set.

In general, it's expected that each parallel-aware node may register a
shm_toc entry using the plan_node_id as the key. So if you wanted
per-worker instrumentation of any sort of some particular node, you
could possibly add it to that chunk of memory. This would work well,
for example, for a custom scan, or any other case where the node is
under the control over the same code that is trying to instrument
stuff. A patch to core could extend both the node's DSM footprint and
the explain.c code that prints data from it. However, if you want to
do something like "for every executor node, count the number of flying
spaghetti monster tendrils that pass through the computer during the
execution of that node," there's not really any great way of doing
that today, with or without this patch, and with or without parallel
query. I mean, you can patch core, but that's it; there's no
extensibility here.

I'm not sure if any of this is responsive to your actual question; if
not, please help me get on the right track.

Thanks,

--
Robert Haas
EDB: http://www.enterprisedb.com

On 28/02/2025 20:26, Robert Haas wrote:
> Prior to PostgreSQL 10, EXPLAIN had just 2 options: VACUUM and
> ANALYZE. Now, we're up to 12 options, which is already quite a lot,
> and there's plenty more things that somebody might like to do.
> However, not all of those things necessarily need to be part of the
> core code. My original reason for wanting to extend EXPLAIN was that I
> was thinking about an extension that would want to do a bunch of
> things and one of those things would be to add some information to the
> 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.
> 
> However, there are other use cases, too, basically any of the normal
> reasons why extensibility is useful and desirable. You might need to
> get some information out a query plan that 99% of people don't care
> about. You could come up with your own way of formatting a query plan,
> but that's a big pain. It's a lot nicer if you can just add the detail
> that you care about to the EXPLAIN output without needing to modify
> PostgreSQL itself. Even if you think of something that really ought to
> be included in the EXPLAIN output by PostgreSQL, you can roll an
> extension out much quicker than you can get a change upstreamed and
> released. So I think EXPLAIN extensibility is, as a general concept,
> useful.
> 
> So here are some patches.
> 
> 0001 allows a loadable module to register new EXPLAIN options.
> Currently, EXPLAIN (FUNGUS) will error out, but if you want to make it
> work, this patch is for you. This patch also allows you to stash some
> state related to your new option, or options, in the ExplainState.
> Core options have hard-coded structure members; e.g. EXPLAIN (BUFFERS)
> sets es->buffers. If you add EXPLAIN (FUNGUS), there won't be an
> es->fungus, but you can get about the same effect using the new
> facilities provided here.
> 
> 0002 provides hooks that you can use to make your new EXPLAIN options
> actually do something. In particular, this adds a new hook that is
> called once per PlanState node, and a new nook that is called once per
> PlannedStmt. Each is called at an appropriate point for you to tack on
> more output after what EXPLAIN would already produce.
> 
> 0003 adds a new contrib module called pg_overexplain, which adds
> EXPLAIN (DEBUG) and EXPLAIN (RANGE_TABLE). I actually think this is
> quite useful for planner hacking, and maybe a few more options would
> be, too. Right now, if you want to see stuff that EXPLAIN doesn't
> clearly show, you have to use SET debug_print_plan = true, and that
> output is so verbose that finding the parts you actually want to see
> is quite difficult. Assuming it gives you the details you need,
> EXPLAIN (RANGE_TABLE) looks way, way better to me, and if we end up
> committing these patches I anticipate using this semi-regularly.
> 
> There are plenty of debatable things in this patch set, and I mention
> some of them in the commit messages. 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 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 that vociferously. I think even what I have here is enough to
> disprove that hypothesis, but I have a bunch of ideas about how to do
> more. Some of those require additional infrastructure and are best
> proposed with that other infrastructure; some can be done with just
> this, but I ran out of time to code up examples so here is what I have
> got so far.
> 
> Hope you like it, sorry if you don't.
> 

I definitely LOVE it. I tried your patches and it works great. No real 
surprise here :) I tried to code my own library (entirely based on 
yours), and it's quite nice. Patch attached, not intended to be applied 
on the repo, but just a nice use case.

This library adds "Tip" line for each tip it can give on a specific 
node. Right now, it only handles "Rows Removed by Filter" on a 
sequential scan, but there's much more we could add to it.

Here is an example on how to use it:

postgres=# show shared_preload_libraries ;
  shared_preload_libraries
--------------------------
  pg_explaintips
(1 row)

postgres=# create table t1 (id integer); 
  

CREATE TABLE 
  

postgres=# insert into t1 select generate_series(1, 1000); 
  

INSERT 0 1000
postgres=# explain (analyze,costs off,tips) select * from t1 where id>2; 

                           QUERY PLAN
---------------------------------------------------------------
  Seq Scan on t1 (actual time=0.042..0.337 rows=998.00 loops=1)
    Filter: (id > 2)
    Rows Removed by Filter: 2
    Buffers: shared hit=5
  Planning:
    Buffers: shared hit=4
  Planning Time: 0.079 ms
  Execution Time: 0.479 ms
(8 rows)

postgres=# explain (analyze,costs off,tips) select * from t1 where id<2; 

                          QUERY PLAN
-------------------------------------------------------------
  Seq Scan on t1 (actual time=0.014..0.113 rows=1.00 loops=1)
    Filter: (id < 2)
    Rows Removed by Filter: 999
    Buffers: shared hit=5
    Tips: You should probably add an index!
  Planning Time: 0.035 ms
  Execution Time: 0.127 ms
(7 rows)

postgres=# explain (analyze,costs off,tips off) select * from t1 where 
id<2;
                          QUERY PLAN
-------------------------------------------------------------
  Seq Scan on t1 (actual time=0.009..0.067 rows=1.00 loops=1)
    Filter: (id < 2)
    Rows Removed by Filter: 999
    Buffers: shared hit=5
  Planning:
    Buffers: shared hit=5
  Planning Time: 0.070 ms
  Execution Time: 0.076 ms
(8 rows)

Just great. Hope your patchs will find their way in the 18 release. 
Thanks a lot.


-- 
Guillaume Lelarge
Consultant
https://dalibo.com

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 optimisation helpers, and it is necessary to show 
which node was influenced and how. I guess pg_hint_plan will also profit 
from this extra extensibility.

Passing through the patches, I would say that changing the order of 0001 
and 0002 would make them more independent.
Also, I'm ok with the floating order of extension messages in the 
explain output. We get used to living with dependencies on extension 
load order (pg_stat_statements quite annoyingly impacts queryid, for 
example), and this issue should be solved generally, in my opinion.
I support the way where extensions are allowed to print info but not 
restructure or remove something.
Wait for the commit!

-- 
regards, Andrei Lepikhov

On Tue, Mar 4, 2025 at 8:56 AM Andrei Lepikhov <lepihov@gmail.com> 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 particularly
compelling reason, it would be less work to commit them in this order.

> Also, I'm ok with the floating order of extension messages in the
> explain output. We get used to living with dependencies on extension
> load order (pg_stat_statements quite annoyingly impacts queryid, for
> example), and this issue should be solved generally, in my opinion.

I've often thought that the solution to this class of problems could
be to have extensions not manipulate a hook variable directly, but
instead call a function to which they pass their callback function and
an integer priority. Then we could call hook functions in priority
order. One difficulty is that this requires extension authors to agree
on what the priority order should be. In some cases that might not be
too hard, but it isn't apparent how it would work here.

IMHO, it's reasonable for the author of an EXPLAIN extension to say
"well, I see Robert already created an extension with an option called
DEBUG, so I will name my option TROUBLESHOOT," or something of that
sort. But if Robert gave the DEBUG hook priority 50, should I also
give my hook priority 50, or should I make it 40 or 25 or 1 or 100 or
what? Even if I know about all of the other extensions it's not really
clear what I should do. Actually, in this case, it feels like it would
be better if the user could control the ordering somehow, but I feel
like that might be over-engineering.

> I support the way where extensions are allowed to print info but not
> restructure or remove something.
> Wait for the commit!

Cool, thanks.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 4/3/2025 15:23, Robert Haas wrote:
> On Tue, Mar 4, 2025 at 8:56 AM Andrei Lepikhov <lepihov@gmail.com> 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 particularly
> compelling reason, it would be less work to commit them in this order.
I have no compelling reason except avoiding adding/removing explain.h 
into the head of auto_explain.c and file_fdw.c
> 
>> Also, I'm ok with the floating order of extension messages in the
>> explain output. We get used to living with dependencies on extension
>> load order (pg_stat_statements quite annoyingly impacts queryid, for
>> example), and this issue should be solved generally, in my opinion.
> 
> I've often thought that the solution to this class of problems could
> be to have extensions not manipulate a hook variable directly, but
> instead call a function to which they pass their callback function and
> an integer priority. Then we could call hook functions in priority
> order. One difficulty is that this requires extension authors to agree
> on what the priority order should be. In some cases that might not be
> too hard, but it isn't apparent how it would work here.
> 
> IMHO, it's reasonable for the author of an EXPLAIN extension to say
> "well, I see Robert already created an extension with an option called
> DEBUG, so I will name my option TROUBLESHOOT," or something of that
> sort. But if Robert gave the DEBUG hook priority 50, should I also
> give my hook priority 50, or should I make it 40 or 25 or 1 or 100 or
> what? Even if I know about all of the other extensions it's not really
> clear what I should do. Actually, in this case, it feels like it would
> be better if the user could control the ordering somehow, but I feel
> like that might be over-engineering.
I think the same way. It would be clearer for an observer to have a 
dependency on load order everywhere than different orders in different 
places with no strong guarantees.

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 first 'analyze' hook up to the end of execution.
For example, in solving user issues, I frequently need to know 
predictions on the number of groups in Memoize, IncrementalSort and some 
other nodes. Such extensibility could allow an extension to gather such 
internal data during the planning stage and show it in the explain 
without any changes in the core!

-- 
regards, Andrei Lepikhov

On Tue, Mar 4, 2025 at 10:12 AM Andrei Lepikhov <lepihov@gmail.com> 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 first 'analyze' hook up to the end of execution.
> For example, in solving user issues, I frequently need to know
> predictions on the number of groups in Memoize, IncrementalSort and some
> other nodes. Such extensibility could allow an extension to gather such
> internal data during the planning stage and show it in the explain
> without any changes in the core!

If you're saying there's a -hackers discussion on this, could you
provide a link? I haven't seen it.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 4/3/2025 16:14, Robert Haas wrote:
> On Tue, Mar 4, 2025 at 10:12 AM Andrei Lepikhov <lepihov@gmail.com> 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 first 'analyze' hook up to the end of execution.
>> For example, in solving user issues, I frequently need to know
>> predictions on the number of groups in Memoize, IncrementalSort and some
>> other nodes. Such extensibility could allow an extension to gather such
>> internal data during the planning stage and show it in the explain
>> without any changes in the core!
> 
> If you're saying there's a -hackers discussion on this, could you
> provide a link? I haven't seen it.
> 
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.

[1] 
https://www.postgresql.org/message-id/30113d59-8678-49ca-a8fb-bbceacf7efb0%40gmail.com
[2] 
https://www.postgresql.org/message-id/CA%2BTgmoYXgBVCnFhrW3X1NxpdjWtJCYRKP38PQ-AdR-RJziTBUQ%40mail.gmail.com

-- 
regards, Andrei Lepikhov

On Tue, Mar 4, 2025 at 10:26 AM Andrei Lepikhov <lepihov@gmail.com> 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.

I agree that it's worth considering. I'd like to get this patch set
committed first, because I feel like it's already good enough to let
people do a bunch of cool stuff, and then we can build on top of it
later to let people do even more cool stuff. I do have some ideas that
involve piping plan-time data through to the final plan so that it can
be made visible via EXPLAIN. However, I think there are several
challenging design questions that need to be figured out, including:

(1) exactly how do we pipe that plan-time data through to the final plan?

(2) how should the plan-time hooks be designed to let people do as
many interesting things as possible with as few hooks as possible?

It sounds like you might already have some ideas about how those
questions should be answered, but I haven't thought about it enough
yet to feel confident and I don't want to make those decisions now.
Let's keep this thread focused on these patches, and we can look at
what else to do once that's done.

--
Robert Haas
EDB: http://www.enterprisedb.com

+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 first reaction.

> That's true. Of course, a lot depends on whether we end up with 3 or
> 5
> or 8 EXPLAIN extensions or more like 30 or 50 or 80. In the former
> case, the people writing those extensions will probably mostly know
> about each other and can just use different names.

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 collide. For example, several may have a
EXPLAIN(PUSHDOWN) option that explains what work is being pushed down
into the TableAM/CustomScan.

In that case it's not even clear to me that a collision is a problem.
Would you really only want pushdown information from extension A, and
be upset that it also emits pushdown information from extension B?
Maybe we should just allow multiple extensions to use the same option
name?

Regards,
    Jeff Davis

On Tue, Mar 4, 2025 at 1:53 PM Jeff Davis <pgsql@j-davis.com> 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 collide. For example, several may have a
> EXPLAIN(PUSHDOWN) option that explains what work is being pushed down
> into the TableAM/CustomScan.
>
> In that case it's not even clear to me that a collision is a problem.
> Would you really only want pushdown information from extension A, and
> be upset that it also emits pushdown information from extension B?
> Maybe we should just allow multiple extensions to use the same option
> name?

One fairly big problem with that idea is that options need not be
simple Booleans.  If extension A adds PUSHDOWN { 'summary' | 'detail'
} and extension B adds PUSHDOWN { 'scan' | 'join' | 'agg' }, it's
definitely not going to work, even if we arrange to call handlers for
both extensions. If we want to have EXPLAIN keywords that have
well-defined meanings that span across different
FDWs/tableAMs/whatever, I think we should add those to core and
document what we expect extensions to do. This mechanism is really for
cases where you need a completely new option that only you will care
about.

Actually, I don't think custom scans or table AMs are the design
center for this feature. Keep in mind that, for a custom scan, we
already have ExplainCustomScan as part of CustomScanState->methods. We
don't currently have anything similar for table AMs, and you could
work around that with these hooks, by checking every node to see
whether it's a scan node and if so whether it scans a relation that
matches your table AM, but it would probably be better to have a
method for it, similar to what we have for CustomScan, if this is
something people want to do. It would be more efficient and require
less code.

Granted, those interfaces don't let you add completely new options,
but I think actually for custom scans and table AMs you most likely
want to just display whatever details you have unconditionally, or
conditional on es->verbose. I'm not sure there's a real reason to add
a new option that gates, say, postgres_fdw's display of join pushdown
information. That seems awfully specific. You'd need a reason why you
would want to control that display separately from everything else,
and the use case for that seems thin.

Where I see this being more useful is for people who want to display
additional information for plan nodes that they did not implement. For
example, my EXPLAIN (RANGE_TABLE) option dumps every
range-table-related fact it can find in the Plan tree. That includes
both information about which plan nodes (as displayed by EXPLAIN) are
scanning which RTIs, and also a list of all the RTIs and a bunch of
properties of each one. When you're trying to show information by
topic, as in this case, hooks on individual node types like custom
scan or (hypothetically) table AM callbacks don't help you get the job
done -- and neither do the existing options.

That having been said, the vigorous response to this proposal so far
suggests to me that people probably will want to use this for things
that are a bit different than what I had in mind, and that's fine.
Maybe there's even some way to adjust this proposal so that it can
suit some of those other use cases better. But, I'm doubtful that
letting unrelated extensions try to share the same option name is that
thing.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 4/3/2025 22:23, Robert Haas wrote:
> On Tue, Mar 4, 2025 at 1:53 PM Jeff Davis <pgsql@j-davis.com> 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 collide. For example, several may have a
>> EXPLAIN(PUSHDOWN) option that explains what work is being pushed down
>> into the TableAM/CustomScan.
>>
>> In that case it's not even clear to me that a collision is a problem.
>> Would you really only want pushdown information from extension A, and
>> be upset that it also emits pushdown information from extension B?
>> Maybe we should just allow multiple extensions to use the same option
>> name?
> ...
> Actually, I don't think custom scans or table AMs are the design
> center for this feature. Keep in mind that, for a custom scan, we
> already have ExplainCustomScan as part of CustomScanState->methods. We
> don't currently have anything similar for table AMs, and you could
> work around that with these hooks, by checking every node to see
> whether it's a scan node and if so whether it scans a relation that
> matches your table AM, but it would probably be better to have a
> method for it, similar to what we have for CustomScan, if this is
> something people want to do. It would be more efficient and require
> less code.
+1. In my experience, ExplainCustomScan has always been enough for the 
CustomScan node.
As for extensions collision - for now, it would be better to treat 
extensions as independent actors, suggesting that the developer, 
designing a software solution based on an extensions' constellation, 
will arrange their behaviour during development.
For instance,  if your library exports a function or variable, adding a 
prefix is essential to prevent overlapping functions when another 
library is loaded.
I recall that Yurii Rashkovskii is a proponent of using multiple 
extensions within a single installation. Perhaps he has insights on this 
topic?

-- 
regards, Andrei Lepikhov

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 collision,
and it happened to work, it's as likely to be a feature as a bug.

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.

Regards,
    Jeff Davis

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 -0500, Robert Haas wrote:
> Where I see this being more useful is for people who want to display
> additional information for plan nodes that they did not implement.
> For
> example, my EXPLAIN (RANGE_TABLE) option dumps every
> range-table-related fact it can find in the Plan tree.

That sounds like (b) or maybe (a).

But the first motivation you listed when introducing the patch was: "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."

which sounds like (c).

And if (c) is part of the intended use, but not CustomScan or TableAM,
then what kind of extensions would need extension-specific explain
options?

I am not trying to push the patch in any particular direction. On the
contrary, I'd just like to know the scope of the feature so that I can
stop accidentally pushing it in some direction by asking questions
about out-of-scope use cases.

Regards,
    Jeff Davis

On Wed, Mar 5, 2025 at 12:20 PM Jeff Davis <pgsql@j-davis.com> 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 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 is a v2 with some documentation and regression tests for
pg_overexplain added.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Mar 5, 2025 at 12:52 PM Jeff Davis <pgsql@j-davis.com> 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 specific to an extension.

I think it could be any of these.

> And if (c) is part of the intended use, but not CustomScan or TableAM,
> then what kind of extensions would need extension-specific explain
> options?
>
> I am not trying to push the patch in any particular direction. On the
> contrary, I'd just like to know the scope of the feature so that I can
> stop accidentally pushing it in some direction by asking questions
> about out-of-scope use cases.

Heh, no problem. I see it probably being most useful for extensions
that touch the planner in some way that cuts across multiple node
types. We don't have a lot of those in contrib yet, but that's partly
because we don't have much infrastructure to support them.

Alena Rybakina gives an example upthread: "while writing the AQO
extension [0] we were just adding a hook (we called it
ExplainOnePlan_hook) similar to this one to add a description for the
node used in the plan, in particular its identifier and the
cardinality that is used during query planning." Andrei Lephikov's
comment upthread suggests similar things: "I write optimisation
helpers, and it is necessary to show which node was influenced and
how".

Another example - and the one that motivated this work - was my
proposal http://postgr.es/m/CA+TgmoZY+baV-T-5ifDn6P=L=aV-VkVBrPmi0TQkcEq-5Finww@mail.gmail.com
- this is an offshoot of that work, several steps removed.

I'm not sure if I'm being clear here, but the difference between these
examples and the ones you proposed is whether what you're adding to
the planner in your extension looks more like a new type of node or
more like a new overall feature. A Custom Scan node or a new table AM
is a new kind of thing that you can scan; those sorts of examples
probably want to solve their problems in some other way, though they
could use this infrastructure if they really wanted. On the other
hand, Alena's example of adaptive query optimization or my example of
letting extensions nudge the planner are a new type of optimizer
capability that happens to live in an extension. They don't show up in
a specific place in the plan; they affect things at some higher level.
That kind of thing is where I expect this infrastructure to be high
value.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> 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 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.

0001:

GetExplainExtensionState and SetExplainExtensionState should probably
have guards against extension_id < 0, even if it's just an Assert.
Or make the id unsigned?

SetExplainExtensionState is repalloc'ing ExplainExtensionNameArray;
why?  Wouldn't that invalidate any other pointers to the array?
It disturbs me that you have ExplainState carrying what could
easily become a stale pointer to that array; I'd try to get rid
of that.

RegisterExtensionExplainOption has "char *option_name", but I think
that should be "const char *", and the function should have the same
disclaimer as GetExplainExtensionId about that string needing to be
constant-or-never-freed.

This bit in explain.h seems non-idiomatic:

+struct ExplainState;
+typedef struct ExplainState ExplainState;

AFAIK it's sufficient to write the typedef line.  Also I'd
strongly recommend a comment saying that the struct is
defined in explain_state.h.

explain_state.h has the same pattern:

+struct ExplainState;
+typedef struct ExplainState ExplainState;

This is a little more problematic, because I'm pretty sure some
compilers will kvetch when they see the duplicate typedefs.
You need to either (a) write the typedef in only one file,
and use "struct ExplainState" in the other file, or (b)
use some dance like

#ifndef EXPLAINSTATE_TYPEDEF_DEFINED
typedef struct ExplainState ExplainState;
#define EXPLAINSTATE_TYPEDEF_DEFINED 1
#endif

in both files.  If memory serves we do have some headers using
the (b) pattern, but I don't love it particularly.

0002:

I'm fine with the order of additions being determined by module
load order.  Users who are feeling picky about that can arrange
the module load order as they wish.  If we put in a priority
mechanism then the order would be determined by module authors,
who probably don't want the responsibility, and not by the users
whose results are actually affected.

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?

Anyway, these are all very minor concerns; overall I think
it's going in the right direction.

            regards, tom lane

On Wed, Mar 5, 2025 at 1:53 PM Tom Lane <tgl@sss.pgh.pa.us> 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 you very much for the review!

> GetExplainExtensionState and SetExplainExtensionState should probably
> have guards against extension_id < 0, even if it's just an Assert.
> Or make the id unsigned?

I like the Assert() idea better, so I added that. This way, an
extension can write "static int es_extension_id = -1;" or similar and
the Assert() will catch use-before-initialization errors.

> SetExplainExtensionState is repalloc'ing ExplainExtensionNameArray;
> why?  Wouldn't that invalidate any other pointers to the array?

Wow, ouch. That's a brown-paper-bag bug. It should be allocating and
then reallocating es->extension_state. The point is to make sure the
assignment at the tail end of the function isn't indexing off the
allocated portion of the array.

> RegisterExtensionExplainOption has "char *option_name", but I think
> that should be "const char *", and the function should have the same
> disclaimer as GetExplainExtensionId about that string needing to be
> constant-or-never-freed.

Added.

> This bit in explain.h seems non-idiomatic:
> explain_state.h has the same pattern:

Fixed, I hope.

> 0002:
>
> I'm fine with the order of additions being determined by module
> load order.  Users who are feeling picky about that can arrange
> the module load order as they wish.  If we put in a priority
> mechanism then the order would be determined by module authors,
> who probably don't want the responsibility, and not by the users
> whose results are actually affected.

Check.

> 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 0001. Because
0001 moves the definition of ExplainState to explain_state.h, files
that need to access to the members of that structure now need to
include that header file. As for the includes in explain_state.h, it
needs definitions for DefElem, ParseState, and PlannedStmt.

> Anyway, these are all very minor concerns; overall I think
> it's going in the right direction.

I am very happy to hear that. Thanks!

v3 attached.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 5, 2025 at 1:53 PM Tom Lane <tgl@sss.pgh.pa.us> 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 0001.

Ah.  I was mistakenly assuming that 0001 would compile on its own ;-)

> Because
> 0001 moves the definition of ExplainState to explain_state.h, files
> that need to access to the members of that structure now need to
> include that header file. As for the includes in explain_state.h, it
> needs definitions for DefElem, ParseState, and PlannedStmt.

Got it.  So does that mean we can remove any #include's from explain.h
after moving the struct definition?

            regards, tom lane

On Wed, Mar 5, 2025 at 4:00 PM Tom Lane <tgl@sss.pgh.pa.us> 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 out but the
other two are still needed, so there is not much gain. But I've made
that change in the attached v4.

I also added "= NULL" to a couple of variable initialization in 0003
in this version, because Andres pointed out to me that cfbot was
seeing compiler warnings. It's a little odd that it warned, because
the variables were initialized by a switch over all enum values, but I
guess some compiler doesn't find that convincing enough.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 5, 2025 at 4:00 PM Tom Lane <tgl@sss.pgh.pa.us> 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 not much gain. But I've made
> that change in the attached v4.

OK.

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 now.

Also I noted one more nitpick: I think SetExplainExtensionState
needs to grow the array with repalloc0 not repalloc, else you
risk GetExplainExtensionState returning garbage pointers for
not-yet-set array entries.

In perhaps-less-nitpicky territory, I wonder how well the
"per backend" notion of extension and option IDs plays with
extensions that assign those during _PG_init, as I see you've
done in 0003.  What I'm concerned about is what happens when
the extension is loaded using shared_preload_libraries.  I
think what will happen is

(1) IDs will be assigned in the postmaster, and nothing bad
happens as long as TopMemoryContext already exists, which it will.

(2) Such IDs propagate to backends via fork(), and thus are
effectively system-wide.

(3) But ... in an EXEC_BACKEND build, it doesn't work like
that.  Child processes will repeat the postmaster's processing
of shared_preload_libraries.  Probably they will load the
modules in the same order and get the same ID assignments,
but I'm not quite sure that's guaranteed.

In short, I wonder if somebody might build code that depends
on the IDs being the same across a cluster, and find that
it mostly works but sometimes not on Windows.  Maybe we don't
care given that we explicitly disclaimed them being the same.
But it's probably worth a bit of thought.

            regards, tom lane

On Wed, Mar 5, 2025 at 4:38 PM Tom Lane <tgl@sss.pgh.pa.us> 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 now.

Ugh, I suck at this, sorry. Adjusted in v5. It's hard to avoid the
conclusion that our IWYU configuration must be fairly lenient, because
every change seems to surface more source files that are depending on
indirect includes.

> Also I noted one more nitpick: I think SetExplainExtensionState
> needs to grow the array with repalloc0 not repalloc, else you
> risk GetExplainExtensionState returning garbage pointers for
> not-yet-set array entries.

Hardly a nitpick, thanks. I've tried to fix this.

> In short, I wonder if somebody might build code that depends
> on the IDs being the same across a cluster, and find that
> it mostly works but sometimes not on Windows.  Maybe we don't
> care given that we explicitly disclaimed them being the same.
> But it's probably worth a bit of thought.

I think that if the only thing a backend does with the ID returned by
GetExplainExtensionId is call GetExplainExtensionState() and
SetExplainExtensionState(), there's no problem here. So to me, the
question here is whether there's a reason to use that ID for anything
else.

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
entirely. I chose the current design because I thought that the IDs
did not need to be shared and I thought it would be a bit extravagant
to have an EXPLAIN extension need to do a hash table lookup for every
plan node, but maybe it wouldn't really matter in practice.

But I wonder what else you think that the ID might get used for. If
you're trying to do some kind of general coordination across all
backends using shared memory, we have ShmemInitStruct() and
GetNamedDSMSegment() for that purpose, and those do indeed use names,
and an extension should probably use those interfaces for that kind of
work rather than trying to do something with the explain extension ID
that is not intended.

The best argument that I can see for maybe using that extension ID for
some other purpose is parallel query. The ExplainState is not shared
across backends and presumably never will be, so there's no immediate
problem. But you can perhaps imagine someone wanting to do more than
this infrastructure allows. In particular, I'm imagining that people
might want to run extensions that annotate parts of the plan with
extension-provided data. If such annotations were added at plan time,
and if they used the extension ID, that still wouldn't give rise to
any cross-backend use, but if they were added at execution time, then
parallel query could be happening, and if the extension ID were then
used, trouble could occur.

But to me that sounds a little too hypothetical for me to be concerned
about it. For one thing, we haven't actually agreed on doing anything
of that sort. More importantly, using integer extension IDs for this
feature does not require that we use the integer extension IDs for
that feature, if and when it gets implemented, and it certainly
doesn't require that we use the same ones. And I feel like it might be
slightly surprising if it did, because they seem like two different
features that maybe ought to work in different ways. Those
hypothetical future features seem like part of the planner or part of
the executor, respectively, so I can't really see a reason why they
would pull their main identifier out of the EXPLAIN system.

So I guess I can't really think of a convincing reason why the current
design would cause a problem for anybody. Of course, my imagination
might be lacking.

Regarding commit timeframe, you expressed a preference for 0001 and
0002 to be committed "soon," but I'm going to be leaving town for a
week on Saturday and would prefer not to put myself in a situation
where somebody is expecting me to fix the buildfarm while I am gone.
So I'm happy to commit them today or tomorrow if you don't mind being
responsible for anything that blows up while I'm gone, or I'm happy to
have you commit them whenever you want, but barring one of those
outcomes, I'm going to deal with it on or after March 17th.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> 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
> entirely. I chose the current design because I thought that the IDs
> did not need to be shared and I thought it would be a bit extravagant
> to have an EXPLAIN extension need to do a hash table lookup for every
> plan node, but maybe it wouldn't really matter in practice.

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't good enough.
(2) The IDs aren't free either; where will an extension keep the
ID it assigned?  We're trying to move away from global variables.

But, if you're convinced otherwise, the current design is OK.

Other than that point, I think 0001 and 0002 are ready.

> Regarding commit timeframe, you expressed a preference for 0001 and
> 0002 to be committed "soon," but I'm going to be leaving town for a
> week on Saturday and would prefer not to put myself in a situation
> where somebody is expecting me to fix the buildfarm while I am gone.
> So I'm happy to commit them today or tomorrow if you don't mind being
> responsible for anything that blows up while I'm gone, or I'm happy to
> have you commit them whenever you want, but barring one of those
> outcomes, I'm going to deal with it on or after March 17th.

Sure, I will backstop you if you want to push and run.  There's
at least one nearby thread where some interest has been expressed in
using this (the postgres_fdw include-the-remote-EXPLAIN business).
So I think there is value in having it in the tree sooner.

            regards, tom lane

On Thu, Mar 6, 2025 at 4:16 PM Tom Lane <tgl@sss.pgh.pa.us> 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't good enough.
> (2) The IDs aren't free either; where will an extension keep the
> ID it assigned?  We're trying to move away from global variables.
>
> But, if you're convinced otherwise, the current design is OK.

Interesting. I hadn't even considered just iterating every time to
find the ID, but I agree with you that might be totally fine. As you
say, we're not expecting there to be many extensions here. I can try
coding that up and see how it looks (or you can, if you like).

I don't buy your second point, though. Our globals are probably going
to turn into thread-locals at some point, and we may want to do things
like bundle a bunch of related ones together in a struct, but I don't
see any real hope of getting rid of them altogether, and if we do, the
one integer you need to power this kind of extension will hardly be
the biggest problem.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 06.03.25 21:23, Robert Haas wrote:
> On Wed, Mar 5, 2025 at 4:38 PM Tom Lane <tgl@sss.pgh.pa.us> 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 now.
> 
> Ugh, I suck at this, sorry. Adjusted in v5. It's hard to avoid the
> conclusion that our IWYU configuration must be fairly lenient, because
> every change seems to surface more source files that are depending on
> indirect includes.

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 unneeded.  Indirect includes cleanup is a different 
project that is not currently happening, AFAIK.

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 about 
accidental C11 use.  We could probably have a discussion about that, but 
for this patch set, it's probably better to just deal with the status quo.

On Thu, Mar 6, 2025 at 4:24 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Mar 6, 2025 at 4:16 PM Tom Lane <tgl@sss.pgh.pa.us> 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't good enough.
> > (2) The IDs aren't free either; where will an extension keep the
> > ID it assigned?  We're trying to move away from global variables.
> >
> > But, if you're convinced otherwise, the current design is OK.
>
> Interesting. I hadn't even considered just iterating every time to
> find the ID, but I agree with you that might be totally fine. As you
> say, we're not expecting there to be many extensions here. I can try
> coding that up and see how it looks (or you can, if you like).

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(). With this approach, the backend-scope
IDs still exist, but they are private to explain_state.c. An alternate
design could be to make each individual ExplainState have its own list
of extension names alongside its own list of opaque pointers, so that
the IDs become ExplainState-scoped rather than backend-scoped. At the
moment, that seems to me to be just deciding to make the code more
complicated for no obvious benefit, but maybe I'm missing something.
At any rate, my overall conclusion here is that this is giving up a
probably-insignificant amount of performance for an
also-not-terribly-significant abstraction improvement, so I find it a
little hard to get excited about it one way or the other, but it's
fine.

Tom, what do you think?

Thanks,

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Mar 7, 2025 at 9:38 AM Peter Eisentraut <peter@eisentraut.org> 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 unneeded.  Indirect includes cleanup is a different
> project that is not currently happening, AFAIK.

OK, thanks. I wonder whether that's a good use of effort or just not
worth worrying about.

> 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 about
> accidental C11 use.  We could probably have a discussion about that, but
> for this patch set, it's probably better to just deal with the status quo.

Agreed. +1 for having a discussion at some point, though, because the
effect of the current rules seems to be that you have to write "struct
BananaSplit *" in a bunch of places instead of just 'BananaSplit *" to
avoid redefining the typedef. That's worth doing it if it solves a
real problem, but if compilers where it is a real problem are extinct
in the wild, then I think I would prefer not to have to add the
"struct" keyword in a bunch of places just for compliance with
historical compiler behavior.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Mar 7, 2025 at 9:38 AM Peter Eisentraut <peter@eisentraut.org> 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 about
>> accidental C11 use.  We could probably have a discussion about that, but
>> for this patch set, it's probably better to just deal with the status quo.

> Agreed. +1 for having a discussion at some point, though, because the
> effect of the current rules seems to be that you have to write "struct
> BananaSplit *" in a bunch of places instead of just 'BananaSplit *" to
> avoid redefining the typedef.

I'd be +1 if there's a way to allow that particular thing without
thereby opening the floodgates to every other C11 feature.  I expect
not all of C11 is universal yet, so I think the buildfarm animals
that are using -std=gnu99 are mostly doing us a service.  But yeah,
this particular thing is a pain in the rear.

            regards, tom lane

Robert Haas <robertmhaas@gmail.com> 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 s/offests/offsets/ and s/direcrtly/directly/.
Other than that, I think 0001 and 0002 are good to go.
I've still not looked at 0003.

            regards, tom lane

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 sounds like a good idea.. FWIW, There is a
>> discussion [0]
>> for showing FDW remote plans ( postgres_fdw specifically), and I think
>> we  will need to
>> add some new options to EXPLAIN to make that possible.

> Have not looked at your patches, but I will do so now.

Over the past few days I have had a chance to experiment with these
patches and good news is that it has allowed me to extend EXPLAIN
for postrges_fdw to show remote plans. I will share the update and
patch for this in [0], but thought it will be good to share here as well.

postgres=# explain (remote_plans) select * from t_r1, t1_r1;
                                 QUERY PLAN
----------------------------------------------------------------------------
 Nested Loop  (cost=200.00..83272.80 rows=6553600 width=16)
   Plan Node ID: 0
   ->  Foreign Scan on t_r1  (cost=100.00..673.20 rows=2560 width=8)
         Plan Node ID: 1
   ->  Materialize  (cost=100.00..686.00 rows=2560 width=8)
         Plan Node ID: 2
         ->  Foreign Scan on t1_r1  (cost=100.00..673.20 rows=2560 width=8)
               Plan Node ID: 3
 Remote Plans:
       Seq Scan on t  (cost=0.00..32.60 rows=2260 width=8)
       Statement Name: Plan Node ID = 1

       Seq Scan on t1  (cost=0.00..32.60 rows=2260 width=8)
       Statement Name: Plan Node ID = 3
(14 rows)

I do have some findings/suggestions:

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..673.20 rows=2560 width=8) (node_id=1)
   ->  Materialize  (cost=100.00..686.00 rows=2560 width=8) (node_id=2)
         ->  Foreign Scan on t1_r1  (cost=100.00..673.20 rows=2560
width=8) (node_id=3)

Can we add a hook at that point [1] which will allow an extension to modify
the first line of a node? I think this is not just useful for my case, but also
for other use-cases in which some high level node details could be placed.
what do you think?

2/ I registered an options handler, and I wanted this options handler to
validate that my new extension option is not used with the analyze
option.

So, the behavior is if the core explain option was first in the list, it worked,
but if it was first in the list it does not.

postgres=# explain (analyze, remote_plans) select from t_r1, t1_r1;
ERROR:  EXPLAIN options REMOTE_PLANS and ANALYZE cannot be used together

postgres=# explain (remote_plans, analyze) select from t_r1, t1_r1;
                                                       QUERY PLAN

-------------------------------------------------------------------------------------------------------------------------
 Nested Loop  (cost=200.00..147337.37 rows=11648569 width=0) (actual
time=3.222..3.236 rows=0.00 loops=1)
 ...
....

This is because the ApplyExtensionExplainOption is called inside the main
loop that parses the options, so when ApplyExtensionExplainOption, the core
option may or may not have been set yet. I also think this will break if there
are multiple extension options that need to be validated together.

One way I thought to fix this is to allow the user to register another handler
for validation, which can then be called after the parse option loop, and after
all the in-core options have been validated against each other. Right after
this line.

+    /* if the summary was not set explicitly, set default value */
+    es->summary = (summary_set) ? es->summary : es->analyze;

What do you think?


[0]
https://www.postgresql.org/message-id/flat/CAP%2BB4TD%3Diy-C2EnsrJgjpwSc7_4pd3Xh-gFzA0bwsw3q8u860g%40mail.gmail.com
[1] https://github.com/postgres/postgres/blob/master/src/backend/commands/explain.c#L2013

Thanks

--
Sami Imseih
Amazon Web Services (AWS)

Sami Imseih <samimseih@gmail.com> 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..673.20 rows=2560 width=8) (node_id=1)
>    ->  Materialize  (cost=100.00..686.00 rows=2560 width=8) (node_id=2)
>          ->  Foreign Scan on t1_r1  (cost=100.00..673.20 rows=2560
> width=8) (node_id=3)

> Can we add a hook at that point [1] which will allow an extension to modify
> the first line of a node? I think this is not just useful for my case, but also
> for other use-cases in which some high level node details could be placed.
> what do you think?

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.  Plus, this doesn't consider what to do in non-text
output formats.  ISTM you should be good with adding a new
"Plan Node ID" property to each node.  No, you don't get to put it
first.  Too bad.  (If we did try to cater to that, what shall we do
with multiple extensions that all think they should be first?)

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.

            regards, tom lane

> 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
desirable behavior. I get that also.

I have no strong feelings for this, but wanted to see what
others think.

thanks!

--
Sami Imseih

> 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 felt the best way is to create another handler for registering a validation
function. This means we have to loop through the options list twice,
but I don't think that is a problem.

postgres=# explain (remote_plans, analyze) select * from t_r1;
ERROR:  EXPLAIN options REMOTE_PLANS and ANALYZE cannot be used together
postgres=# explain (analyze, remote_plans) select * from t_r1;
ERROR:  EXPLAIN options REMOTE_PLANS and ANALYZE cannot be used together

Regards,

Sami

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 non-text formats, which is not
> desirable behavior. I get that also.
> 
> I have no strong feelings for this, but wanted to see what
> others think.
I'm against it. For me, the best model there is to allow extensions to 
add something and nothing more. If it wants to change the core explain 
code - use ExplainOneQuery_hook instead.
The reason here is to reduce possible competition among extensions.

I already have troubles with conflict on queryid modifications and 
potential conflict in the planner_hook - if someone invents another 
extension that will provide a plan tree. So, it would be better to 
reduce conflicts whenever possible.

-- 
regards, Andrei Lepikhov

On Thu, Mar 13, 2025 at 5:52 PM Sami Imseih <samimseih@gmail.com> 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 only as Robert may have a different opinion on how to fix
> this.
>
> I felt the best way is to create another handler for registering a validation
> function. This means we have to loop through the options list twice,
> but I don't think that is a problem.

Hmm. I thought about this at some stage in the development of these
patches, but nothing made it into the final version. I agree we should
probably do something about it, but I don't really like the idea of
calling the second hook "validate" as you've done here, because most
validation can and should be done in the regular apply hook. You only
need the second hook if you want to check the values of options
against the values of other options.

I wonder if we should just add a "plain" hook to the bottom of
ParseExplainOptionList, like post_parse_explain_options_list_hook or
something, and then EXPLAIN options that don't need this kind of
validation can just do nothing and those that do can do the usual
dance to add themselves to the hook. We could also keep it as you have
it here, with an extra handler that is called per option, but what if
some loadable module adds two new options LEFT and RIGHT and wants to
check that you don't specify LEFT and RIGHT together? Either they
register the same validate handler for both, or they register the real
validate handler for one and a no-op handler for the other. Neither of
those options seems very appealing.

--
Robert Haas
EDB: http://www.enterprisedb.com

> 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 extension author can just put all their checks in one
place rather than having to register a bunch of handlers.

> some loadable module adds two new options LEFT and RIGHT and wants to
> check that you don't specify LEFT and RIGHT together? Either they
> register the same validate handler for both, or they register the real
> validate handler for one and a no-op handler for the other. Neither of
> those options seems very appealing.

When I thought about this, I figured that one of the options will register
a validate handler and the other option will set the handler to NULL. But,
I do see why this is not appealing.

--
Sami

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 needed to pass 
data from the best_path chosen to the plan and needed to compare 
prediction and reality at the end of execution.

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.
2. In my patch I inserted the hook before the line 1894:
/* in text format, the first line ends here */
Why have you chosen a different way? I don't have specific reasons to 
insist, except the extension data right under the node looks better to 
me personally.

Some changes of the explain format have already been made in the current 
master. Applying this proposal now would reduce additional work for 
extension (and fork) maintainers in the next version.

-- 
regards, Andrei Lepikhov

On Mon, Mar 17, 2025 at 11:54 PM Sami Imseih <samimseih@gmail.com> 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
> place rather than having to register a bunch of handlers.

Do you want to propose a patch?

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Mar 18, 2025 at 8:02 AM Andrei Lepikhov <lepihov@gmail.com> 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, because the hooks will be called from explain.c, not explain_state.c.

> 2. In my patch I inserted the hook before the line 1894:
> /* in text format, the first line ends here */
> Why have you chosen a different way? I don't have specific reasons to
> insist, except the extension data right under the node looks better to
> me personally.

Tom discusses why we shouldn't try to add to the first line in
http://postgr.es/m/2234935.1741809008@sss.pgh.pa.us and I'm fully in
agreement.

I've committed 0001 and 0002 for now. The additional hook for
cross-option validation can be added in a separate commit. v6-0003,
now v7-0001, needs more substantive review before commit. I hope it
gets some, and soon.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 3/18/25 14:33, Robert Haas wrote:
> On Tue, Mar 18, 2025 at 8:02 AM Andrei Lepikhov <lepihov@gmail.com> 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, because the hooks will be called from explain.c, not explain_state.c.
Ok.
> 
>> 2. In my patch I inserted the hook before the line 1894:
>> /* in text format, the first line ends here */
>> Why have you chosen a different way? I don't have specific reasons to
>> insist, except the extension data right under the node looks better to
>> me personally.
> 
> Tom discusses why we shouldn't try to add to the first line in
> http://postgr.es/m/2234935.1741809008@sss.pgh.pa.us and I'm fully in
> agreement.
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, I should say) sometimes makes it difficult 
to read.

> 
> I've committed 0001 and 0002 for now. The additional hook for
> cross-option validation can be added in a separate commit. v6-0003,
> now v7-0001, needs more substantive review before commit. I hope it
> gets some, and soon.
Ok, I am ready to review it thoroughly, if needed.

-- 
regards, Andrei Lepikhov

On Tue, Mar 18, 2025 at 9:58 AM Andrei Lepikhov <lepihov@gmail.com> 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, I should say) sometimes makes it difficult
> to read.

OK, I wasn't sure if you meant just before or just after emitting the
end-of-first-line newline.

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
bikeshed this endlessly, but there's no single choice that's going to
make everybody 100% happy, and adding a whole bunch of extra hooks to
cater to various preferences about exactly how the output should look
does not seem worth it to me.

> > I've committed 0001 and 0002 for now. The additional hook for
> > cross-option validation can be added in a separate commit. v6-0003,
> > now v7-0001, needs more substantive review before commit. I hope it
> > gets some, and soon.
> Ok, I am ready to review it thoroughly, if needed.

Yeah, I don't know if Tom is going to jump in here, but if we want to
get something into this release we don't have much time. I personally
think this is good enough that it would be better to have it than
nothing and we can always change stuff later. I'm not going to be too
sympathetic if somebody complains about a backward compatibility break
for pg_overexplain; it's labelled as a developer tool that shows
information about internals. That said, I don't want to ship something
and then have everybody say "what is this trash?".

--
Robert Haas
EDB: http://www.enterprisedb.com

> Do you want to propose a patch?

yes, will attach a patch shortly.

--
Sami

Robert Haas <robertmhaas@gmail.com> 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
> bikeshed this endlessly, but there's no single choice that's going to
> make everybody 100% happy, and adding a whole bunch of extra hooks to
> cater to various preferences about exactly how the output should look
> does not seem worth it to me.

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.

There's certainly room for bikeshedding here, but I think it'd be good
to get some actual experience before redesigning what you've done.

            regards, tom lane

On Tue, Mar 18, 2025 at 2:40 PM Tom Lane <tgl@sss.pgh.pa.us> 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.

Yep.

> There's certainly room for bikeshedding here, but I think it'd be good
> to get some actual experience before redesigning what you've done.

I think so, too, although obviously I'm biased.

--
Robert Haas
EDB: http://www.enterprisedb.com

> > 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
validation fails.

--
Sami Imseih
Amazon Web Services (AWS)

On Tue, Mar 18, 2025 at 11:21 PM Sami Imseih <samimseih@gmail.com> 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.
> It is expected the extension will raise an error if the
> validation fails.

Since the new hook will be called from explain_state.c, please declare
it in explain_state.h.

I think the header-file comment should be just one line rather than a
block comment as we do for other hooks in this area. If you think a
more detailed explanation is needed, I'd put that in the .c file.

Is there any value in passing "options" or "pstate" to the hook?
Couldn't the hook want to use "pstate" for error reporting purposes?

--
Robert Haas
EDB: http://www.enterprisedb.com

> On Tue, Mar 18, 2025 at 11:21 PM Sami Imseih <samimseih@gmail.com> 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.
> > It is expected the extension will raise an error if the
> > validation fails.
>
> Since the new hook will be called from explain_state.c, please declare
> it in explain_state.h.

done

> I think the header-file comment should be just one line rather than a
> block comment as we do for other hooks in this area. If you think a
> more detailed explanation is needed, I'd put that in the .c file.

on second thought, no reason for the detailed explanation...

> Is there any value in passing "options" or "pstate" to the hook?
> Couldn't the hook want to use "pstate" for error reporting purposes?

... as I made the hook signature match that of
ParseExplainOptionList, so both pstate and the options list
are now available to the hook.

--

Sami Imseih
Amazon Web Services (AWS)

On Wed, Mar 19, 2025 at 11:38 AM Sami Imseih <samimseih@gmail.com> 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 Wed, Mar 19, 2025 at 11:38 AM Sami Imseih <samimseih@gmail.com> 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 modified files and it found one
indentation correction from v2.

> index 1d4be3c18ac..8fe1ca5c73e 100644
38c38
< +        (*explain_validate_options_hook) (es, options, pstate);
---
> +        (*explain_validate_options_hook)(es, options, pstate);
59c59
< 2.47.1

v3 attached.


--
Sami Imseih
Amazon Web Services (AWS)

On Wed, Mar 19, 2025 at 1:41 PM Sami Imseih <samimseih@gmail.com> 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.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 19/3/2025 18:41, Sami Imseih wrote:
>> On Wed, Mar 19, 2025 at 11:38 AM Sami Imseih <samimseih@gmail.com> 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 modified files and it found one
> indentation correction from v2.
> 
>> index 1d4be3c18ac..8fe1ca5c73e 100644
> 38c38
> < +        (*explain_validate_options_hook) (es, options, pstate);
> ---
>> +        (*explain_validate_options_hook)(es, options, pstate);
> 59c59
> < 2.47.1
> 
> v3 attached.
I want to criticize this patch a bit. ;) Why do you think this hook is 
not redundant? May we avoid it at all? It seems to perform a too-narrow 
task.
For example, if you want to separate parameters that may be used on a 
plain EXPLAIN and on EXPLAIN ANALYZE to get behaviour like the following:

EXPLAIN (BUFFERS ON, TIMING ON) SELECT * FROM pg_class;
ERROR: EXPLAIN option TIMING requires ANALYZE

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.

 > ERROR:  EXPLAIN options REMOTE_PLANS and ANALYZE cannot be used
 > together
I think the additional parameter covers the case you provided, isn't it?

-- 
regards, Andrei Lepikhov

> 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 by the core inside the ParseExplainOptionList.

hmm, IIUC, what you are describing is flag that will be limited to
only check if an option can be used with EXPLAIN_ONLY, ANALYZE_ONLY
or both. But what about if I have a case to check against between other
extension options? let's say ExtensionAOptionA and ExtensionAoptionB.
How would that work with the way you are suggesting?

--
Sami Imseih
Amazon Web Services (AWS)

Sami Imseih <samimseih@gmail.com> 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.

> hmm, IIUC, what you are describing is flag that will be limited to
> only check if an option can be used with EXPLAIN_ONLY, ANALYZE_ONLY
> or both. But what about if I have a case to check against between other
> extension options? let's say ExtensionAOptionA and ExtensionAoptionB.
> How would that work with the way you are suggesting?

More generally, I think that's quite a constricted view of what sorts
of checks an extension might need to make.  As an example, suppose
an extension's option conflicts with "GENERIC_PLAN on" for some reason
(perhaps it wants to modify the plan).

I think the current hook proposal is fine.  It leaves all the
validation work to be done by the extension(s), sure, but the amount
of code required to handle that one case is tiny, and the door is
left open to handle cases we didn't foresee.

            regards, tom lane

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 ExplainExtensionOption structure and
>> processed by the core inside the ParseExplainOptionList.
> 
> hmm, IIUC, what you are describing is flag that will be limited to
> only check if an option can be used with EXPLAIN_ONLY, ANALYZE_ONLY
> or both. But what about if I have a case to check against between other
> extension options? let's say ExtensionAOptionA and ExtensionAoptionB.
> How would that work with the way you are suggesting?
That makes sense. It would be more effective to include the meaningful 
example in Robert's extension for v7-0001.

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 could simply be resolved at the end of 
execution within a per-node hook with tiny inconsistency in output. I'm 
pleased to see a change in the narrative.

-- 
regards, Andrei Lepikhov

>
> 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 ExplainExtensionOption structure and
> >> processed by the core inside the ParseExplainOptionList.
> >
> > hmm, IIUC, what you are describing is flag that will be limited to
> > only check if an option can be used with EXPLAIN_ONLY, ANALYZE_ONLY
> > or both. But what about if I have a case to check against between other
> > extension options? let's say ExtensionAOptionA and ExtensionAoptionB.
> > How would that work with the way you are suggesting?
> That makes sense. It would be more effective to include the meaningful
> example in Robert's extension for v7-0001.

While it will be good to provide an example in v7-0001, I can't see
any of the pg_overexplain options needing such validation.

> 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 could simply be resolved at the end of
> execution within a per-node hook with tiny inconsistency in output. I'm
> pleased to see a change in the narrative.

I think you are referring to the idea to put a hook before
/* in text format, the first line ends here */

After considering the comments, I do see that was not a good proposal. You are
correct, and the per-node hook is enough.


--
Sami Imseih
Amazon Web Services (AWS)

On Thu, Mar 20, 2025 at 3:04 AM Andrei Lepikhov <lepihov@gmail.com> 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 could simply be resolved at the end of
> execution within a per-node hook with tiny inconsistency in output. I'm
> pleased to see a change in the narrative.

It sounds like we're sufficiently in agreement so I've committed the
patch. I agree with you that this could be done within the per-node
hook, but that would be awkward, and this is better. I take note of
your frustration with the difficulty of getting hooks added and to
some extent I share it; on the flip side, there are plenty of
poorly-considered hook proposals, too.

I've rebased my pg_overexplain patch and attach that here.

--
Robert Haas
EDB: http://www.enterprisedb.com

> 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 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 reason to do so with
pg_overexplain.

2/ I agree that this is better than debug_print_plan to avoid all the
verbosity of that output emitting at once. But maybe it will be useful
to provide options to show other parts like the target list, quals, etc.
I wonder if something like this will be better?

explain (query_tree_show range_table) select ..
explain (query_tree_show qualification) select ..
explain (query_tree_show target_list) select ..

The user can choose which information they want to see by passing
a value to an option called query_tree_show ( or a better option name)

Initially, we can only support range_table, but future enhancements
could include other parts.

Maybe a good idea, maybe not, what do you think?


Besides that, I found a few minor things:


1/ trailing whitespace

+     <literal>Subplans Needing Rewind</literal>. Integer IDs of subplans that


2/ typos
 * We don't try to print everything here. Information that would be displyed

 * to limit the display length by # of columsn or # of characters, but for



[0] https://www.postgresql.org/message-id/8e8b6f17-e1e6-4b16-84d4-37ded802c787%40gmail.com


--

Sami Imseih
Amazon Web Services (AWS)

On Thu, Mar 20, 2025 at 8:37 PM Sami Imseih <samimseih@gmail.com> 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 reason to do so with
> pg_overexplain.

Yeah, there may be a good idea lurking somewhere here but I have not
yet come up with it.

> 2/ I agree that this is better than debug_print_plan to avoid all the
> verbosity of that output emitting at once. But maybe it will be useful
> to provide options to show other parts like the target list, quals, etc.
> I wonder if something like this will be better?
>
> 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 separate
options are better.

Also, the reason I didn't do the quals or the target list is because
regular EXPLAIN already shows that stuff. There could possibly be some
value in trying to show things about those node trees that regular
EXPLAIN doesn't, but that quickly gets into (a) speculation about what
people actually want to see and/or (b) extremely voluminous output
which is the whole reason debug_print_plan is hard to use in the first
place.

> 1/ trailing whitespace
> 2/ typos

Acknowledged, thanks.

--
Robert Haas
EDB: http://www.enterprisedb.com

> > 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 separate
> options are better.

Yeah, that's a valid point. I guess I was thinking that if we ever
decide to have
an option for every part of the query tree, we will end up with many options
(a half dozen options or so), but not being able to support
"give me these 2 parts only" is not ideal either.

> EXPLAIN doesn't, but that quickly gets into (a) speculation about what
> people actually want to see and/or

Correct. This is why these hooks are useful and people can just
write whatever is useful to them, or if there is a convincing reason,
it can be added into pg_overexplain overtime.

overall this LGTM, and I don't really have other comments.

--
Sami Imseih
Amazon Web Services (AWS)

Sami Imseih <samimseih@gmail.com> 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_range_table_handler(ExplainState *, DefElem *,
+                                            ParseState *);

I cordially hate this parameter-name-less style of function
declaration, and consider it Stroustrup's single worst idea in C++.
It rests on the assumption that parameter names convey zero
information, which is wrongheaded for any function more complex than,
say, addition.  Also, even if you like this style, clang-tidy probably
won't (cf for example 035ce1feb).

pgoverexplain.sgml seems to have been formatted to fit in about an
85-column window.  Please don't do that --- you might as well have
made it 99 columns wide or any other random number, it still looks
like heck in 80 columns.

Other than that, there's room to debate exactly what to show.
But as long as we're agreed that we won't hold this module to
high cross-version compatibility standards, that doesn't seem
like a problem.  I'm okay with this as a starting point.

            regards, tom lane

On Fri, Mar 21, 2025 at 2:37 PM Tom Lane <tgl@sss.pgh.pa.us> 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_handler(ExplainState *, DefElem *,
> +                                            ParseState *);
>
> I cordially hate this parameter-name-less style of function
> declaration, and consider it Stroustrup's single worst idea in C++.
> It rests on the assumption that parameter names convey zero
> information, which is wrongheaded for any function more complex than,
> say, addition.  Also, even if you like this style, clang-tidy probably
> won't (cf for example 035ce1feb).

I don't hate the style but I didn't use it intentionally. Fixed now, I hope.

> pgoverexplain.sgml seems to have been formatted to fit in about an
> 85-column window.  Please don't do that --- you might as well have
> made it 99 columns wide or any other random number, it still looks
> like heck in 80 columns.

Oops. Fixed. Also, I tried to disclaim stability and comprehensibility
a little better and fixed a dumb mistake in the page title.

> Other than that, there's room to debate exactly what to show.
> But as long as we're agreed that we won't hold this module to
> high cross-version compatibility standards, that doesn't seem
> like a problem.  I'm okay with this as a starting point.

Great news, thanks again!

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.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 3/21/25 20:54, Robert Haas wrote:
> On Fri, Mar 21, 2025 at 2:37 PM Tom Lane <tgl@sss.pgh.pa.us> 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 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. Having a strong necessity 
for extra explain tools (in the daily routine, all I have is the only 
single explain analyse verbose output to find out planner/executor bug, 
reproduce it and make a patch), I don't see a single case when I would 
use this module. It adds a burden to fix its output on a node change 
(you don't care, but it adds work to Postgres fork maintainers, too, for 
nothing). Also, it breaks my understanding of the principles of the 
Postgres code design - to start the discussion on how to show more, we 
need only the bare minimum of code and output lines.
In my opinion, it should show as few parameters as possible to 
demonstrate principles and test the code on a single planner node. It 
only deserves src/test/modules because it is not helpful for a broad 
audience.

-- 
regards, Andrei Lepikhov

On Sat, Mar 22, 2025 at 4:46 AM Andrei Lepikhov <lepihov@gmail.com> 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. Having a strong necessity
> for extra explain tools (in the daily routine, all I have is the only
> single explain analyse verbose output to find out planner/executor bug,
> reproduce it and make a patch), I don't see a single case when I would
> use this module. It adds a burden to fix its output on a node change
> (you don't care, but it adds work to Postgres fork maintainers, too, for
> nothing). Also, it breaks my understanding of the principles of the
> Postgres code design - to start the discussion on how to show more, we
> need only the bare minimum of code and output lines.
> In my opinion, it should show as few parameters as possible to
> demonstrate principles and test the code on a single planner node. It
> only deserves src/test/modules because it is not helpful for a broad
> audience.

Gee, thanks for the ringing endorsement. :-)

I think *I* will use it pretty regularly; I already have. In my
experience, using debug_query_plan for this sort of thing sucks quite
a lot. Finding the information you need in the output takes a long
time because the output is quite long. This is much more
understandable, at least for me. I agree with you that a trivial test
module could demonstrate that the hook works, but a trivial example
would not demonstrate that the hook can be used to do something
actually useful. It sounds like what I've written also fails the
"actually useful" test for you, but it doesn't for me. 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.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Sat, Mar 22, 2025 at 12:10 PM Robert Haas <robertmhaas@gmail.com> 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 buildfarm, because I just noticed
that the changes I made in v9 seem not to have resolved the problem
with debug_parallel_query, for reasons I do not yet understand.
Investigating...

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Mar 26, 2025 at 2:09 PM Robert Haas <robertmhaas@gmail.com> 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 reasons I do not yet understand.
> Investigating...

I pushed a couple of fixes for this, but I still see one failure, on drongo:

diff --strip-trailing-cr -U3
C:/prog/bf/root/HEAD/pgsql/contrib/pg_overexplain/expected/pg_overexplain.out
C:/prog/bf/root/HEAD/pgsql.build/testrun/pg_overexplain/regress/results/pg_overexplain.out
--- C:/prog/bf/root/HEAD/pgsql/contrib/pg_overexplain/expected/pg_overexplain.out
2025-03-26 20:33:47.834896600 +0000
+++ C:/prog/bf/root/HEAD/pgsql.build/testrun/pg_overexplain/regress/results/pg_overexplain.out
2025-03-26 21:57:55.332431300 +0000
@@ -419,7 +419,7 @@
                Plan Node ID: 5
                extParam: 0
                allParam: 0
-         ->  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)
                Index Cond: (id = v1.id)
                Index Searches: 8
                Disabled Nodes: 0

As some of you will recall, I hate floating point arithmetic with a
fiery passion, but I'm still surprised to learn that 1/8 rounded to
two decimal places does not produce the same answer on all modern
systems. It's not even like this is some weird architecture -- it's
listed as x86_64. I'll look into a fix for this in the morning.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Mar 26, 2025 at 02:09:32PM -0400, Robert Haas wrote:
> On Sat, Mar 22, 2025 at 12:10 PM Robert Haas <robertmhaas@gmail.com> 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 buildfarm, because I just noticed
> that the changes I made in v9 seem not to have resolved the problem
> with debug_parallel_query, for reasons I do not yet understand.
> Investigating...

There's at least one buildfarm member complaining with the tests of
pg_overexplain, not for the reason you are mentioning here:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2025-03-26%2020%3A33%3A20
--
Michael

Robert Haas <robertmhaas@gmail.com> 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 somebody.  We have a very strong rule
that you *do not* make regression test results dependent on exact
computed cost or rowcount values, because those are inherently
platform-dependent.  I do not understand how you thought that
pg_overexplain might be exempt from that problem.

            regards, tom lane

On Wed, Mar 26, 2025 at 10:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> 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 somebody. We have a very strong rule
> that you *do not* make regression test results dependent on exact
> computed cost or rowcount values, because those are inherently
> platform-dependent.  I do not understand how you thought that
> pg_overexplain might be exempt from that problem.

Is this rule written down someplace?

--
Robert Haas
EDB: http://www.enterprisedb.com

On 3/26/25 19:09, Robert Haas wrote:
> On Sat, Mar 22, 2025 at 12:10 PM Robert Haas <robertmhaas@gmail.com> 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 buildfarm, because I just noticed
> that the changes I made in v9 seem not to have resolved the problem
> with debug_parallel_query, for reasons I do not yet understand.
> Investigating...
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?

-- 
regards, Andrei Lepikhov

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
 auto_explain | 18devel | auto_explain.so
 plpgsql      | 18devel | plpgsql.so
(3 rows)

Some minor changes may be needed.

// PG_MODULE_MAGIC;
PG_MODULE_MAGIC_EXT(
                    .name = "pg_overexplain",
                    .version = PG_VERSION
);

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 preload it into some or all sessions
by including pg_overexplain in session_preload_libraries
or shared_preload_libraries in postgresql.conf. 

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

On Thu, Mar 27, 2025 at 5:34 PM Andrei Lepikhov <lepihov@gmail.com> 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 part. Thanks, committed.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Mar 28, 2025 at 12:13 AM Man Zeng <zengman@halodbtech.com> wrote:
> // PG_MODULE_MAGIC;
> PG_MODULE_MAGIC_EXT(
>                                         .name = "pg_overexplain",
>                                         .version = PG_VERSION
> );

Good catch, committed.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Mar 28, 2025 at 5:39 AM Pavel Luzanov <p.luzanov@postgrespro.ru> 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_overexplain';
>
> (You must be superuser to do that.)
> Another way is to preload it into some or all sessions
> by including pg_overexplain in session_preload_libraries
> or shared_preload_libraries in postgresql.conf.

Yeah, I think this could make sense, but I'm not sure how much detail
to include. pg_overexplain is a little unusual in being one of few
contrib modules where all you have to do is load it -- there's no
CREATE EXTENSION required, and shared_preload_libraries is not
required either. So, it probably makes sense to explain something, but
if we eventually have 10 more modules that work like this, we wouldn't
want a lengthy explanation in each one.

Another thing that we might want to do is adjust the documentation for
EXPLAIN. At the very least, the "See also" section should probably
reference pg_overexplain, but we might also want some text there
explaining the general concept that EXPLAIN is now extensible via
loadable modules.

--
Robert Haas
EDB: http://www.enterprisedb.com