Re: POC: Sharing record typmods between backends

On Wed, Aug 23, 2017 at 5:46 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Committing 0003. This'll probably need further adjustment, but I think
> it's good to make progress here.

Thanks!

> Changes:
> - pgindent'ed after adding the necessary typedefs to typedefs.list
> - replaced INT64CONST w UINT64CONST
> - moved count assertion in delete_item to before decrementing - as count
> is unsigned, it'd just wrap around on underflow not triggering the assertion.
> - documented and asserted resize is called without partition lock held
> - removed reference to iterator in dshash_find comments
> - removed stray references to dshash_release (rather than dshash_release_lock)
> - reworded dshash_find_or_insert reference to dshash_find to also
> mention error handling.

Doh. Thanks.

> Notes for possible followup commits of the dshash API:
> - nontrivial portions of dsahash are essentially critical sections lest
> dynamic shared memory is leaked. Should we, short term, introduce
> actual critical section markers to make that more obvious? Should we,
> longer term, make this more failsafe / easier to use, by
> extending/emulating memory contexts for dsa memory?

Hmm. I will look into this.

> - I'm very unconvinced of supporting both {compare,hash}_arg_function
> and the non-arg version. Why not solely support the _arg_ version, but
> add the size argument? On all relevant platforms that should still be
> register arg callable, and the branch isn't free either.

Well, the idea was that both versions were compatible with existing
functions: one with DynaHash's hash and compare functions and the
other with qsort_arg's compare function type. In the attached version
I've done as you suggested in 0001. Since I guess many users will
finish up wanting raw memory compare and hash I've provided
dshash_memcmp() and dshash_memhash(). Thoughts?

Since there is no attempt to be compatible with anything else, I was
slightly tempted to make equal functions return true for a match,
rather than the memcmp-style return value but figured it was still
better to be consistent.

> - might be worthwhile to try to reduce duplication between
> delete_item_from_bucket, delete_key_from_bucket, delete_item
> dshash_delete_key.

Yeah. I will try this and send a separate refactoring patch.

> For later commits in the series:
> - Afaict the whole shared tupledesc stuff, as tqueue.c before, is
> entirely untested. This baffles me. See also [1]. I can force the code
> to be reached with force_parallel_mode=regress/1, but this absolutely
> really totally needs to be reached by the default tests. Robert?

A fair point. 0002 is a simple patch to push some blessed records
through a TupleQueue in select_parallel.sql. It doesn't do ranges and
arrays (special cases in the tqueue.c code that 0004 rips out), but
for exercising the new shared code I believe this is enough. If you
apply just 0002 and 0004 then this test fails with a strange confused
record decoding error as expected.

> - gcc wants static before const (0004).

Fixed.

> - Afaict GetSessionDsmHandle() uses the current rather than
> TopMemoryContext. Try running the regression tests under
> force_parallel_mode - crashes immediately for me without fixing that.

Gah, right. Fixed.

> - SharedRecordTypmodRegistryInit() is called from GetSessionDsmHandle()
> which calls EnsureCurrentSession(), but
> SharedRecordTypmodRegistryInit() does so again - sprinkling those
> around liberally seems like it could hide bugs.

Yeah. Will look into this.

--
Thomas Munro
http://www.enterprisedb.com