Horton

Thread

Commits

  1. Improve support for ExplainOneQuery() hook

  1. Hooking into ExplainOneQuery() complicated by missing standard_ExplainOneQuery

    Mats Kindahl <mats@timescale.com> — 2024-03-04T11:59:46Z

    Hi hackers,

I wanted to hook into the EXPLAIN output for queries and add some extra
information, but since there is no standard_ExplainOneQuery() I had to copy
the code and create my own version.

Since the pattern with other hooks for a function WhateverFunction() seems
to be that there is a standard_WhateverFunction() for each
WhateverFunction_hook, I created a patch to follow this pattern for your
consideration.

I was also considering adding a callback so that you can annotate any node
with explanatory information that is not a custom scan node. This could be
used to propagate and summarize information from custom scan nodes, but I
had no immediate use for that so did not add it here. I would still be
interested in hearing if you think this is something that would be useful
to the community.

Best wishes,
Mats Kindahl, Timescale

  2. Re: Hooking into ExplainOneQuery() complicated by missing standard_ExplainOneQuery

    Aleksander Alekseev <aleksander@timescale.com> — 2024-03-04T12:41:16Z

    Hi,

> I wanted to hook into the EXPLAIN output for queries and add some extra information, but since there is no standard_ExplainOneQuery() I had to copy the code and create my own version.
>
> Since the pattern with other hooks for a function WhateverFunction() seems to be that there is a standard_WhateverFunction() for each WhateverFunction_hook, I created a patch to follow this pattern for your consideration.
>
> I was also considering adding a callback so that you can annotate any node with explanatory information that is not a custom scan node. This could be used to propagate and summarize information from custom scan nodes, but I had no immediate use for that so did not add it here. I would still be interested in hearing if you think this is something that would be useful to the community.

Thanks for the patch. LGTM.

I registered the patch on the nearest open CF [1] and marked it as
RfC. It is a pretty straightforward refactoring.

[1]: https://commitfest.postgresql.org/48/4879/

-- 
Best regards,
Aleksander Alekseev

  3. Re: Hooking into ExplainOneQuery() complicated by missing standard_ExplainOneQuery

    Michael Paquier <michael@paquier.xyz> — 2024-03-05T06:31:25Z

    On Mon, Mar 04, 2024 at 03:41:16PM +0300, Aleksander Alekseev wrote:
>> I wanted to hook into the EXPLAIN output for queries and add some
>> extra information, but since there is no standard_ExplainOneQuery() I
>> had to copy the code and create my own version. 
>>
>> Since the pattern with other hooks for a function
>> WhateverFunction() seems to be that there is a
>> standard_WhateverFunction() for each WhateverFunction_hook, I
>> created a patch to follow this pattern for your consideration.

So you've wanted to be able to add some custom information at the end
or the beginning of ExplainState's output buffer, before falling back
to the in-core path.  What was the use case, if I may ask?

>> I was also considering adding a callback so that you can annotate
>> any node with explanatory information that is not a custom scan
>> node. This could be used to propagate and summarize information
>> from custom scan nodes, but I had no immediate use for that so did
>> not add it here. I would still be interested in hearing if you
>> think this is something that would be useful to the community.

That depends.

> I registered the patch on the nearest open CF [1] and marked it as
> RfC. It is a pretty straightforward refactoring.
> 
> [1]: https://commitfest.postgresql.org/48/4879/

I know that we're in the middle of commit fest 47 while this is in 48,
but I can't really see a reason why we should not do that earlier than
v18.  One point about core is to be flexible for extension code.  So I\
have no objections, others are free to comment.
--
Michael

  4. Re: Hooking into ExplainOneQuery() complicated by missing standard_ExplainOneQuery

    Mats Kindahl <mats@timescale.com> — 2024-03-05T07:21:34Z

    On Tue, Mar 5, 2024 at 7:31 AM Michael Paquier <michael@paquier.xyz> wrote:

