Michail Nikolaev <michail.nikol...@gmail.com> writes:
>> * when idle - since we have time to do it when that happens, which
>> happens often since most workloads are bursty
> I have added getting of ProcArrayLock for this case.
That seems like a fairly bad idea: it will add extra contention
on ProcArrayLock, and I see no real strong argument that the path
can't get traversed often enough for that to matter. It would
likely be better for KnownAssignedXidsCompress to obtain the lock
for itself, only after it knows there is something worth doing.
(This ties back to previous discussion: the comment claiming it's
safe to read head/tail because we have the lock is misguided.
It's safe because we're the only process that changes them.
So we can make the heuristic decision before acquiring lock.)
While you could use the "reason" code to decide whether you need
to take the lock, it might be better to add a separate boolean
argument specifying whether the caller already has the lock.
Beyond that, I don't see any issues except cosmetic ones.
> Also, I think while we still in the context, it is good to add:
> * Simon's optimization (1) for KnownAssignedXidsRemoveTree (it is
> simple and effective for some situations without any seen drawbacks)
> * Maybe my patch (2) for replacing known_assigned_xids_lck with memory
> barrier?
Doesn't seem like we have any hard evidence in favor of either of
those being worth doing. We especially haven't any evidence that
they'd still be worth doing after this patch. I'd be willing to
make the memory barrier change anyway, because that seems like
a simple change that can't hurt. I'm less enthused about the
patch at (1) because of the complexity it adds.
regards, tom lane
