Re: POC: Sharing record typmods between backends

On 2017-08-22 16:41:23 -0700, Andres Freund wrote:
> On 2017-08-21 11:02:52 +1200, Thomas Munro wrote:
> > On Mon, Aug 21, 2017 at 6:17 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > Pushing 0001, 0002 now.
> > >
> > > - rebased after conflicts
> > > - fixed a significant number of too long lines
> > > - removed a number of now superflous linebreaks
> >
> > Thanks! Please find attached a rebased version of the rest of the patch set.
>
> Pushed 0001, 0002. Looking at later patches.

Committing 0003. This'll probably need further adjustment, but I think
it's good to make progress here.

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.

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?
- 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.
- might be worthwhile to try to reduce duplication between
delete_item_from_bucket, delete_key_from_bucket, delete_item
dshash_delete_key.

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?
- gcc wants static before const (0004).
- Afaict GetSessionDsmHandle() uses the current rather than
TopMemoryContext. Try running the regression tests under
force_parallel_mode - crashes immediately for me without fixing that.
- SharedRecordTypmodRegistryInit() is called from GetSessionDsmHandle()
which calls EnsureCurrentSession(), but
SharedRecordTypmodRegistryInit() does so again - sprinkling those
around liberally seems like it could hide bugs.

Regards,

Andres

[1] https://coverage.postgresql.org/src/backend/executor/tqueue.c.gcov.html