Re: Speed up transaction completion faster after many relations are accessed in a transaction

Hi,

On 2019-04-05 23:03:11 -0400, Tom Lane wrote:
> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> > I can't detect any performance improvement with the patch applied to
> > current master, using the test case from Yoshikazu Imai (2019-03-19).
>
> FWIW, I tried this patch against current HEAD (959d00e9d).
> Using the test case described by Amit at
> <be25cadf-982e-3f01-88b4-443a6667e16a(at)lab(dot)ntt(dot)co(dot)jp>
> I do measure an undeniable speedup, close to 35%.
>
> However ... I submit that that's a mighty extreme test case.
> (I had to increase max_locks_per_transaction to get it to run
> at all.) We should not be using that sort of edge case to drive
> performance optimization choices.
>
> If I reduce the number of partitions in Amit's example from 8192
> to something more real-world, like 128, I do still measure a
> performance gain, but it's ~ 1.5% which is below what I'd consider
> a reproducible win. I'm accustomed to seeing changes up to 2%
> in narrow benchmarks like this one, even when "nothing changes"
> except unrelated code.

I'm not sure it's actually that narrow these days. With all the
partitioning improvements happening, the numbers of locks commonly held
are going to rise. And while 8192 partitions is maybe on the more
extreme side, it's a workload with only a single table, and plenty
workloads touch more than a single partitioned table.

> Trying a standard pgbench test case (pgbench -M prepared -S with
> one client and an -s 10 database), it seems that the patch is about
> 0.5% slower than HEAD. Again, that's below the noise threshold,
> but it's not promising for the net effects of this patch on workloads
> that aren't specifically about large and prunable partition sets.

Yea, that's concerning.

> I'm also fairly concerned about the effects of the patch on
> sizeof(LOCALLOCK) --- on a 64-bit machine it goes from 72 to 88
> bytes, a 22% increase. That's a lot if you're considering cases
> with many locks.

I'm not sure I'm quite that concerned. For one, a good bit of that space
was up for grabs until the recent reordering of LOCALLOCK and nobody
complained. But more importantly, I think commonly the amount of locks
around is fairly constrained, isn't it? We can't really have that many
concurrently held locks, due to the shared memory space, and the size of
a LOCALLOCK isn't that big compared to say relcache entries. We also
probably fairly easily could win some space back - e.g. make nLocks 32
bits.

I wonder if one approach to solve this wouldn't be to just make the
hashtable drastically smaller. Right now we'll often have have lots
empty entries that are 72 bytes + dynahash overhead. That's plenty of
memory that needs to be skipped over. I wonder if we instead should
have an array of held locallocks, and a hashtable with {hashcode,
offset_in_array} + custom comparator for lookups. That'd mean we could
either scan the array of locallocks at release (which'd need to skip
over entries that have already been released), or we could scan the much
smaller hashtable sequentially.

I don't think the above idea is quite there, and I'm tired, but I
thought it might still be worth bringing up.

> I spent some time wondering whether we could adjust the data structure
> so that all the live entries in a hashtable are linked into one chain,
> but I don't quite see how to do it without adding another list link to
> struct HASHELEMENT, which seems pretty expensive.

Yea :(

Greetings,

Andres Freund