Re: Speedup twophase transactions - Mailing list pgsql-hackers

> On 06 Sep 2016, at 04:41, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> On Sat, Sep 3, 2016 at 10:26 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Fri, Sep 2, 2016 at 5:06 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> On 13 April 2016 at 15:31, Stas Kelvich <s.kelvich@postgrespro.ru> wrote:
>>>
>>>> Fixed patch attached. There already was infrastructure to skip currently
>>>> held locks during replay of standby_redo() and I’ve extended that with check for
>>>> prepared xids.
>>>
>>> Please confirm that everything still works on current HEAD for the new
>>> CF, so review can start.
>>
>> The patch does not apply cleanly. Stas, could you rebase? I am
>> switching the patch to "waiting on author" for now.
>
> So, I have just done the rebase myself and did an extra round of
> reviews of the patch. Here are couple of comments after this extra
> lookup.
>
> LockGXactByXid() is aimed to be used only in recovery, so it seems
> adapted to be to add an assertion with RecoveryInProgress(). Using
> this routine in non-recovery code paths is risky because it assumes
> that a PGXACT could be missing, which is fine in recovery as prepared
> transactions could be moved to twophase files because of a checkpoint,
> but not in normal cases. We could also add an assertion of the kind
> gxact->locking_backend == InvalidBackendId before locking the PGXACT
> but as this routine is just normally used by the startup process (in
> non-standalone mode!) that's fine without.
>
> The handling of invalidation messages and removal of relfilenodes done
> in FinishGXact can be grouped together, checking only once for
> !RecoveryInProgress().
>
> + *
> + *     The same procedure happens during replication and crash recovery.
>  *
> "during WAL replay" is more generic and applies here.
>
> +
> +next_file:
> +       continue;
> +
> That can be removed and replaced by a "continue;".
>
> +   /*
> +    * At first check prepared tx that wasn't yet moved to disk.
> +    */
> +   LWLockAcquire(TwoPhaseStateLock, LW_SHARED);
> +   for (i = 0; i < TwoPhaseState->numPrepXacts; i++)
> +   {
> +       GlobalTransaction gxact = TwoPhaseState->prepXacts[i];
> +       PGXACT *pgxact = &ProcGlobal->allPgXact[gxact->pgprocno];
> +
> +       if (TransactionIdEquals(pgxact->xid, xid))
> +       {
> +           LWLockRelease(TwoPhaseStateLock);
> +           return true;
> +       }
> +   }
> +   LWLockRelease(TwoPhaseStateLock);
> This overlaps with TwoPhaseGetGXact but I'd rather keep both things
> separated: it does not seem worth complicating the existing interface,
> and putting that in cache during recovery has no meaning.

Oh, I was preparing new version of patch, after fresh look on it. Probably, I should
said that in this topic. I’ve found a bug in sub transaction handling and now working
on fix.

>
> I have also reworked the test format, and fixed many typos and grammar
> problems in the patch as well as in the tests.

Thanks!

>
> After review the result is attached. Perhaps a committer could get a look at it?

I'll check it against my failure scenario with subtransactions and post results or updated patch here.

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