On Tue, Jan 12, 2016 at 3:35 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Tue, Jan 12, 2016 at 4:57 AM, Simon Riggs <si...@2ndquadrant.com> wrote: >> Performance tests for me show that the patch is effective; my results match >> Jesper's roughly in relative numbers. >> >> My robustness review is that the approach and implementation are safe. >> >> It's clear there are various additional tuning opportunities, but the >> objective of the current patch to improve performance is very, very clearly >> met, so I'm aiming to commit *this* patch soon. > > - /* initialize LSN to 0 (start of WAL) */ > - gxact->prepare_lsn = 0; > + /* initialize LSN to InvalidXLogRecPtr */ > + gxact->prepare_start_lsn = InvalidXLogRecPtr; > + gxact->prepare_end_lsn = InvalidXLogRecPtr; > > I think that it would be better to explicitly initialize gxact->ondisk > to false here. > > + xlogreader = XLogReaderAllocate(&logical_read_local_xlog_page, NULL); > + if (!xlogreader) > + ereport(ERROR, > + (errcode(ERRCODE_OUT_OF_MEMORY), > + errmsg("out of memory"), > + errdetail("Failed while allocating an > XLog reading processor."))); > Depending on something that is part of logical decoding to decode WAL > is not a good idea. If you want to move on with this approach, you > should have a dedicated function. Even better, it would be nice to > come up with a generic function used by both 2PC and logical decoding.
+ if (log_checkpoints && n > 0) + ereport(LOG, + (errmsg("%u two-phase state files were written " + "for long-running prepared transactions", + n))); This would be better as an independent change. That looks useful for debugging, and I guess that's why you added it. -- Michael -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers