Re: Speedup twophase transactions

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