> On Mon, Mar 04, 2024 at 03:41:16PM +0300, Aleksander Alekseev wrote:
> >> I wanted to hook into the EXPLAIN output for queries and add some
> >> extra information, but since there is no standard_ExplainOneQuery() I
> >> had to copy the code and create my own version.
> >>
> >> Since the pattern with other hooks for a function
> >> WhateverFunction() seems to be that there is a
> >> standard_WhateverFunction() for each WhateverFunction_hook, I
> >> created a patch to follow this pattern for your consideration.
>
> So you've wanted to be able to add some custom information at the end
> or the beginning of ExplainState's output buffer, before falling back
> to the in-core path.  What was the use case, if I may ask?
>

Yes, that was the use-case. We have some caches added by extending
ExecutorStart and other executor-related functions using the hooks there
and want to show cache hits and misses in the plan.

I realize that a more advanced system is possible to create where you can
customize the output even more, but in this case I just wanted to add a
section with some additional information related to plan execution. Also,
the code in explain.c seems to not be written with extensibility in mind,
so I did not want to make too big a change here before thinking through how
this would work.


> >> I was also considering adding a callback so that you can annotate
> >> any node with explanatory information that is not a custom scan
> >> node. This could be used to propagate and summarize information
> >> from custom scan nodes, but I had no immediate use for that so did
> >> not add it here. I would still be interested in hearing if you
> >> think this is something that would be useful to the community.
>
> That depends.
>

Just to elaborate: the intention was to allow a section to be added to
every node in the plan containing information from further down and also
allow this information to propagate upwards. We happen to have buffer
information right now, but allowing something similar to be added
dynamically by extending ExplainNode and passing down a callback to
standard_ExplainOneQuery.

Best wishes,
Mats Kindahl

  5. Re: Hooking into ExplainOneQuery() complicated by missing standard_ExplainOneQuery

    Michael Paquier <michael@paquier.xyz> — 2024-03-05T23:25:16Z

    On Tue, Mar 05, 2024 at 08:21:34AM +0100, Mats Kindahl wrote:
> I realize that a more advanced system is possible to create where you can
> customize the output even more, but in this case I just wanted to add a
> section with some additional information related to plan execution. Also,
> the code in explain.c seems to not be written with extensibility in mind,
> so I did not want to make too big a change here before thinking through how
> this would work.

Sure.

> Just to elaborate: the intention was to allow a section to be added to
> every node in the plan containing information from further down and also
> allow this information to propagate upwards. We happen to have buffer
> information right now, but allowing something similar to be added
> dynamically by extending ExplainNode and passing down a callback to
> standard_ExplainOneQuery.

Or an extra hook at the end of ExplainNode() to be able to append more
information at node level?  Not sure if others would agree with that,
though.
--
Michael

  6. Re: Hooking into ExplainOneQuery() complicated by missing standard_ExplainOneQuery

    Andrei Lepikhov <a.lepikhov@postgrespro.ru> — 2024-03-06T02:26:49Z

    On 6/3/2024 06:25, Michael Paquier wrote:
>> Just to elaborate: the intention was to allow a section to be added to
>> every node in the plan containing information from further down and also
>> allow this information to propagate upwards. We happen to have buffer
>> information right now, but allowing something similar to be added
>> dynamically by extending ExplainNode and passing down a callback to
>> standard_ExplainOneQuery.
> 
> Or an extra hook at the end of ExplainNode() to be able to append more
> information at node level?  Not sure if others would agree with that,
> though.

We already discussed EXPLAIN hooks, at least in [1]. IMO, extensions 
should have a chance to add something to the node explain and the 
summary, if only because they can significantly influence the planner 
and executor's behaviour.

[1] 
https://www.postgresql.org/message-id/flat/6cd5caa7-06e1-4460-bf35-00a59da3f677%40garret.ru

-- 
regards,
Andrei Lepikhov
Postgres Professional

  7. Re: Hooking into ExplainOneQuery() complicated by missing standard_ExplainOneQuery

    Mats Kindahl <mats@timescale.com> — 2024-03-06T07:31:31Z

    On Wed, Mar 6, 2024 at 3:27 AM Andrei Lepikhov <a.lepikhov@postgrespro.ru>
