Re: Speedup twophase transactions

> On Mar 29, 2016, at 6:04 PM, David Steele <david(at)pgmasters(dot)net> wrote:
>
> It looks like you should post a new patch or respond to Michael's comments. Marked as "waiting on author".
>

Yep, here it is.

> On Mar 22, 2016, at 4:20 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>
> Looking at this patch….

Thanks.

>
> +++ 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.

--
Stas Kelvich
Postgres Professional: http://www.postgrespro.com <http://www.postgrespro.com/>
Russian Postgres Company