Re: [HACKERS] A design for amcheck heapam verification

On 2018-03-29 19:42:42 -0700, Peter Geoghegan wrote:
> >> + /*
> >> + * Aim for two bytes per element; this is sufficient to get a false
> >> + * positive rate below 1%, independent of the size of the bitset or total
> >> + * number of elements. Also, if rounding down the size of the bitset to
> >> + * the next lowest power of two turns out to be a significant drop, the
> >> + * false positive rate still won't exceed 2% in almost all cases.
> >> + */
> >> + bitset_bytes = Min(bloom_work_mem * 1024L, total_elems * 2);
> >> + /* Minimum allowable size is 1MB */
> >> + bitset_bytes = Max(1024L * 1024L, bitset_bytes);
> >
> > Some upcasting might be advisable, to avoid dangers of overflows?
>
> When it comes to sizing work_mem, using long literals to go from KB to
> bytes is How It's Done™. I actually think that's silly myself, because
> it's based on the assumption that long is wider than int, even though
> it isn't on Windows. But that's okay because we have the old work_mem
> size limits on Windows.

> What would the upcasting you have in mind look like?

Just use UINT64CONST()? Let's try not to introduce further code that'll
need to get painfully fixed.

>
> >> + /* Use 64-bit hashing to get two independent 32-bit hashes */
> >> + hash = DatumGetUInt64(hash_any_extended(elem, len, filter->seed));
> >
> > Hm. Is that smart given how some hash functions are defined? E.g. for
> > int8 the higher bits aren't really that independent for small values:
>
> Robert suggested that I do this. I don't think that we need to make it
> about the quality of the hash function that we have available. That
> really seems like a distinct question to me. It seems clear that this
> ought to be fine (or should be fine, if you prefer). I understand why
> you're asking about this, but it's not scalable to ask every user of a
> hash function to care that it might be a bit crap. Hash functions
> aren't supposed to be a bit crap.

Well, they're not supposed to be, but they are. Practical realities
matter... But I think it's moot because we don't use any of the bad
ones, I only got to that later...

> >> +DROP FUNCTION bt_index_check(regclass);
> >> +CREATE FUNCTION bt_index_check(index regclass,
> >> + heapallindexed boolean DEFAULT false)
> >> +RETURNS VOID
> >> +AS 'MODULE_PATHNAME', 'bt_index_check'
> >> +LANGUAGE C STRICT PARALLEL RESTRICTED;
> >
> > This breaks functions et al referencing the existing function.
>
> This sounds like a general argument against ever changing a function's
> signature. It's not like we haven't done that a number of times in
> extensions like pageinspect. Does it really matter?

Yes, it does. And imo we shouldn't.

> > Also, I
> > don't quite recall the rules, don't we have to drop the function from
> > the extension first?
>
> But...I did drop the function?

I mean in the ALTER EXTENSION ... DROP FUNCTION sense.

Greetings,

Andres Freund