Re: [HACKERS] GSOC'17 project introduction: Parallel COPY executionwith errors handling - Mailing list pgsql-hackers

From Alexey Kondratov
Subject Re: [HACKERS] GSOC'17 project introduction: Parallel COPY executionwith errors handling
Date
Msg-id D31F460B-317C-405F-B591-BE752CE8D90E@gmail.com
Whole thread Raw
In response to Re: [HACKERS] GSOC'17 project introduction: Parallel COPY executionwith errors handling  (Peter Geoghegan <pg@bowt.ie>)
Responses Re: [HACKERS] GSOC'17 project introduction: Parallel COPY executionwith errors handling  (Alex K <kondratov.aleksey@gmail.com>)
List pgsql-hackers
Sorry for a previous email, I have accidentally sent it unfinished.

On 13 Jun 2017, at 01:44, Peter Geoghegan <pg@bowt.ie> wrote:

Speculative insertion has the following special entry points to
heapam.c and execIndexing.c, currently only called within
nodeModifyTable.c

Offhand, it doesn't seem like it would be that hard to teach another
heap_insert() caller the same tricks.

I went through the nodeModifyTable.c code and it seems not to be so 
difficult to do the same inside COPY.

My sense is that it's going to be hard to sell a committer on any
design that consumes subtransactions in a way that's not fairly
obvious to the user, and doesn't have a pretty easily understood worse
case. 

Yes, and worse case probably will be a quite frequent case, since it is not 
possible to do heap_multi_insert, when BEFORE/INSTEAD triggers or partitioning 
exist (according to the current copy.c code). Thus, it will frequently fall back 
into a single heap_insert, each being wrapped with subtransaction will 
consume XIDs too greedy and seriously affect performance. I like my initial 
idea less and less.

By the way, is it possible to use heap_multi_insert with speculative insertion too?

I haven't thought about this very carefully, but I guess you could do
something like passing a flag to ExecConstraints() that indicates
"don't throw an error; instead, just return false so I know not to
proceed"

Currently, ExecConstraints always throws an error and I do not think, that 
it would be wise from my side to modify its behaviour.

I have updated my patch (rebased over the topmost master commit 
94da2a6a9a05776953524424a3d8079e54bc5d94). Please, find patch 
file attached or always up to date version on GitHub 

It catches all major errors in the input data:

1) Rows with less/extra columns cause WARNINGs and are skipped

2) I found that input type format errors are thrown from the 
InputFunctionCall; and wrapped it up with PG_TRY/CATCH. 

I am not sure that it is 100% transactionally safe, but it seems so, 
since all these errors are handled before this point
where current COPY implementation has a mechanism to skip tuple. 
I use the same skip_tuple flag.

Patch passes all regression tests, excepting a few tests due to the slightly 
changed error message texts.


Now, I think that it may be a good idea to separate all possible errors 
into two groups:
– Malformed input data
– DB conflicts during insertion

First is solved (I hope) well with the current patch. I can add, e.g. 
MAXERRORS flag to COPY, which will limit number of errors.

Second may be solved with speculative insertion using the same 
syntax ON CONFLICT DO as in INSERT statement.

Following this way, we do not use subtransactions at all; and keeping 
predictable and consistent behaviour of INSERT and COPY along the 
database. For me it sounds much better, than just swallowing all errors 
without a difference and any logic.


Alexey

Attachment

pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Next
From: Bruce Momjian
Date:
Subject: Re: [HACKERS] Preliminary results for proposed new pgindentimplementation