Re: RFC: Logging plan of the running query

On 2023-06-03 02:51, James Coleman wrote:
> Hello,
>
> Thanks for working on this patch!
>
> On Thu, Dec 8, 2022 at 12:10 AM torikoshia <torikoshia(at)oss(dot)nttdata(dot)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.

Thanks a lot for the discussion and sharing it!
I really appreciate it.

BTW I'm not sure whether all the CFI are called in valid transaction,
do you think we should check each of them?

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

Is the concern here limited to the case where explain code goes crazy
as Robert pointed out?
If so, this may be a trade-off worth doing.
I am a little concerned about whether there will be cases where the
explain code is not problematic but just takes long time.

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

Checking the log output may be better, but I didn't add it since there
is only a little content that can be checked, and similar function
pg_log_backend_memory_contexts() does not do such type of tests.

> Are you interested in re-opening this patch? I'd be happy to provide
> further review and help to try to push this along.
Sure, I'm going to re-open this.

> I've rebased the patch and attached as v26.
Thanks again for your work!

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION