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/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services