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@anarazel.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
underflownot 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,
shortterm, 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
thebranch 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
canforce the code to be reached with force_parallel_mode=regress/1, but this absolutely really totally needs to be
reachedby 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