MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Wed, 14 Jun 2023 00:22:14 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: James Coleman <jtc331@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@lists.postgresql.org,
 Greg Stark <stark@mit.edu>, Ronan Dunklau <ronan.dunklau@aiven.io>,
 david.christensen@crunchydata.com, Heikki Linnakangas <hlinnaka@iki.fi>
Subject: Re: RFC: Logging plan of the running query
In-Reply-To: <CAAaqYe9u-ywvRX32iBj1SkQrLf1hqMhR9HFCc0dAk4rGDJpyFw@mail.gmail.com>
References: <3bb7bacbc5ef9e58e2ece0557adac98b@oss.nttdata.com>
 <03c8dcbb-72d2-d3ad-3ad1-9efe69676529@oss.nttdata.com>
 <20220208.171852.1699766364147189900.horikyota.ntt@gmail.com>
 <ab761e68f3fbcee80cf1ca19b1db85f1@oss.nttdata.com>
 <3e94e9b3-83ef-c328-0c5c-f93aa3224790@oss.nttdata.com>
 <3d121ed5f81cef588bac836b43f5d1f9@oss.nttdata.com>
 <379d69c0b3d058997550f88e73ffddd6@oss.nttdata.com>
 <727712bcaa8d449615820c16d15e452e@oss.nttdata.com>
 <598171663576373@mail.yandex.ru>
 <38f4e19323b264e5975c60010b512b15@oss.nttdata.com>
 <20221206184101.exj2zyhmt7ta5fco@awork3.anarazel.de>
 <c51098ac613f908776e5564a7c7a0e16@oss.nttdata.com>
 <CAAaqYe9euUZD8bkjXTVcD9e4n5c7kzHzcvuCJXt-xds9X4c7Fw@mail.gmail.com>
 <2f466f82de025f745dff9377a368cee4@oss.nttdata.com>
 <CAAaqYe8LXVXQhYy3yT0QOHUymdM=uha0dJ0=BEPzVAx2nG1gsw@mail.gmail.com>
 <788e5db60b6c5de4b73fc9810b635d44@oss.nttdata.com>
 <CAAaqYe9u-ywvRX32iBj1SkQrLf1hqMhR9HFCc0dAk4rGDJpyFw@mail.gmail.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <9c3445ab0f695dbd0a879cf380f313c5@oss.nttdata.com>
Archived-At: <https://www.postgresql.org/message-id/9c3445ab0f695dbd0a879cf380f313c5%40oss.nttdata.com>
Precedence: bulk

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