Re: Patch to improve performance of replay of AccessExclusiveLock

On 17 March 2017 at 00:04, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Thu, Mar 16, 2017 at 7:22 AM, David Rowley
> <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>> On 16 March 2017 at 13:31, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>>
>>> On 8 March 2017 at 10:02, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
>>> wrote:
>>> > On 8 March 2017 at 01:13, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> >> Don't understand this. I'm talking about setting a flag on
>>> >> commit/abort WAL records, like the attached.
>>> >
>>> > There's nothing setting a flag in the attached. I only see the
>>> > checking of the flag.
>>>
>>> Yeh, it wasn't a full patch, just a way marker to the summit for you.
>>>
>>> >> We just need to track info so we can set the flag at EOXact and we're
>>> >> done.
>>> >
>>> > Do you have an idea where to store that information? I'd assume we'd
>>> > want to store something during LogAccessExclusiveLocks() and check
>>> > that in XactLogCommitRecord() and XactLogAbortRecord(). I don't see
>>> > anything existing which might help with that today. Do you think I
>>> > should introduce some new global variable for that? Or do you propose
>>> > calling GetRunningTransactionLocks() again while generating the
>>> > Commit/Abort record?
>>>
>>> Seemed easier to write it than explain further. Please see what you think.
>>
>>
>> Thanks. I had been looking for some struct to store the flag in. I'd not
>> considered just adding a new global variable.
>>
>> I was in fact just working on this again earlier, and I had come up with the
>> attached.
>>
>
> I think this will not work if you take AEL in one of the
> subtransaction and then commit the transaction. The reason is that
> you are using CurrentTransactionState to remember AEL locks and during
> commit (at the time of XactLogCommitRecord) only top level
> CurrentTransactionState will be available which is not same as
> subtransactions CurrentTransactionState. You can try with a simple
> test like below:
>
> Create table t1(c1 int);
> Begin;
> insert into t1 values(1);
> savepoint s1;
> insert into t1 values(2);
> savepoint s2;
> insert into t1 values(3);
> Lock t1 in Access Exclusive mode;
> commit;
>
> Now, we might want to store this info in top level transaction state
> to make such cases work, but I am not sure if it is better than what
> Simon has proposed in his approach. Although, I have not evaluated in
> detail, but it seems Simon's patch looks better way to solve this
> problem (In his patch, there is an explicit handling of two-phase
> commits which seems more robust).
>
> Few other comments on patch:
> 1.
> @@ -5325,6 +5341,9 @@ XactLogAbortRecord(TimestampTz abort_time,
> if (xl_xinfo.xinfo &
> XACT_XINFO_HAS_TWOPHASE)
> XLogRegisterData((char *) (&xl_twophase), sizeof
> (xl_xact_twophase));
>
> + if (CurrentTransactionState->didLogAELock)
> + xl_xinfo.xinfo |=
> XACT_XINFO_HAS_AE_LOCKS;
>
> You need to set xl_info before the below line in XactLogAbortRecord()
> to get it logged properly.
> if (xl_xinfo.xinfo != 0)
> info |= XLOG_XACT_HAS_INFO;
>
> 2.
> Explicitly Initialize didLogAELock in StartTransaction as we do for didLogXid.

Thanks for looking over this Amit. I'll drop my version of the patch
and make the changes described by Simon about moving
MyXactAccessedTempRel and MyXactAcquiredAccessExclusiveLock into a
bitmap fiags variable.

Thanks for confirming my concerns about the subxacts. I was confused
over how it was meant to work due to StandbyReleaseLockTree() having
some dead code.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services