Re: Reducing opr_sanity test's runtime under CLOBBER_CACHE_ALWAYS

Hi,

On 2021-05-10 16:17:18 -0400, Tom Lane wrote:
> I noticed that we already have counters that can tell whether a
> catcache or dynahash table is empty, so I experimented with the
> attached patch. Testing one of the slow queries from privileges.sql
> (which might not be very representative of the overall issue),
> I see:
>
> HEAD:
> Time: 536429.715 ms (08:56.430)
>
> with ResetCatalogCache hack:
> Time: 488938.597 ms (08:08.939)
>
> plus hash_seq_search hack:
> Time: 475400.634 ms (07:55.401)

Oh, nice.

Perhaps we generally ought to lower the initial sycache sizes further?
20cb18db4668 did that, but we still have things like PROCNAMEARGNSP,
PROCOID, RELNAMENSP, RELOID, STATRELATTINH, ... using 128 as the initial
size. Not hard to imagine that some of these are larger than what simple
workloads or CCA encounter.

> Of course, the issue with these patches is that they change these
> counters from things that (I think) we only trust for statistical
> purposes into things that had better be right or you're going to
> have impossible-to-track-down bugs with sometimes failing to
> invalidate cache entries. My gut feeling is that the risk-to-reward
> ratio is worth it for changing ResetCatalogCache, but not for
> hash_seq_search. This is partly because of the greater absolute
> payback and partly because ResetCatalogCache doesn't deal with
> shared data structures, reducing the risk of counting issues.

That sounds reasonable. We could mitigate the risk for dynahash by
testing HASH_SHARED_MEM (which we don't store right now), but it's not
clear it's worth it here. But I wonder if there's other cases where it
could help? If we did make the check support shared memory *and*
partitioned tables, I could easily see it be a win for things like
LockReleaseAll().

Greetings,

Andres Freund