On Mon, Aug 05, 2024 at 12:32:20AM +0500, Andrey M. Borodin wrote:
> I´ve found some dead code: BufMappingPartitionLockByIndex() is unused,
> and unused for a long time. See patch 1.
I don't see a reason to also get rid of BufTableHashPartition(), but
otherwise this looks reasonable to me. It would probably be a good idea to
first check whether there are any external callers we can find.
> I´ve prototyped similar GUC for anyone willing to do such experiments.
> See patch 2, 4. Probably, I´ll do some experiments too, on customer's
> clusters and workloads :)
Like Tomas, I'm not too wild about making this a GUC. And as Heikki
pointed out upthread, a good first step would be to benchmark different
NUM_BUFFER_PARTITIONS settings on modern hardware. I suspect the current
setting is much lower than optimal (e.g., I doubled it and saw a TPS boost
for read-only pgbench on an i5-13500T), but I don't think we fully
understand the performance characteristics with different settings yet. If
we find that the ideal setting depends heavily on workload/hardware, then
perhaps we can consider adding a GUC, but I don't think we are there yet.
>> Anyway, this value is inherently a trade off. If it wasn't, we'd set it
>> to something super high from the start. But having more partitions of
>> the lock table has a cost too, because some parts need to acquire all
>> the partition locks (and that's O(N) where N = number of partitions).
>
> I´ve found none such cases, actually. Or, perhaps, I was not looking good
> enough. pg_buffercache iterates over buffers and releases locks. See
> patch 3 to fix comments.
Yeah, I think 0003 is reasonable, too. pg_buffercache stopped acquiring
all the partition locks in v10 (see commit 6e65454), which is also the
commit that removed all remaining uses of BufMappingPartitionLockByIndex().
In fact, I think BufMappingPartitionLockByIndex() was introduced just for
this pg_buffercache use-case (see commit ea9df81).
--
nathan
