Re: [HACKERS] Infrastructure changes for recovery - Mailing list pgsql-patches

On Mon, 2008-09-08 at 13:34 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > Included patch with the following changes:
>
> > * new postmaster mode known as consistent recovery, entered only when
> > recovery passes safe/consistent point. InRedo is now set in all
> > processes when started/connected in consistent recovery mode.
>
> I looked at this and didn't like the InRedo signaling at all.  In the
> first place, it looks like you're expecting the flag to be passed down
> via fork(), but that won't work in EXEC_BACKEND mode.  (Yes, easily
> fixed, but it's wrong as-is.)

OK, thanks. I didn't check that.

>  In the second place, the method of
> signaling it to the bgwriter looks to have race conditions: in
> principle, at least, I think the startup process could try to clear
> the shared-memory InRedo flag before the bgwriter has gotten as far as
> setting it.  (This might work if the startup process can't exit redo
> mode without the bgwriter having finished a checkpoint, but such a
> dependency is too rube goldbergian for me.)

We never interrupt what the bgwriter does. If it is in the middle of
doing a checkpoint when recovery finishes, then Startup process just
waits behind it and then issues a shutdown checkpoint. If there's loads
of blocks to be written it doesn't matter who writes them. There's only
one soup spoon, and it doesn't matter who stirs the soup.

> ISTM that it would probably be better if there were exactly one InRedo
> flag in shared memory, probably in xlog.c's shared state, with the
> postmaster not being responsible for setting or clearing it; rather
> the startup process should do those things.

So replace InRedo with an IsInRedo() function that accesses XLogCtl?

Sounds good

> > * bgwriter and stats process starts in consistent recovery mode.
> > bgwriter changes mode when startup process completes.
>
> I'm not sure about the interaction of this.  In particular, what about
> recovery restart points before we have reached the safe stop point?
> I don't think we want to give up the capability of having those.

Maybe. I felt it made the code cleaner to give that up. Realistically
its a fairly short window of time. But shouldn't be too hard to put
back.

> Also, it seems pretty bogus to update the in-memory ControlFile
> checkpoint values before the restart point is actually done.  It looks
> to me like what you have done is to try to use those fields as signaling
> for the restart request in addition to their existing purposes, which
> I think is confusing and probably dangerous.  I'd rather there were a
> different signaling path and ControlFile maintains its currrent
> definition.

OK

> I found the changes in bgwriter.c unreadable.  You've evidently
> moved blocks of code around, but exactly what did you mean to
> change?  Why is there so much duplicated code now?

The patch utility did that. Some code reuse confused it. It's real easy
to read though if you apply the patch and then read the main loop.
You'll see what I mean.

> > I've kept the distinction between restartpoints and checkpoints in
> > code, to avoid convoluted code.
>
> Hmm, I dunno, it seems like that might be a bad choice.  Are you sure
> it's not cleaner to just use the regular checkpoint code?

When I tried to write it, it just looked to my eyes like every single
line had a caveat which looked ugly and multiplied the testing. You're
the code dude, always happy to structure things as you suggest. If
you're sure, that is.

Thanks for the review.

--
 Simon Riggs           www.2ndQuadrant.com
 PostgreSQL Training, Services and Support