David Rowley <david.rowley@2ndquadrant.com> writes:
> Here's a more polished version with the debug code removed, complete
> with benchmarks.
A few gripes:
You're measuring the number of locks held at completion of the
transaction, which fails to account for locks transiently taken and
released, so that the actual peak usage will be more. I think we mostly
only do that for system catalog accesses, so *maybe* the number of extra
locks involved isn't very large, but that's a very shaky assumption.
I don't especially like the fact that this seems to have a hard-wired
(and undocumented) assumption that buckets == entries, ie that the
fillfactor of the table is set at 1.0. lock.c has no business knowing
that. Perhaps instead of returning the raw bucket count, you could have
dynahash.c return bucket count times fillfactor, so that the number is in
the same units as the entry count.
This:
running_avg_locks -= running_avg_locks / 10.0;
running_avg_locks += numLocksHeld / 10.0;
seems like a weird way to do the calculation. Personally I'd write
running_avg_locks += (numLocksHeld - running_avg_locks) / 10.0;
which is more the way I'm used to seeing exponential moving averages
computed --- at least, it seems clearer to me why this will converge
towards the average value of numLocksHeld over time. It also makes
it clear that it wouldn't be sane to use two different divisors,
whereas your formulation makes it look like maybe they could be
set independently.
Your compiler might not complain that LockReleaseAll's numLocksHeld
is potentially uninitialized, but other people's compilers will.
On the whole, I don't especially like this approach, because of the
confusion between peak lock count and end-of-xact lock count. That
seems way too likely to cause problems.
regards, tom lane
