On Tue, Jan 12, 2016 at 3:35 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Jan 12, 2016 at 4:57 AM, Simon Riggs <simon@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
