Craig already replied to most points. I think we can sum up as "we've
done the best we can with what we have, maybe somebody can figure how to
do better in the future but for now this works."
Andres Freund wrote:
> On 2016-03-14 20:10:58 -0300, Alvaro Herrera wrote:
> > +#ifndef FRONTEND
> > + if (state->timelineHistory)
> > + list_free_deep(state->timelineHistory);
> > +#endif
>
> Hm. So we don't support timelines following for frontend code, although
> it'd be rather helpful for pg_xlogdump. And possibly pg_rewind.
Exactly. I had the same comment, but the argument that we would need to
rewrite a lot of timeline.c won me. That can wait for a future patch;
we certainly don't want to be rewriting these things during the last CF.
> > if (state->ReadRecPtr == InvalidXLogRecPtr)
> > - randAccess = true;
> > + randAccess = true; /* allow readPageTLI to go
> > backwards */
>
> randAccess is doing more than that, so I'm doubtful that comment is an
> improvment.
As I said, it's only a copy of what's already a few lines above. I
would be happier removing both, and instead adding an explanatory
comment about that variable elsewhere.
> > #include <unistd.h>
> >
> > -#include "miscadmin.h"
> > -
>
> spurious change imo.
Yeah, in a way this is cleanup after previous patches that have messed
up things somewhat. I wouldn't have posted it unless I was close to
committing this ... I think it'd be better to push these cleanup changes
separately.
> > /* Need to seek in the file? */
> > if (sendOff != startoff)
> > {
> > if (lseek(sendFile, (off_t) startoff, SEEK_SET) < 0)
> > - {
> > - char path[MAXPGPATH];
> > -
> > - XLogFilePath(path, tli, sendSegNo);
> > -
> > ereport(ERROR,
> > (errcode_for_file_access(),
> > errmsg("could not seek in log segment %s to
> > offset %u: %m",
> > - path, startoff)));
> > - }
> > + XLogFileNameP(tli, sendSegNo),
> > startoff)));
> > sendOff = startoff;
> > }
>
> Not a serious issue, more a general remark: I'm doubtful that going for
> palloc in error situations is good practice. This will be allocated in
> the current memory context; without access to the emergency error
> reserves.
Yeah, this code was copied from elsewhere in 422a55a68784f but there is
already another version of this routine in walsender which is using
XLogFileNameP instead of the stack-allocated one. I just made this one
more similar to the walsender's (which is in backend environment just
like this one) instead of continuing to use the frontend-limited
version (which is where this one was copied from).
I'm having a hard time getting concerned about using palloc there,
considering that every single call of XLogFileNameP occurs in an error
message. If we want to reject this one, surely we should change all the
others too.
> I'm also getting the feeling that the patch is bordering on doing some
> relatively random cleanups mixed in with architectural changes. Makes
> things a bit harder to review.
Yes.
> > +static void
> > +XLogReadDetermineTimeline(XLogReaderState *state)
> > +{
> > + /* Read the history on first time through */
> > + if (state->timelineHistory == NIL)
> > + state->timelineHistory = readTimeLineHistory(ThisTimeLineID);
> > +
> > + /*
> > + * Are we reading the record immediately following the one we read last
> > + * time? If not, then don't use the cached timeline info.
> > + */
> > + if (state->currRecPtr != state->EndRecPtr)
> > + {
> > + state->currTLI = 0;
> > + state->currTLIValidUntil = InvalidXLogRecPtr;
> > + }
>
> Hm. So we grow essentially a second version of the last end position and
> the randAccess stuff in XLogReadRecord().
Craig's argument against that seems reasonable to me.
> XLogReadDetermineTimeline() doesn't sit quite right with me, I do wonder
> whether there's not a simpler way to write this.
Feel free to suggest something :-)
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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
