Re: Restructured Shared Buffer Hash Table

Hi Dhruv,

I am formatting your patch, and doing a few minor adjustments

It addresses Andres Freund's feedback
> FWIW, on machines with the necessary hardware support, postgres'
instr_time.h
> should now be quite fast, it's using rdtsc[p] if available.

This version uses instr_time and a a little bit of pre-processing to write
the
benchmark as

+ BEGIN_TIMING("insert", n)
+ int32 j = w_ord[i];
+ BufTableInsert(&ptag[j], phash[j], bufids[j]);
+ END_TIMING
+
+ BEGIN_TIMING("hit", n)
+ int32 j = r_ord[i];
+ sink += BufTableLookup(&ptag[j], phash[j]);
+ END_TIMING

I also increased the number of buckets to make 1/3 < occupation < 2/3
instead of 1/2 < occupation < 1. 100% occupation could produce longer
chains (not critical though, as it is not using open addressing).

> I don't really understand the race condition this is trying to address:
>
>> + /* Unlock buffer header after the entry is deleted to avoid a race
condition:
>> + * If unlocked prior, a concurrent GetVictimBuffer() could insert
a new entry
>> + * for the same buffer and overwrite the entry slot. Then, the
BufTableDelete()
>> + * would be unable to find the entry and would corrupt the
hashtable. */
>> + UnlockBufHdrExt(buf, buf_state,
>> + 0,
>> + BUF_FLAG_MASK | BUF_USAGECOUNT_MASK,
>> + 0);
>
>How could there be a concurrent insertion while the buffer partition lock
is
>held?

I am removing that part as there is a consensus among the big guys that
we shouldn't hold a spin-lock while doing the BufferTableDelete.

I think I could explain why this is necessary, but I want to see your
argument.
Was it safe before? if so, what is the property of dynhash that we lost on
this
patch.

I could argue that BufferTableDelete is safe if we check for the bounds of
.next and make sure there are no cycles in the chain. But again I will let
you elaborate that argument.

Regards,
Alexandre