Re: [HACKERS] POC: Sharing record typmods between backends - Mailing list pgsql-hackers

On Wed, Aug 23, 2017 at 5:46 PM, Andres Freund <andres@anarazel.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

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