> On 06 Sep 2016, at 04:41, Michael Paquier <michael.paqu...@gmail.com> wrote:
>
> On Sat, Sep 3, 2016 at 10:26 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
>> On Fri, Sep 2, 2016 at 5:06 AM, Simon Riggs <si...@2ndquadrant.com> wrote:
>>> On 13 April 2016 at 15:31, Stas Kelvich <s.kelv...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
