On Tue, Mar 29, 2016 at 5:47 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Wed, Mar 30, 2016 at 6:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Mar 29, 2016 at 3:17 AM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> OK, so I am switching this patch as "Ready for committer", for 0001.
>>> It is in better shape now.
>>
>> Well... I have a few questions yet.
>>
>> The new argument to SyncRepWaitForLSN is called "bool commit", but
>> RecordTransactionAbortPrepared passes true. Either it should be
>> passing false, or the parameter is misnamed or at the least in need of
>> a better comment.
>>
>> I don't understand why this patch is touching the abort paths at all.
>> XactLogAbortRecord sets XACT_COMPLETION_SYNC_APPLY_FEEDBACK, and
>> xact_redo_abort honors it. But surely it makes no sense to wait for
>> an abort to become visible.
>
> You're right, that was totally unnecessary. Here is a version that
> removes that (ie XactLogAbortRecord doesn't request apply feedback
> from the standby, xact_redo_abort doesn't send apply feedback to the
> primary and RecordTransactionAbortPrepared now passes false to
> SyncRepWaitForLSN so it doesn't wait for apply feedback from the
> standby). Also I fixed a silly bug in SyncRepWaitForLSN when capping
> the mode. I have also renamed XACT_COMPLETION_SYNC_APPLY_FEEDBACK to
> the more general XACT_COMPLETION_APPLY_FEEDBACK, because the later
> 0004 patch will use it for a more general purpose than
> synchronous_commit.
OK, I committed this, with a few tweaks. In particular, I added a
flag variable instead of relying on "latch set" == "need to send
reply"; the other changes were cosmetic.
I'm not sure how much more of this we can realistically get into 9.6;
the latter patches haven't had much review yet. But I'll set this
back to Needs Review in the CommitFest and we'll see where we end up.
But even if we don't get anything more than this, it's still rather
nice: remote_apply turns out to be only slightly slower than remote
flush, and it's a guarantee that a lot of people are looking for.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
