On Sat, Dec 2, 2017 at 3:46 PM, Andres Freund <and...@anarazel.de> wrote: > Looking at 0004-Add-shared-tuplestores.patch > > Comments: > - I'd rename mutex to lock. Seems quite possible that we end up with shared > lockers too.
Done. > - Expand "Simple mechanism for sharing tuples between backends." intro > comment - that doesn't really seem like a meaningful description of > the mechanism. Should probably mention that it's similar to > tuplestores etc... Done. > - I'm still concerned about the chunking mechanism. How about this > sketch of an alternative solution? > > Chunks are always the same length. To avoid having to read the length > from disk while holding a lock, introduce continuation chunks which > store the amount of space needed by the overlarge tuple at the > start. The reading process stays largely the same, except that if a > backend reads a chunk that's a continuation, it reads the length of > the continuation and skips ahead. That could mean that more than one > backend read continuation chunks, but the window is small and there's > normally not goign to be that many huge tuples (otherwise things are > slow anyway). Done. I've also included a simple test harness that can be used to drive SharedTuplestore independently of Parallel Hash, but that patch is not for commit. See example of usage in the commit message. (Incidentally I noticed that ParallelWorkerNumber is not marked PGDLLIMPORT so that fails to build on Windows CI.) > - why are we using a separate hardcoded 32 for sts names? Why not just > go for NAMEDATALEN or such? Done. > - I'd replace most of the "should's" in comments with "need". Done. Another problem I discovered is that v27's way of installing a different function into ExecProcNode in ExecInitHashJoin() was broken: it didn't allow for the possibility that there is no DSA area available due to lack of resources. ExecInitNode() is too soon to decide. My solution is to provide a way for executor nodes to change their ExecProcNode functions at any later time, which requires a way for execProcnode.c to redo any wrapper functions. -- Thomas Munro http://www.enterprisedb.com
parallel-hash-v28.patchset.tgz
Description: GNU Zip compressed data
