On 2013-01-17 18:50:35 +0200, Heikki Linnakangas wrote:
> On 17.01.2013 18:42, Andres Freund wrote:
> >On 2013-01-17 18:33:42 +0200, Heikki Linnakangas wrote:
> >>On 17.01.2013 17:42, Andres Freund wrote:
> >>>Ok, the attached patch seems to fix a) and b). c) above is bogus, as
> >>>explained in a comment in the patch. I also noticed that the TLI check
> >>>didn't mark the last source as failed.
> >>
> >>This looks fragile:
> >>
> >>> /*
> >>> * We only end up here without a message when XLogPageRead() failed
> >>> * - in that case we already logged something.
> >>> * In StandbyMode that only happens if we have been triggered, so
> >>> * we shouldn't loop anymore in that case.
> >>> */
> >>> if (errormsg == NULL)
> >>> break;
> >>
> >>I don't like relying on the presence of an error message to control logic
> >>like that. Should we throw in an explicit CheckForStandbyTrigger() check in
> >>the condition of that loop?
> >
> >I agree, I wasn't too happy about that either. But in some sense its
> >only propagating state from XLogReadPage which already has dealt with
> >the error and decided its ok.
> >Its the solution closest to what happened in the old implementation,
> >thats why I thought it would be halfway to acceptable.
> >
> >Adding the CheckForStandbyTrigger() in the condition would mean
> >promotion would happen before all the available records are processed
> >and it would increase the amount of stat()s tremendously.
> >So I don't really like that either.
>
> I was thinking of the attached. As long as we check for
> CheckForStandbyTrigger() after the "record == NULL" check, we won't perform
> extra stat() calls on successful reads, only when we're polling after
> reaching the end of valid WAL. That seems acceptable.
Looks good to me.
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services