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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
