Re: Patch to improve performance of replay of AccessExclusiveLock

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.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com