Re: StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

On Wed, Apr 26, 2017 at 4:53 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 25 April 2017 at 16:28, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Simon Riggs <simon(at)2ndquadrant(dot)com> writes:
>>> I can't see any reason now why overwriteOK should exist at all. I'm
>>> guessing that the whole "overwriteOK" idea was an incorrect response
>>> to xids appearing where they shouldn't have done because of the
>>> mistake you just corrected. So I will now remove the parameter from
>>> the call.
>>
>> Seems reasonable, but I don't like the logic change you made in
>> SubTransSetParent; you broke the former invariant, for non-Assert
>> builds, that the target pg_subtrans entry is guaranteed to have
>> the correct value on exit. I do like fixing it to not dirty the
>> page unnecessarily, but I'd suggest that we write it like
>>
>> if (*ptr != parent)
>> {
>> Assert(*ptr == InvalidTransactionId);
>> *ptr = parent;
>> SubTransCtl->shared->page_dirty[slotno] = true;
>> }
>
> OK, thanks. I'll commit that tomorrow.

A couple of comments about the last patch of this thread.

Nit: there is a complain about a trailing whitespace:
$ git diff --check
src/backend/access/transam/twophase.c:2011: trailing whitespace.
+ bool fromdisk,

+ * It's possible that SubTransSetParent has been set before, if
+ * the prepared transaction generated xid assignment records.
*/
for (i = 0; i < hdr->nsubxacts; i++)
- SubTransSetParent(subxids[i], xid, true);
+ SubTransSetParent(subxids[i], xid);
You can remove this part in RecoverPreparedTransactions() and just
switch ProcessTwoPhaseBuffer()'s setParent to true to do the same
thing. The existing comment block should be moved before
ProcessTwoPhaseBuffer() call. It is critical to mention that
pg_subtrans is not kept after a restart.
--
Michael