Re: Making "COPY partitioned_table FROM" faster

On Fri, Jul 20, 2018 at 7:57 AM, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
wrote:

> > So, overall, we feel that the code from lines 2689 until 2691 and from
> 2740 to 2766 could use further clarification with regard to switching from
> parent to child partition and sibling to sibling partition as well as
> regarding saving relinfo for partitions that have not been seen before in
> the proute->partitions[]
>
> I hope it's more clear now that I've got rid of saved_resultRelInfo.
>
> I think all of the refactoring and clarifications look good to me and are
much more clear.

One small additional typo I noticed:

In the patched code on line 2555, there appears to be a typo:
/* ...
* inserting into and act differently if the tuples that have already
* been processed any prepared for insertion are not there.
*/
The word "any" here is probably wrong, though I am actually not sure what
was meant.

Though it is from the existing comments, it would be nice to spell out once
what is meant by "BR trigger". I'm sure it is mentioned elsewhere in the
code, but, since it is not easily googled, it might be helpful to specify.

Many thanks to both of you for the thorough review. I hope the
> attached addresses all the concerns.
>

I feel that the code itself is clear and feel our concerns are addressed.

>
> One final note: I'm not entirely convinced we need this adaptive
> code, but it seems easy enough to rip it back out if it's more trouble
> than it's worth. But if the other option is a GUC, then I'd rather
> stick with the adaptive code, it's likely going to do much better than
> a GUC since it can change itself during the copy which will be useful
> when the stream contains a mix small and large sets of consecutive
> tuples which belong to the same partition.
>
> Though the v2 numbers do look better, it doesn't complete alleviate the
slow-down in the worst case. Perhaps the GUC and the adaptive code are not
alternatives and could instead be used together. You could even make the
actual RECHECK_MULTI_INSERT_THRESHOLD the GUC.
However, I think that the decision as to whether or not it makes sense to
do the adaptive code without a GUC is really based on the average use case,
to which I can't really speak.
The counter-argument I see, however, is that it is not acceptable to have
completely un-optimized insertion of data into partition tables and that an
overwhelming flood of GUCs is undesirable.