Hello, Thanks for working on this patch!
On Thu, Dec 8, 2022 at 12:10 AM torikoshia <torikos...@oss.nttdata.com> wrote: > > BTW, since this patch depends on ProcessInterrupts() and EXPLAIN codes > which is used in auto_explain, I'm feeling that the following discussion > also applies to this patch. > > > -- > > https://www.postgresql.org/message-id/CA%2BTgmoYW_rSOW4JMQ9_0Df9PKQ%3DsQDOKUGA4Gc9D8w4wui8fSA%40mail.gmail.com > > > > explaining a query is a pretty > > complicated operation that involves catalog lookups and lots of > > complicated stuff, and I don't think that it would be safe to do all > > of that at any arbitrary point in the code where ProcessInterrupts() > > happened to fire. > > If I can't come up with some workaround during the next Commitfest, I'm > going to withdraw this proposal. While at PGCon this week I'd brought up this idea with a few people without realizing you'd already worked on it previously, and then after I discovered this thread several of us (Greg, Ronan, David, Heikki, and myself -- all cc'd) discussed the safety concerns over dinner last night. Our conclusion was that all of the concerns we could come up with (for example, walking though the code for ExplainTargetRel() and discussing the relevant catalog and syscache accesses) all applied specifically to Robert's concerns about running explain inside an aborted transaction. After all, I'd originally started that thread to ask about running auto-explain after a query timeout. To put it positively: we believe that, for example, catalog accesses inside CHECK_FOR_INTERRUPTS() -- assuming that the CFI call is inside an existing valid transaction/query state, as it would be for this patch -- are safe. If there were problems, then those problems are likely bugs we already have in other CFI cases. Another concern Robert raised may apply here: what if a person tries to cancel a query when we're explaining? I believe that's a reasonable question to ask, but I believe it's a trade-off that's worth making for the (significant) introspection benefits this patch would provide. On to the patch itself: overall I think it looks like it's in pretty good shape. I also noticed we don't have any tests (I assume it'd have to be TAP tests) of the actual output happening, and I think it would be worth adding that. Are you interested in re-opening this patch? I'd be happy to provide further review and help to try to push this along. I've rebased the patch and attached as v26. Thanks, James Coleman
v26-0001-Add-function-to-log-the-plan-of-the-query.patch
Description: Binary data
