SEO

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



---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

"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

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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



---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

"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

---------------------------(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

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



---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

"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

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

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



---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

"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

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

"Simon Riggs" <simon@2ndquadrant.com> writes:
> [ patch to remove logId/logSeg from pg_control ]

Applied with revisions.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

"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

---------------------------(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