Re: [HACKERS] 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  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