From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: jgdr@dalibo.com
Cc: michael@paquier.xyz
Cc: sawada.mshk@gmail.com
Cc: peter.eisentraut@2ndquadrant.com
Cc: pgsql-hackers@lists.postgresql.org
Cc: thomas.munro@enterprisedb.com
Cc: sk@zsrv.org
Cc: michael.paquier@gmail.com
Subject: Re: [HACKERS] Restricting maximum keep segments by repslots
Date: Sat, 16 May 2020 22:51:50 -0400
Message-ID: <20200517025150.GA12478@alvherre.pgsql> (raw)
In-Reply-To: <20200517010005.jzaf2245w4rrgs2o@alap3.anarazel.de>
On 2020-May-16, Andres Freund wrote:
> I, independent of this patch, added a few additional paths in which
> checkpointer's latch is reset, and I found a few shutdowns in regression
> tests to be extremely slow / timing out. The reason for that is that
> the only check for interrupts is at the top of the loop. So if
> checkpointer gets SIGUSR2 we don't see ShutdownRequestPending until we
> decide to do a checkpoint for other reasons.
Ah, yeah, this seems a genuine bug.
> I also suspect that it could have harmful consequences to not do a
> AbsorbSyncRequests() if something "ate" the set latch.
I traced through this when looking over the previous fix, and given that
checkpoint execution itself calls AbsorbSyncRequests frequently, I
don't think this one qualifies as a bug.
> I don't think it's reasonable to expect this much code between a
> ResetLatch and WaitLatch to never reset a latch. So I think we need to
> make the coding more robust in face of that. Without having to duplicate
> the top and the bottom of the loop.
That makes sense to me.
> One way to do that would be to WaitLatch() call to much earlier, and
> only do a WaitLatch() if do_checkpoint is false. Roughly like in the
> attached.
Hm. I'd do "WaitLatch() / continue" in the "!do_checkpoint" block, and
put the checpkoint code not in the else block; seems easier to read to
me.
While we're here, can we change CreateCheckPoint to return true so
that we can do
ckpt_performed = do_restartpoint ? CreateRestartPoint(flags) : CreateCheckPoint(flags);
instead of the mess we have there now? (Also add a comment that
CreateCheckPoint must not return false, to avoid messing with the
schedule)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@2ndquadrant.com, andres@anarazel.de, horikyota.ntt@gmail.com, jgdr@dalibo.com, michael@paquier.xyz, sawada.mshk@gmail.com, peter.eisentraut@2ndquadrant.com, pgsql-hackers@lists.postgresql.org, thomas.munro@enterprisedb.com, sk@zsrv.org, michael.paquier@gmail.com
Subject: Re: [HACKERS] Restricting maximum keep segments by repslots
In-Reply-To: <20200517025150.GA12478@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox