Thread: Re: [HACKERS] Configuring BLCKSZ and XLOGSEGSZ (in 8.3)
On Mon, 2006-11-27 at 18:26 -0500, Tom Lane wrote: > [ studies code a bit more... ] I'm also wondering whether the forced > pg_control update at each xlog seg switch is worth its keep. Offhand > it > seems like the checkpoint pointer is enough; why are we maintaining > logId/logSeg in pg_control? We maintain the values in shared memory to allow us to determine whether or not its time to checkpoint, and also to ensure that there is one and only one call to checkpoint. So we need to keep track of this somewhere and that may as well be where it already is. However, that doesn't mean we need to update the file on disk each time we switch xlog files, so I've removed the UpdateControlFile() at that point. That fsync was done while holding WALWriteLock() so removing it should be good for a few extra points of speed - at least we know there were some problems in that area. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
Attachment
"Simon Riggs" <simon@2ndquadrant.com> writes:
> On Mon, 2006-11-27 at 18:26 -0500, Tom Lane wrote:
>> [ studies code a bit more... ] I'm also wondering whether the forced
>> pg_control update at each xlog seg switch is worth its keep. Offhand
>> it seems like the checkpoint pointer is enough; why are we maintaining
>> logId/logSeg in pg_control?
> We maintain the values in shared memory to allow us to determine whether
> or not its time to checkpoint, and also to ensure that there is one and
> only one call to checkpoint. So we need to keep track of this somewhere
> and that may as well be where it already is.
Say again? AFAICT those fields are write-only; the only place we
consult them is to decide whether they need to be updated. My thought
was to remove 'em altogether.
regards, tom lane
On Tue, 2006-12-05 at 15:14 -0500, Tom Lane wrote: > "Simon Riggs" <simon@2ndquadrant.com> writes: > > On Mon, 2006-11-27 at 18:26 -0500, Tom Lane wrote: > >> [ studies code a bit more... ] I'm also wondering whether the forced > >> pg_control update at each xlog seg switch is worth its keep. Offhand > >> it seems like the checkpoint pointer is enough; why are we maintaining > >> logId/logSeg in pg_control? > > > We maintain the values in shared memory to allow us to determine whether > > or not its time to checkpoint, and also to ensure that there is one and > > only one call to checkpoint. So we need to keep track of this somewhere > > and that may as well be where it already is. > > Say again? AFAICT those fields are write-only; the only place we > consult them is to decide whether they need to be updated. My thought > was to remove 'em altogether. Thats what I thought originally. However, they guard the entrance to RequestCheckpoint() and after they have been set nobody else will call it - look at the test immediately prior to the rows changed by the patch. That comparison is why we still need them and why they aren't just write-only. So they need to be there, but we just don't need to write them to pg_control. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
"Simon Riggs" <simon@2ndquadrant.com> writes:
> On Tue, 2006-12-05 at 15:14 -0500, Tom Lane wrote:
>> Say again? AFAICT those fields are write-only; the only place we
>> consult them is to decide whether they need to be updated. My thought
>> was to remove 'em altogether.
> Thats what I thought originally.
> However, they guard the entrance to RequestCheckpoint() and after they
> have been set nobody else will call it - look at the test immediately
> prior to the rows changed by the patch.
Sure, what would happen is that every backend passing through this code
would execute the several lines of computation needed to decide whether
to call RequestCheckpoint. That's still way cheaper than an xlog switch
as a whole, so it doesn't bother me. I think the first test is probably
effectively redundant anyway, since the whole thing is executed with
WALWriteLock held and so there can be only one backend doing it at a
time --- it's not apparent to me that it's possible for someone else to
have updated pg_control before the backend executing XLogWrite does.
But in any case, the point here is that it doesn't matter whether the
RequestCheckpoint code is inside the update-pg_control test or not.
It was only put there on the thought that we could save some small
number of cycles by not doing it if the update-pg_control test failed.
regards, tom lane
On Tue, 2006-12-05 at 16:24 -0500, Tom Lane wrote: > "Simon Riggs" <simon@2ndquadrant.com> writes: > > On Tue, 2006-12-05 at 15:14 -0500, Tom Lane wrote: > >> Say again? AFAICT those fields are write-only; the only place we > >> consult them is to decide whether they need to be updated. My thought > >> was to remove 'em altogether. > > > Thats what I thought originally. > > > However, they guard the entrance to RequestCheckpoint() and after they > > have been set nobody else will call it - look at the test immediately > > prior to the rows changed by the patch. > > Sure, what would happen is that every backend passing through this code > would execute the several lines of computation needed to decide whether > to call RequestCheckpoint. That's still way cheaper than an xlog switch > as a whole, so it doesn't bother me. I think the first test is probably > effectively redundant anyway, since the whole thing is executed with > WALWriteLock held and so there can be only one backend doing it at a > time --- it's not apparent to me that it's possible for someone else to > have updated pg_control before the backend executing XLogWrite does. Right, but the calculation uses RedoRecPtr, which may not be completely up to date. So presumably you want to re-read the shared memory value again to make sure we are exactly accurate and allow only one person to call checkpoint? Either way we have to take a lock. Insert lock causes deadlock, so we would need to use infolock. Yes, one backend at a time executes this code, but we need a way to tell whether the backend is the first to come through that code. I just left it with the lock it was already requesting. If you really think it should use infolock then I'll code it that way instead. > But in any case, the point here is that it doesn't matter whether the > RequestCheckpoint code is inside the update-pg_control test or not. > It was only put there on the thought that we could save some small > number of cycles by not doing it if the update-pg_control test failed. Understood, that wasn't why I left it that way. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
"Simon Riggs" <simon@2ndquadrant.com> writes:
> On Tue, 2006-12-05 at 16:24 -0500, Tom Lane wrote:
>> Sure, what would happen is that every backend passing through this code
>> would execute the several lines of computation needed to decide whether
>> to call RequestCheckpoint.
> Right, but the calculation uses RedoRecPtr, which may not be completely
> up to date. So presumably you want to re-read the shared memory value
> again to make sure we are exactly accurate and allow only one person to
> call checkpoint? Either way we have to take a lock. Insert lock causes
> deadlock, so we would need to use infolock.
Not at all. It's highly unlikely that RedoRecPtr would be so out of
date as to result in a false request for a checkpoint, and if it does,
so what? Worst case is we perform an extra checkpoint.
Also, given the current structure of the routine, this is probably not
the best place for that code at all --- it'd make more sense for it to
be in the just-finished-a-segment code stretch, which would ensure that
it's only done by one backend once per segment.
regards, tom lane
On Tue, 2006-12-05 at 17:26 -0500, Tom Lane wrote: > "Simon Riggs" <simon@2ndquadrant.com> writes: > > On Tue, 2006-12-05 at 16:24 -0500, Tom Lane wrote: > >> Sure, what would happen is that every backend passing through this code > >> would execute the several lines of computation needed to decide whether > >> to call RequestCheckpoint. > > > Right, but the calculation uses RedoRecPtr, which may not be completely > > up to date. So presumably you want to re-read the shared memory value > > again to make sure we are exactly accurate and allow only one person to > > call checkpoint? Either way we have to take a lock. Insert lock causes > > deadlock, so we would need to use infolock. > > Not at all. It's highly unlikely that RedoRecPtr would be so out of > date as to result in a false request for a checkpoint, and if it does, > so what? Worst case is we perform an extra checkpoint. On its own, I wouldn't normally agree... > Also, given the current structure of the routine, this is probably not > the best place for that code at all --- it'd make more sense for it to > be in the just-finished-a-segment code stretch, which would ensure that > it's only done by one backend once per segment. But thats a much better plan since it requires no locking. There's a lot more changes there for such a simple fix though and lots more potential bugs, but I've coded it as you suggest and removed the fields from pg_control. Patch passes make check, applies cleanly on HEAD. pg_resetxlog and pgcontroldata tested. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
"Simon Riggs" <simon@2ndquadrant.com> writes:
> [ patch to remove logId/logSeg from pg_control ]
Looking this over, I realize that there's an unresolved problem.
Although it's true that xlog.c itself doesn't use the logId/logSeg
fields for anything interesting, pg_resetxlog relies on them to
determine how far the old WAL extends, so that it can determine a
safely higher start address for the new WAL. This puts a damper
both on my thought of removing the fields altogether, and on Simon's
earlier proposal to update them in shared memory but not immediately
write pg_control during a segment switch.
The proposed patch uses pg_control's last checkpoint location to
drive the end-of-WAL computation, but that is obviously not good
enough, as WAL might have gone many segments beyond that.
Now, underestimating the WAL end address is not fatal; AFAIK the
only consequence would be some complaints about "xlog flush request is
not satisfied" until we had managed to advance the end of WAL past the
largest page LSN present in the data files. But it's still annoying.
What I'm considering is having pg_resetxlog scan the pg_xlog directory
and assume that any segment files present might have been used.
Thoughts?
regards, tom lane
"Simon Riggs" <simon@2ndquadrant.com> writes:
> [ patch to remove logId/logSeg from pg_control ]
Applied with revisions.
regards, tom lane
On Fri, 2006-12-08 at 13:18 -0500, Tom Lane wrote: > "Simon Riggs" <simon@2ndquadrant.com> writes: > What I'm considering is having pg_resetxlog scan the pg_xlog directory > and assume that any segment files present might have been used. [Reading committed code...] That's a very neat short cut - just using the file names rather than trying to read the files themselves as the abortive patch a few months back tried. Question: What happens when we run out of LogIds? I know we don't wrap onto the next timeline, but do we start from LogId=1 again? Looks to me like we just fall on the floor right now. We're probably not pressed for an answer... :-) -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
"Simon Riggs" <simon@2ndquadrant.com> writes:
> Question: What happens when we run out of LogIds?
We die. That will occur after generating 2^64 bytes of WAL, which for
an installation generating 100MB/second would be something over 5000
years if I'm counting correctly.
regards, tom lane
The original discussion of this patch was here:
http://archives.postgresql.org/pgsql-hackers/2006-11/msg00876.php
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Simon Riggs wrote:
> On Tue, 2006-12-05 at 17:26 -0500, Tom Lane wrote:
> > "Simon Riggs" <simon@2ndquadrant.com> writes:
> > > On Tue, 2006-12-05 at 16:24 -0500, Tom Lane wrote:
> > >> Sure, what would happen is that every backend passing through this code
> > >> would execute the several lines of computation needed to decide whether
> > >> to call RequestCheckpoint.
> >
> > > Right, but the calculation uses RedoRecPtr, which may not be completely
> > > up to date. So presumably you want to re-read the shared memory value
> > > again to make sure we are exactly accurate and allow only one person to
> > > call checkpoint? Either way we have to take a lock. Insert lock causes
> > > deadlock, so we would need to use infolock.
> >
> > Not at all. It's highly unlikely that RedoRecPtr would be so out of
> > date as to result in a false request for a checkpoint, and if it does,
> > so what? Worst case is we perform an extra checkpoint.
>
> On its own, I wouldn't normally agree...
>
> > Also, given the current structure of the routine, this is probably not
> > the best place for that code at all --- it'd make more sense for it to
> > be in the just-finished-a-segment code stretch, which would ensure that
> > it's only done by one backend once per segment.
>
> But thats a much better plan since it requires no locking.
>
> There's a lot more changes there for such a simple fix though and lots
> more potential bugs, but I've coded it as you suggest and removed the
> fields from pg_control.
>
> Patch passes make check, applies cleanly on HEAD.
> pg_resetxlog and pgcontroldata tested.
>
>
> --
> Simon Riggs
> EnterpriseDB http://www.enterprisedb.com
>
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
On Sat, 2007-02-03 at 20:37 -0500, Bruce Momjian wrote: > Your patch has been added to the PostgreSQL unapplied patches list at: > > http://momjian.postgresql.org/cgi-bin/pgpatches > > It will be applied as soon as one of the PostgreSQL committers reviews > and approves it. Tom applied the patch a few months ago. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
Patch already applied by Tom. Removed from queue. --------------------------------------------------------------------------- Simon Riggs wrote: > On Tue, 2006-12-05 at 17:26 -0500, Tom Lane wrote: > > "Simon Riggs" <simon@2ndquadrant.com> writes: > > > On Tue, 2006-12-05 at 16:24 -0500, Tom Lane wrote: > > >> Sure, what would happen is that every backend passing through this code > > >> would execute the several lines of computation needed to decide whether > > >> to call RequestCheckpoint. > > > > > Right, but the calculation uses RedoRecPtr, which may not be completely > > > up to date. So presumably you want to re-read the shared memory value > > > again to make sure we are exactly accurate and allow only one person to > > > call checkpoint? Either way we have to take a lock. Insert lock causes > > > deadlock, so we would need to use infolock. > > > > Not at all. It's highly unlikely that RedoRecPtr would be so out of > > date as to result in a false request for a checkpoint, and if it does, > > so what? Worst case is we perform an extra checkpoint. > > On its own, I wouldn't normally agree... > > > Also, given the current structure of the routine, this is probably not > > the best place for that code at all --- it'd make more sense for it to > > be in the just-finished-a-segment code stretch, which would ensure that > > it's only done by one backend once per segment. > > But thats a much better plan since it requires no locking. > > There's a lot more changes there for such a simple fix though and lots > more potential bugs, but I've coded it as you suggest and removed the > fields from pg_control. > > Patch passes make check, applies cleanly on HEAD. > pg_resetxlog and pgcontroldata tested. > > > -- > Simon Riggs > EnterpriseDB http://www.enterprisedb.com > [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 1: if posting/reading through Usenet, please send an appropriate > subscribe-nomail command to majordomo@postgresql.org so that your > message can get through to the mailing list cleanly -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +