Re: Making "COPY partitioned_table FROM" faster - Mailing list pgsql-hackers

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            not tested

This patch applied cleanly and worked as expected.

Patch description:
This patch implements the use of heap_multi_insert() for partition tables when using the COPY FROM command, benefiting
theperformance of COPY FROM in cases in which multiple tuples in the file are destined for the same partition.
 

Tests:
All tests passed make check-world and TAP tests

Functionality:
Specifically, we tried the cases in the bottom section "Exercising the code" and found all behavior as described and
expected.
Note that we did conduct performance testing for our test cases enumerated in this section using COPY FROM PROGRAM
comparingthe patched and unpatched code using one example with tuples all destined for the same partition and one
examplewith tuples destined for alternating partitions. We saw a similar performance improvement to the one recorded by
Davidin his email, including a decrease in performance for copying data with tuples alternating between destination
partitionsas compared to the unpatched code. 
 
However, in our cases below, we focus on exercising the code added as opposed to on performance.

Feature feedback from a user perspective:
There will be two differences in performance that may be perceptible to the user after the inclusion of this patch:
1) the relatively small decrease in performance (as compared to master) for copying data from a file or program in
whichthe destination partition changes frequently
 
2) the stark contrast between the performance of copying data destined for the same partition and data destined for
alternatingpartitions
 
Based on the numbers in David's email the performance difference 
27721.054 ms patched for copying data destined for the same partition vs 46151.844 ms patched for copying data destined
fortwo different partitions alternating each row
 
Because both differences could impact users in data loading, it is worth considering making this transparent to the
userin some way. Because this will be noticeable to the user, and, furthermore, because it is potentially within the
powerof the user to make changes to their data copying strategy given this information, it might be prudent to either
createa GUC allowing the user to disable heap_multi_insert for COPY FROM (if the user knows the data he/she is copying
overis very varied) or to add a comment to the docs that when using COPY FROM for a partition table, it is advisable to
sortthe data in some manner which would optimize the number of consecutive tuples destined for the same partition.
 

Code Review:
Naming of newly introduced enum:
The CopyInsertMethod enum value CIM_MULTI_PARTITION is potentially confusing, since, for a partition table with BEFORE
orINSTEAD INSERT triggers on child partitions only heap_insert is valid, requiring the additional variable
part_can_multi_insertto determine if heap_multi_insert can be used or not.
 
It might be more clear to name it something that indicates it is a candidate for multi-insertion, pending further
review.This decouples the state of potentially applicable multi-insertion from the table being a partition table. In
thefuture, perhaps, some other feature might make a table conditionally eligible for multi-insertion of tuples, so, it
maybe desirable to use this intermediate state for other kinds of tables as well.
 
Alternatively, it might make it more clear if the variable part_can_multi_insert was clearly for a leaf partition
table,since this can change from leaf to leaf, maybe named leaf_part_can_multi_insert
 

Code comment potential typo corrections:
In the following comment, in the patched code line 2550
     /* ... 
     * Note: It does not matter if any partitions have any volatile default
     * expression as we use the defaults from the target of the COPY command.
     */
It seems like "volatile default expression" should be "volatile default expressions"

In the following comment, in the patched code starting on line 2582

        /* ...
         * the same partition as the previous tuple, and since we flush the
         * previous partitions buffer once the new tuple has already been
         * built, we're unable to reset the estate since we'd free the memory
         * which the new tuple is stored.  To work around this we maintain a
         * secondary expression context and alternate between these when the
         ... */
"previous partitions" should probably be "previous partition's" and 
"since we'd free the memory which the new tuple is stored" should be "since we'd free the memory in which the new tuple
isstored"
 

In the following comment, in the patched code starting on line 2769

                /*
                 * We'd better make the bulk insert mechanism gets a new
                 * buffer when the partition being inserted into changes.
                 */
"We'd better make the bulk insert mechanism..." should be "We'd better make sure the bulk insert mechanism..."

Code comment clarification 1:
In the patched code, starting on line 2553, the addition of the new OR condition makes this if statement very difficult
toparse as a reader. It would be helpful to the reader if the comment above it was partially inlined in the if
statement--e.g.
    if (
        /* relation has either a row-level insert trigger or an insert instead of trigger */
        (resultRelInfo->ri_TrigDesc != NULL &&
         (resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
          resultRelInfo->ri_TrigDesc->trig_insert_instead_row)) ||
          /* relation is a partition table with a statement-level insert trigger */
        (proute != NULL && resultRelInfo->ri_TrigDesc != NULL &&
         resultRelInfo->ri_TrigDesc->trig_insert_new_table) ||
        /* relation is a foreign table or has volatile default expressions */ 
        resultRelInfo->ri_FdwRoutine != NULL ||
          cstate->volatile_defexprs
       )

Code comment clarification 2:
Starting with patched code line 2689 and ending on line 2766, there are several comments we found confusing:

In the following comment, in the patched code starting on line 2740, 
                /*
                 * Save the old ResultRelInfo and switch to the one
                 * corresponding to the selected partition.
                 */
