On 22 March 2017 at 22:27, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 20 March 2017 at 08:31, David Rowley <david.rowley@2ndquadrant.com> wrote:
>> 0003:
>>
>> Is intended to be patched atop of 0002 (for master only) and revises
>> this code further to remove the StandbyReleaseLockTree() function. To
>> me it seems better to do this to clear up any confusion about what's
>> going on here. This patch does close the door a little on tracking
>> AELs on sub-xacts. I really think if we're going to do that, then it
>> won't be that hard to put it back again. Seems better to be consistent
>> here and not leave any code around that causes people to be confused
>> about the same thing as I was confused about. This patch does get rid
>> of StandbyReleaseLockTree() which isn't static. Are we likely to break
>> any 3rd party code by doing that?
>
> We've solved the performance problem due to your insight, so ISTM ok
> now to enable AELs on subxids, since not having them causes locks to
> be held for too long, which is a problem also. Rearranging the code
> doesn't gain us any further performance, but as you say it prevents
> prevents us solving the AELs on subxids problem.
>
> Given that, do you agree to me applying assign_aels_against_subxids.v1.patch
> as well?
Does applying assign_aels_against_subxids.v1.patch still need to keep
the loop to release the subxacts? Won't this be gone already with the
subxact commit/abort record replays?
This does possibly mean that we perform more loops over the
RecoveryLockList even if the subxact does not have an AELs, but its
parent xact does. Wonder if this is a good price to pay for releasing
the locks earlier?
> We can discuss any backpatches after the CF is done.
>
>> 0004:
>>
>> Again master only, is a further small optimization and of
>> StandbyReleaseLocks() to further tighten the slow loop over each held
>> lock. I also think that it's better to be more explicit about the case
>> of when the function would release all locks. Although I'm not all
>> that sure in which case the function will actually be called with an
>> invalid xid.
>>
>> Patch series is attached.
>
> This does improve things a little more, but we already hit 95% of the
> gain, so I'd prefer to keep the code as similar as possible.
Seems fair. Its just a blemish that I saw every time I looked at the
code and was inspired not to see it anymore. I agree that there won't
be much gain from it.
-- David Rowley http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services