On Mon, Sep 15, 2014 at 8:00 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > Alvaro got faster than me... I was just looking at the first patch and > +1 on those comments. It is worth noting that the first patch, as it > does only a large refactoring, does not impact performance or size of > WAL records.
Some comments after a second and closer read of the first patch: 1) Wouldn't it be better to call GetFullPageWriteInfo directly in XLogRecordAssemble, removing RedoRecPtr and doPageWrites from its arguments? 2) XLogCheckBufferNeedsBackup is not used. It can be removed. 3) If I am following correctly, there are two code paths where XLogInsertRecData can return InvalidXLogRecPtr, and then there is this process in XLogInsert + { + /* + * Undo the changes we made to the rdata chain. + * + * XXX: This doesn't undo *all* the changes; the XLogRecData + * entries for buffers that we had already decided to back up have + * had their data-pointers cleared. That's OK, as long as we + * decide to back them up on the next iteration as well. Hence, + * don't allow "doPageWrites" value to go from true to false after + * we've modified the rdata chain. + */ + bool newDoPageWrites; + + GetFullPageWriteInfo(&RedoRecPtr, &newDoPageWrites); + if (!doPageWrites && newDoPageWrites) + doPageWrites = true; + rdt_lastnormal->next = NULL; + } Wouldn't it be better to keep that in XLogInsertRecData? This way GetFullPageWriteInfo could be removed (I actually see little point in keeping it as part of this patch, but does this change make its sense in patch 2?). 4) This patch is in DOS encoding (?!) That's all I have for now... Regards, -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers