On Wed, Dec 2, 2015 at 8:59 PM, Robert Haas <
robertmhaas@gmail.com> wrote:
>
>
> I think the approach this patch takes is pretty darned strange, and
> almost certainly not what we want. What you're doing here is putting
> every outstanding CLOG-update request into a linked list, and then the
> leader goes and does all of those CLOG updates. But there's no
> guarantee that the pages that need to be updated are even present in a
> CLOG buffer. If it turns out that all of the batched CLOG updates are
> part of resident pages, then this is going to work great, just like
> the similar ProcArrayLock optimization. But if the pages are not
> resident, then you will get WORSE concurrency and SLOWER performance
> than the status quo. The leader will sit there and read every page
> that is needed, and to do that it will repeatedly release and
> reacquire CLogControlLock (inside SimpleLruReadPage). If you didn't
> have a leader, the reads of all those pages could happen at the same
> time, but with this design, they get serialized. That's not good.
>
I think the way to address is don't add backend to Group list if it is
not intended to update the same page as Group leader. For transactions
to be on different pages, they have to be 32768 transactionid's far apart
and I don't see much possibility of that happening for concurrent
transactions that are going to be grouped.
> My idea for how this could possibly work is that you could have a list
> of waiting backends for each SLRU buffer page.
>
Won't this mean that first we need to ensure that page exists in one of
the buffers and once we have page in SLRU buffer, we can form the
list and ensure that before eviction, the list must be processed?
If my understanding is right, then for this to work we need to probably
acquire CLogControlLock in Shared mode in addition to acquiring it
in Exclusive mode for updating the status on page and performing
pending updates for other backends.
>
> I agree with Simon that it's probably a good idea for this
> optimization to handle cases where a backend has a non-overflowed list
> of subtransactions. That seems doable.
>
Agreed and I have already handled it in the last version of patch posted
by me.