On Fri, Nov 16, 2018 at 11:40 AM Alvaro Herrera <alvhe...@2ndquadrant.com> wrote: > On 2018-Nov-15, Amit Langote wrote: > > > Maybe name it PARTITION_INIT_ALLOCSIZE (dropping the ROUTING from it), or > > PROUTE_INIT_ALLOCSIZE, to make it clear that it's only allocation size? > > Here's a proposed delta on v17 0001. Most importantly, I noticed that > the hashed subplans stuff didn't actually work, because the hash API was > not being used correctly. So the search in the hash would never return > a hit, and we'd create RRIs for those partitions again. To fix this, I > added a new struct to hold hash entries.
I'm a bit surprised that you found that the hash table didn't work, because I remember having checked by attaching gdb that it works when I was hacking on my own delta patch, but I may have been looking at too many things. > I think this merits that the performance tests be redone. (Unless I > misunderstand, this shouldn't change the performance of INSERT, only > that of UPDATE.) Actually, I don't remember seeing performance tests done with UPDATEs on this thread. Since we don't needlessly scan *all* subplan result rels anymore, maybe this removes a good deal of overhead for UPDATEs that update partition key. > On the subject of the ArraySpace routines, I decided to drop them and > instead do the re-allocations on the places where they were needed. > In the original code there were two places for the partitions array, but > both did the same thing so it made sense to create a separate routine to > do it instead (ExecRememberPartitionRel), and do the allocation there. > Just out of custom I moved the palloc to appear at the same place as the > repalloc, and after doing so it became obvious that we were > over-allocating memory for the PartitionDispatchData pointer -- > allocating the size for the whole struct instead of just the pointer. > > (I renamed the "allocsize" struct members to "max", as is customary.) These changes look good to me. > I added CHECK_FOR_INTERRUPTS to the ExecFindPartition loop. It > shouldn't be useful if the code is correct, but if there are bugs it's > better to be able to interrupt infinite loops :-) Good measure. :) > I reindented the comment atop PartitionTupleRouting. The other way was > just too unwieldy. > > Let me know what you think. Regression tests still pass for me. Overall, it looks good to me. Thanks, Amit
