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 <and...@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.

- 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

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.



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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to