wrote:

> On 6/3/2024 06:25, Michael Paquier wrote:
> >> Just to elaborate: the intention was to allow a section to be added to
> >> every node in the plan containing information from further down and also
> >> allow this information to propagate upwards. We happen to have buffer
> >> information right now, but allowing something similar to be added
> >> dynamically by extending ExplainNode and passing down a callback to
> >> standard_ExplainOneQuery.
> >
> > Or an extra hook at the end of ExplainNode() to be able to append more
> > information at node level?  Not sure if others would agree with that,
> > though.
>

That is what I had in mind, yes.


> We already discussed EXPLAIN hooks, at least in [1]. IMO, extensions
> should have a chance to add something to the node explain and the
> summary, if only because they can significantly influence the planner
> and executor's behaviour.
>
> [1]
>
> https://www.postgresql.org/message-id/flat/6cd5caa7-06e1-4460-bf35-00a59da3f677%40garret.ru


This is an excellent example of where such a hook would be useful.
-- 
Best wishes,
Mats Kindahl, Timescale

  8. Re: Hooking into ExplainOneQuery() complicated by missing standard_ExplainOneQuery

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-06T09:32:01Z

    This patch would definitely be useful for Citus. We indeed currently
copy all of that code into our own explain hook. And it seems we
actually have some bug. Because the es->memory branches were not
copied (probably because this code didn't exist when we copied it).

https://github.com/citusdata/citus/blob/d59c93bc504ad32621d66583de6b65f936b0ed13/src/backend/distributed/planner/multi_explain.c#L1248-L1289

  9. Re: Hooking into ExplainOneQuery() complicated by missing standard_ExplainOneQuery

    Michael Paquier <michael@paquier.xyz> — 2024-03-06T22:45:01Z

    On Wed, Mar 06, 2024 at 10:32:01AM +0100, Jelte Fennema-Nio wrote:
> This patch would definitely be useful for Citus. We indeed currently
> copy all of that code into our own explain hook. And it seems we
> actually have some bug. Because the es->memory branches were not
> copied (probably because this code didn't exist when we copied it).
> 
> https://github.com/citusdata/citus/blob/d59c93bc504ad32621d66583de6b65f936b0ed13/src/backend/distributed/planner/multi_explain.c#L1248-L1289

That's nice.  You would be able to shave quite a bit of code.  If
there are no objections, I propose to apply the change of this thread
to have this standard explain wrapper at the beginning of next week.
If others have any comments, feel free.
--
Michael

  10. Re: Hooking into ExplainOneQuery() complicated by missing standard_ExplainOneQuery

    Michael Paquier <michael@paquier.xyz> — 2024-03-10T23:42:39Z

    On Thu, Mar 07, 2024 at 07:45:01AM +0900, Michael Paquier wrote:
> That's nice.  You would be able to shave quite a bit of code.  If
> there are no objections, I propose to apply the change of this thread
> to have this standard explain wrapper at the beginning of next week.
> If others have any comments, feel free.

Well, done as of a04ddd077e61.
--
Michael

  11. Re: Hooking into ExplainOneQuery() complicated by missing standard_ExplainOneQuery

    Mats Kindahl <mats@timescale.com> — 2024-03-11T06:59:48Z

    On Mon, Mar 11, 2024 at 12:42 AM Michael Paquier <michael@paquier.xyz>
wrote:

> On Thu, Mar 07, 2024 at 07:45:01AM +0900, Michael Paquier wrote:
> > That's nice.  You would be able to shave quite a bit of code.  If
> > there are no objections, I propose to apply the change of this thread
> > to have this standard explain wrapper at the beginning of next week.
> > If others have any comments, feel free.
>
> Well, done as of a04ddd077e61.
>

Thanks Michael!
-- 
Best wishes,
Mats Kindahl, Timescale