On 03/30/2016 09:19 AM, Stas Kelvich wrote:
>> > +++ b/src/test/recovery/t/006_twophase.pl
>> > @@ -0,0 +1,226 @@
>> > +# Checks for recovery_min_apply_delay
>> > +use strict;
>> > This description is wrong, this file has been copied from 005.
>>
>> Yep, done.
>>
>> >
>> > +my $node_master = get_new_node("Candie");
>> > +my $node_slave = get_new_node('Django');
>> > Please let's use a node names that are more descriptive.
>>
>> Hm, it’s hard to create descriptive names because test changes master/slave
>> roles for that nodes several times during test. It’s possible to call them
>> “node1” and “node2” but I’m not sure that improves something. But anyway I’m not
>> insisting on that particular names and will agree with any reasonable suggestion.
>>
>> >
>> > +# Switch to synchronous replication
>> > +$node_master->append_conf('postgresql.conf', qq(
>> > +synchronous_standby_names = '*'
>> > +));
>> > +$node_master->restart;
>> > Reloading would be fine.
>>
>> Good catch, done.
>>
>> >
>> > + /* During replay that lock isn't really necessary, but let's take
>> > it anyway */
>> > + LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
>> > + for (i = 0; i < TwoPhaseState->numPrepXacts; i++)
>> > + {
>> > + gxact = TwoPhaseState->prepXacts[i];
>> > + proc = &ProcGlobal->allProcs[gxact->pgprocno];
>> > + pgxact = &ProcGlobal->allPgXact[gxact->pgprocno];
>> > +
>> > + if (TransactionIdEquals(xid, pgxact->xid))
>> > + {
>> > + gxact->locking_backend = MyBackendId;
>> > + MyLockedGxact = gxact;
>> > + break;
>> > + }
>> > + }
>> > + LWLockRelease(TwoPhaseStateLock);
>> > Not taking ProcArrayLock here?
>>
>> All accesses to 2pc dummies in ProcArray are covered with TwoPhaseStateLock, so
>> I thick that’s safe. Also I’ve deleted comment above that block, probably it’s
>> more confusing than descriptive.
>>
>> >
>> > The comment at the top of XlogReadTwoPhaseData is incorrect.
>>
>> Yep, fixed.
>>
>> >
>> > RecoverPreparedFromXLOG and RecoverPreparedFromFiles have a lot of
>> > code in common, having this duplication is not good, and you could
>> > simplify your patch.
>>
>> I reworked patch to avoid duplicated code between
>> RecoverPreparedFromXLOG/RecoverPreparedFromFiles and also
>> between FinishPreparedTransaction/XlogRedoFinishPrepared.
>>
>>
Patch applies with hunks, and test cases are passing.
Best regards, Jesper