On 11 January 2016 at 19:03, Andres Freund <and...@anarazel.de> wrote:
> Hi,
>
> On 2016-01-09 15:29:11 +0000, Simon Riggs wrote:
> > Hmm, I was just preparing this for commit.
>
> Just read downthread that you want to commit this soon. Please hold of
> for a while, this doesn't really look ready to me. I don't have time for
> a real review right now, but I'll try to get to it asap.
>
"A real review"? Huh.
> > +
> > +/*
> > + * Reads 2PC data from xlog. During checkpoint this data will be moved
> to
> > + * twophase files and ReadTwoPhaseFile should be used instead.
> > + */
> > +static void
> > +XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
> > +{
> > + XLogRecord *record;
> > + XLogReaderState *xlogreader;
> > + char *errormsg;
> > +
> > + xlogreader = XLogReaderAllocate(&logical_read_local_xlog_page,
> > NULL);
>
> logical_read_local_xlog_page isn't really suitable for the use
> here. Besides the naming issue, at the very least it'll be wrong during
> WAL replay in the presence of promotions on an upstream node - it
> doesn't dealwith timelines.
>
I'm aware of that, though note that it isn't being used in that way here.
> More generally, I'm doubtful that the approach of reading data from WAL
> as proposed here is a very good idea. It seems better to "just" dump the
> entire 2pc state into *one* file at checkpoint time.
>
I think you misunderstand the proposed approach. This isn't just to do
with reading things back at checkpoint.
--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
