On 2023-06-13 00:52, James Coleman wrote:
> I've attached v27. The important change here in 0001 is that it
> guarantees the interrupt handler is re-entrant, since that was a bug
> exposed by my testing. I've also included 0002 which is only meant for
> testing (it attempts to log in the plan in every
> CHECK_FOR_INTERRUPTS() call).
When SIGINT is sent during ProcessLogQueryPlanInterrupt(),
ProcessLogQueryPlanInterruptActive can remain true.
After that, pg_log_query_plan() does nothing but just returns.
AFAIU, v26 logs plan for each pg_log_query_plan() even when another
pg_log_query_plan() is running, but it doesn't cause errors or
critical
problem.
Considering the problem solved and introduced by v27, I think v26
might
be fine.
How do you think?
The testing I did with calling this during every CFI is what uncovered
the re-entrancy problem. IIRC (without running that test again) the
problem was a stack overflow. Now, to be sure this is a particularly
degenerate case because in real-world usage it'd be impossible in
practice, I think, to trigger that many calls to this function (and by
extension the interrupt handler).
Yeah.In addition, currently only superusers are allowed to execute
pg_log_query_plan(), I think we don't need to think about cases
where users are malicious.
If SIGINT is the only concern we could reset
ProcessLogQueryPlanInterruptActive in error handling code. I admit
that part of my thought process here is thinking ahead to an
additional patch I'd like to see on top of this, which is logging a
query plan before cleaning up when statement timeout occurs.
I remember this is what you wanted do.[1]
The
re-entrancy issue becomes more interesting then, I think, since we
would then have automated calling of the logging code. BTW: I'd
thought that would make a nice follow-up patch for this, but if you'd
prefer I could add it as another patch in the series here.
What do you think about resetting the flag versus just not having it?
If I understand you correctly, adding the flag is not necessary for this
proposal.
To keep the patch simple, I prefer not having it.
[1]
https://www.postgresql.org/message-id/flat/CA%2BTgmoYW_rSOW4JMQ9_0Df9PKQ%3DsQDOKUGA4Gc9D8w4wui8fSA%40mail.gmail.com#b57432077f8045be8588049269f7a8dd
--
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