upon initially reading this comment, we thought "saving the old ResultRelInfo" meant saving the relinfo of the
partitionfrom which we are switching, however, it seems like that is happening above on line 2691, and, that that
partitionfrom which we are switching is not the sibling partition but the parent partition and that we do not, in fact,
needto "switch" the resultRelInfo from one sibling to another. Related to this, on walking through the code with the
partitiontable described in the DDL below (table foo(a int) in the section "Exercising the code")
 
and copying the following data from a file 4 4 7 7 11 11 7 7 4 4, we found that the variable saved_resultRelInfo first
seton line 2689
 
    saved_resultRelInfo = resultRelInfo;
is always the parent partition rather than a sibling. Perhaps saved_resultRelInfo could be named something like
parent_resultRelInfo,if this is a correct understanding of the code.
 
Or, if this is an incorrect understanding of the code, perhaps a comment could clarify this.

In fact, on line 2744, it appears we are "saving" the information for the leaf partition for which the current tuple is
destinedinto the proute->partitions[]. It would be helpful to clarify this, perhaps with a comment to the effect of "it
thedestined partition has not already had its information initialized and saved into the proute->partitions list, do so
now"

So, overall, we feel that the code from lines 2689 until 2691 and from 2740 to 2766 could use further clarification
withregard to switching from parent to child partition and sibling to sibling partition as well as regarding saving
relinfofor partitions that have not been seen before in the proute->partitions[]
 

Exercising the code:
-- DDL to create a partition table
CREATE TABLE foo(a int) partition by range (a);
CREATE TABLE foo_prt1 partition of foo for values from (1) to (5);
CREATE TABLE foo_prt2 partition of foo for values from (5) to (10);
CREATE TABLE foo_prt_default partition of foo DEFAULT;

-- Case 1 
-- a simple partition table
-- copying a file containing the following values for foo(a) : 4 4 7 7 11 11 7 7 4 4 in file 'f.csv'
COPY foo FROM 'f.csv';
-- we see that the insertMethod value is correctly set to CIM_MULTI_PARTITION
-- the value of part_can_multi_insert is true for each of the leaf partitions
-- heap_multi_insert is called to insert data into these leaf partitions

-- Case 2
-- a partition table with a statement level insert trigger
-- DDL to create the statement level insert trigger
CREATE OR REPLACE FUNCTION parent_stmt_insert_function() RETURNS TRIGGER AS $$
BEGIN
    RETURN NEW;
END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER parent_stmt_trig
    AFTER INSERT ON foo
    REFERENCING NEW TABLE AS newtable
    FOR EACH STATEMENT execute procedure parent_stmt_insert_function();

-- copying a file containing the following values for foo(a) : 4 4 7 7 11 11 7 7 4 4 in file 'f.csv'
COPY foo FROM 'f.csv';
-- we see that the value of insertMethod is correctly set to CIM_SINGLE due to the statement level insert trigger
-- heap_insert is called to insert data into these leaf partitions

-- Case 3
-- a partition table with a row-level before insert trigger on exactly one child partition out of 3
-- DDL to create the row level before insert trigger on one child partition which executes a function that inserts data
intodefault partition
 
DROP FUNCTION IF EXISTS parent_stmt_insert_function CASCADE;
CREATE OR REPLACE FUNCTION child_row_insert_function() RETURNS TRIGGER AS $$
BEGIN
    RETURN NEW;
END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER child_row_trig
  BEFORE INSERT ON foo_prt1
  FOR EACH ROW EXECUTE PROCEDURE child_row_insert_function();
-- copying a file containing the following values for foo(a) : 4 4 7 7 11 11 7 7 4 4 in file 'f.csv'
COPY foo FROM 'f.csv';
-- we see that the insertMethod value is set to CIM_MULTI_PARTITION
-- the value of part_can_multi_insert is false for foo_prt1, true for foo_prt2, and true for foo_prt_default
-- heap_multi_insert is called for foo_prt2 and foo_prt_default (for the rows inserted by COPY)
-- heap_insert is called for foo_prt1
-- heap_insert is called for the rows inserted by our row-level trigger into foo_prt_default


-- Case 4
-- a partition table with a row-level after insert trigger on the root partition
-- DDL to create the row-level after insert trigger on the root
DROP FUNCTION IF EXISTS parent_stmt_insert_function CASCADE;
DROP FUNCTION IF EXISTS child_row_insert_function CASCADE;
CREATE OR REPLACE FUNCTION parent_row_insert_function() RETURNS TRIGGER AS $$
BEGIN
    RETURN NEW;
END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER parent_row_trig
    AFTER INSERT ON foo
    FOR EACH ROW execute procedure parent_row_insert_function();
-- copying a file containing the following values for foo(a) : 4 4 7 7 11 11 7 7 4 4 in file 'f.csv'
COPY foo FROM 'f.csv';
-- we see that the insertMethod is CIM_MULTI_PARTITION
-- foo_prt1, foo_prt2, and foo_prt_default have part_can_multi_insert as true

Thanks,
Melanie & Karen

The new status of this patch is: Waiting on Author