On Mon, Sep 15, 2014 at 8:00 PM, Michael Paquier
<michael.paquier@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