Thread: CheckpointLock needed in CreateCheckPoint()?
Hi ALL, Snip from CreateCheckPoint(): -- /* * Acquire CheckpointLock to ensure only one checkpoint happens at a time. * (This is just pro forma, since in the present system structure there is * only one process that is allowed to issue checkpoints at any given * time.) */ LWLockAcquire(CheckpointLock, LW_EXCLUSIVE); -- As per this comment, it seems to be that we don't really need this LW lock. We could have something else instead if we are afraid of having multiple checkpoints at any given time which isn't possible, btw. This CheckpointLock is acquired almost at the start of CreateCheckPoint() and released at the end. The in-between code can take a while to be complete. All that time interrupt will be on hold which doesn't seem to be a great idea but that wasn't problematic until now. I am having trouble due to this interrupt hold for a long since I was trying to add code changes where the checkpointer process is suppose to respond to the system read-write state change request as soon as possible[1]. That cannot be done if the interrupt is disabled since read-write state change uses the global barrier mechanism to convey system state changes to other running processes. So my question is, can we completely get rid of the CheckpointLock need in CreateCheckPoint()? Thoughts/Comments/Suggestions? Thank you! Regards, Amul 1]http://postgr.es/m/CA+TgmoYexwDQjdd1=15KMz+7VfHVx8VHNL2qjRRK92P=CSZDxg@mail.gmail.com
On Thu, Jan 7, 2021 at 11:32 AM Amul Sul <sulamul@gmail.com> wrote:
> Snip from CreateCheckPoint():
> --
> /*
> * Acquire CheckpointLock to ensure only one checkpoint happens at a time.
> * (This is just pro forma, since in the present system structure there is
> * only one process that is allowed to issue checkpoints at any given
> * time.)
> */
> LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
> --
>
> As per this comment, it seems to be that we don't really need this LW lock. We
> could have something else instead if we are afraid of having multiple
> checkpoints at any given time which isn't possible, btw.
Looks like CheckpointLock() can also be called in standalone backends
(RequestCheckpoint->CreateCheckPoint) along with the checkpointer
process. Having said that, I'm not quite sure whether it can get
called at the same time from a backend(may be with checkpoint;
command) and the checkpointer process.
/*
* If in a standalone backend, just do it ourselves.
*/
if (!IsPostmasterEnvironment)
{
/*
* There's no point in doing slow checkpoints in a standalone backend,
* because there's no other backends the checkpoint could disrupt.
*/
CreateCheckPoint(flags | CHECKPOINT_IMMEDIATE);
See the below comment for IsPostmasterEnvironment:
/*
* IsPostmasterEnvironment is true in a postmaster process and any postmaster
* child process; it is false in a standalone process (bootstrap or
* standalone backend).
> This CheckpointLock is acquired almost at the start of CreateCheckPoint() and
> released at the end. The in-between code can take a while to be complete. All
> that time interrupt will be on hold which doesn't seem to be a great idea but
> that wasn't problematic until now. I am having trouble due to this interrupt
> hold for a long since I was trying to add code changes where the checkpointer
> process is suppose to respond to the system read-write state change request as
> soon as possible[1]. That cannot be done if the interrupt is disabled
> since read-write state change uses the global barrier mechanism to convey system
> state changes to other running processes.
>
> So my question is, can we completely get rid of the CheckpointLock need in
> CreateCheckPoint()?
>
> Thoughts/Comments/Suggestions?
I don't think we can completely get rid of CheckpointLock in
CreateCheckPoint given the fact that it can get called from multiple
processes.
How about serving that ASRO barrier request alone for checkpointer
process in ProcessInterrupts even though the CheckpointLock is held by
the checkpointer process? And also while the checkpointing is
happening, may be we should call CHECK_FOR_INTERRUPTS to see if the
ASRO barrier has arrived. This may sound bit hacky, but just a
thought. I'm thinking that in ProcessInterrupts we can get to know the
checkpointer process type via MyAuxProcType, and whether the lock is
held or not using CheckpointLock, and then we can handle the ASRO
global barrier request.
I may be missing something.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 7, 2021 at 12:45 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Thu, Jan 7, 2021 at 11:32 AM Amul Sul <sulamul@gmail.com> wrote: > > Snip from CreateCheckPoint(): > > -- > > /* > > * Acquire CheckpointLock to ensure only one checkpoint happens at a time. > > * (This is just pro forma, since in the present system structure there is > > * only one process that is allowed to issue checkpoints at any given > > * time.) > > */ > > LWLockAcquire(CheckpointLock, LW_EXCLUSIVE); > > -- > > > > As per this comment, it seems to be that we don't really need this LW lock. We > > could have something else instead if we are afraid of having multiple > > checkpoints at any given time which isn't possible, btw. > > Looks like CheckpointLock() can also be called in standalone backends > (RequestCheckpoint->CreateCheckPoint) along with the checkpointer > process. Having said that, I'm not quite sure whether it can get > called at the same time from a backend(may be with checkpoint; > command) and the checkpointer process. If it is a standalone backend then there will be no postmaster and no other process i.e. no checkpoint process also. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 7, 2021 at 12:45 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Jan 7, 2021 at 11:32 AM Amul Sul <sulamul@gmail.com> wrote:
> > Snip from CreateCheckPoint():
> > --
> > /*
> > * Acquire CheckpointLock to ensure only one checkpoint happens at a time.
> > * (This is just pro forma, since in the present system structure there is
> > * only one process that is allowed to issue checkpoints at any given
> > * time.)
> > */
> > LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
> > --
> >
> > As per this comment, it seems to be that we don't really need this LW lock. We
> > could have something else instead if we are afraid of having multiple
> > checkpoints at any given time which isn't possible, btw.
>
> Looks like CheckpointLock() can also be called in standalone backends
> (RequestCheckpoint->CreateCheckPoint) along with the checkpointer
> process. Having said that, I'm not quite sure whether it can get
> called at the same time from a backend(may be with checkpoint;
> command) and the checkpointer process.
>
> /*
> * If in a standalone backend, just do it ourselves.
> */
> if (!IsPostmasterEnvironment)
> {
> /*
> * There's no point in doing slow checkpoints in a standalone backend,
> * because there's no other backends the checkpoint could disrupt.
> */
> CreateCheckPoint(flags | CHECKPOINT_IMMEDIATE);
>
> See the below comment for IsPostmasterEnvironment:
>
> /*
> * IsPostmasterEnvironment is true in a postmaster process and any postmaster
> * child process; it is false in a standalone process (bootstrap or
> * standalone backend).
>
I am not sure I understood your point completely. Do you mean there could be
multiple bootstrap or standalone backends that could exist at a time and can
perform checkpoint?
> > This CheckpointLock is acquired almost at the start of CreateCheckPoint() and
> > released at the end. The in-between code can take a while to be complete. All
> > that time interrupt will be on hold which doesn't seem to be a great idea but
> > that wasn't problematic until now. I am having trouble due to this interrupt
> > hold for a long since I was trying to add code changes where the checkpointer
> > process is suppose to respond to the system read-write state change request as
> > soon as possible[1]. That cannot be done if the interrupt is disabled
> > since read-write state change uses the global barrier mechanism to convey system
> > state changes to other running processes.
> >
> > So my question is, can we completely get rid of the CheckpointLock need in
> > CreateCheckPoint()?
> >
> > Thoughts/Comments/Suggestions?
>
> I don't think we can completely get rid of CheckpointLock in
> CreateCheckPoint given the fact that it can get called from multiple
> processes.
>
How?
> How about serving that ASRO barrier request alone for checkpointer
> process in ProcessInterrupts even though the CheckpointLock is held by
> the checkpointer process? And also while the checkpointing is
> happening, may be we should call CHECK_FOR_INTERRUPTS to see if the
> ASRO barrier has arrived. This may sound bit hacky, but just a
> thought. I'm thinking that in ProcessInterrupts we can get to know the
> checkpointer process type via MyAuxProcType, and whether the lock is
> held or not using CheckpointLock, and then we can handle the ASRO
> global barrier request.
>
I am afraid that this will easily get rejected by the community. Note that this
is a very crucial code path of the database server. There are other options
too, but don't want to propose them until clarity on the CheckpointLock
necessity.
Regards,
Amul
On Thu, Jan 7, 2021 at 1:22 PM Amul Sul <sulamul@gmail.com> wrote: > I am not sure I understood your point completely. Do you mean there could be > multiple bootstrap or standalone backends that could exist at a time and can > perform checkpoint? Actually, my understanding of the standalone backend was wrong initially. Now, I'm clear with that. > > I don't think we can completely get rid of CheckpointLock in > > CreateCheckPoint given the fact that it can get called from multiple > > processes. > > > > How? When the server is run in a standalone backend, it seems like there can be only one process running in single user mode, as pointed by Dilip upthread, so there will be no simultaneous calls to CreateCheckPoint. > > How about serving that ASRO barrier request alone for checkpointer > > process in ProcessInterrupts even though the CheckpointLock is held by > > the checkpointer process? And also while the checkpointing is > > happening, may be we should call CHECK_FOR_INTERRUPTS to see if the > > ASRO barrier has arrived. This may sound bit hacky, but just a > > thought. I'm thinking that in ProcessInterrupts we can get to know the > > checkpointer process type via MyAuxProcType, and whether the lock is > > held or not using CheckpointLock, and then we can handle the ASRO > > global barrier request. > > > > I am afraid that this will easily get rejected by the community. Note that this > is a very crucial code path of the database server. There are other options > too, but don't want to propose them until clarity on the CheckpointLock > necessity. I understand that. I'm sorry, if I have sidetracked the main point here. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 7, 2021 at 1:02 AM Amul Sul <sulamul@gmail.com> wrote: > As per this comment, it seems to be that we don't really need this LW lock. We > could have something else instead if we are afraid of having multiple > checkpoints at any given time which isn't possible, btw. Yeah, I think this lock is useless. In fact, I think it's harmful, because it makes large sections of the checkpointer code, basically all of it really, run with interrupts held off for no reason. It's not impossible that removing the lock could reveal bugs elsewhere: suppose we have segments of code that actually aren't safe to interrupt, but because of this LWLock, it never happens. But, if that happens to be true, I think we should just view those cases as bugs to be fixed. One thing that blunts the impact of this quite a bit is that the checkpointer doesn't use rely much on CHECK_FOR_INTERRUPTS() in the first place. It has its own machinery: HandleCheckpointerInterrupts(). Possibly that's something we should be looking to refactor somehow as well. -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, Jan 14, 2021 at 10:21 AM Robert Haas <robertmhaas@gmail.com> wrote: > Yeah, I think this lock is useless. In fact, I think it's harmful, > because it makes large sections of the checkpointer code, basically > all of it really, run with interrupts held off for no reason. > > It's not impossible that removing the lock could reveal bugs > elsewhere: suppose we have segments of code that actually aren't safe > to interrupt, but because of this LWLock, it never happens. But, if > that happens to be true, I think we should just view those cases as > bugs to be fixed. > > One thing that blunts the impact of this quite a bit is that the > checkpointer doesn't use rely much on CHECK_FOR_INTERRUPTS() in the > first place. It has its own machinery: HandleCheckpointerInterrupts(). > Possibly that's something we should be looking to refactor somehow as > well. Here's a patch to remove CheckpointLock completely. For ProcessInterrupts() to do anything, one of the following things would have to be true: 1. ProcDiePending = true. Normally this is set by die(), but the checkpointer does not use die(). Perhaps someday it should, or maybe not, but that's an issue for another day. 2. ClientConnectionLost = true. This gets set in pqcomm.c's internal_flush(), but the checkpointer has no client connection. 3. DoingCommandRead = true. Can't happen; only set in PostgresMain(). 4. QueryCancelPending = true. Can be set by recovery conflicts, but those shouldn't happen in the checkpointer. Normally set by StatementCancelHandler, which the checkpointer doesn't use. 5. IdleInTransactionSessionTimeoutPending = true. Set up by InitPostgres(), which is not used by auxiliary processes. 6. ProcSignalBarrierPending = true. This is the case we actually care about allowing for the ASRO patch, so if this occurs, it's good. 7. ParallelMessagePending = true. The checkpointer definitely never starts parallel workers. So I don't see any problem with just ripping this out entirely, but I'd like to know if anybody else does. -- Robert Haas EDB: http://www.enterprisedb.com
Attachment
Robert Haas <robertmhaas@gmail.com> writes:
> Here's a patch to remove CheckpointLock completely. ...
> So I don't see any problem with just ripping this out entirely, but
> I'd like to know if anybody else does.
If memory serves, the reason for the lock was that the CHECKPOINT
command used to run the checkpointing code directly in the calling
backend, so we needed it to keep more than one process from doing
that at once. AFAICS, it's no longer possible for more than one
process to try to run that code concurrently, so we shouldn't need
the lock anymore.
regards, tom lane
On Mon, Jan 18, 2021 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > If memory serves, the reason for the lock was that the CHECKPOINT > command used to run the checkpointing code directly in the calling > backend, so we needed it to keep more than one process from doing > that at once. AFAICS, it's no longer possible for more than one > process to try to run that code concurrently, so we shouldn't need > the lock anymore. Interesting. I think that must have been a *very* long time ago. Perhaps 076a055acf3c55314de267c62b03191586d79cf6 from 2004? -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jan 18, 2021 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If memory serves, the reason for the lock was that the CHECKPOINT
>> command used to run the checkpointing code directly in the calling
>> backend, so we needed it to keep more than one process from doing
>> that at once. AFAICS, it's no longer possible for more than one
>> process to try to run that code concurrently, so we shouldn't need
>> the lock anymore.
> Interesting. I think that must have been a *very* long time ago.
Yeah. Digging further, it looks like I oversimplified things above:
we once launched special background-worker-like processes for checkpoints,
and there could be more than one at a time. That seems to have changed
here:
Author: Tom Lane <tgl@sss.pgh.pa.us>
Branch: master Release: REL8_0_BR [076a055ac] 2004-05-29 22:48:23 +0000
Separate out bgwriter code into a logically separate module, rather
than being random pieces of other files. Give bgwriter responsibility
for all checkpoint activity (other than a post-recovery checkpoint);
so this child process absorbs the functionality of the former transient
checkpoint and shutdown subprocesses.
At the time I thought the CheckpointLock had become totally pro forma,
but there are later references to having to prevent a checkpoint from
running concurrently with a restartpoint, which was originally done
within the startup/WAL-replay process. It looks like that changed here:
Author: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Branch: master Release: REL8_4_BR [7e48b77b1] 2009-06-25 21:36:00 +0000
Fix some serious bugs in archive recovery, now that bgwriter is active
during it:
When bgwriter is active, the startup process can't perform mdsync() correctly
because it won't see the fsync requests accumulated in bgwriter's private
pendingOpsTable. Therefore make bgwriter responsible for the end-of-recovery
checkpoint as well, when it's active.
(The modern checkpointer process wasn't split out of bgwriter until
806a2aee3 of 2011-11-01.)
regards, tom lane
On Mon, Jan 18, 2021 at 02:36:48PM -0500, Robert Haas wrote: > Here's a patch to remove CheckpointLock completely. For > ProcessInterrupts() to do anything, one of the following things would > have to be true: > > [...] > > So I don't see any problem with just ripping this out entirely, but > I'd like to know if anybody else does. Agreed, +1. -- Michael
Attachment
On Mon, Jan 18, 2021 at 5:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Yeah. Digging further, it looks like I oversimplified things above: > we once launched special background-worker-like processes for checkpoints, > and there could be more than one at a time. Thanks. I updated the commit message to mention some of the relevant commits, and then committed this to master. -- Robert Haas EDB: http://www.enterprisedb.com