On Fri, Nov 16, 2018 at 11:40 AM Alvaro Herrera
<alvherre@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