COPY FROM WITH HEADER skips a tuple every 4 billion tuples
I'd been looking over the COPY FROM code tonight when I saw something
pretty scary looking:
/* on input just throw the header line away */
if (cstate->cur_lineno == 0 && cstate->header_line)
{
cstate->cur_lineno++;
if (CopyReadLine(cstate))
return false; /* done */
}
while it might not look too scary by itself, it gets a bit more so
when you learn that the cur_lineno is only 32 bits wide. This will
result in skipping a tuple every time the 32-bit variable wraps back
around to 0 again.
Maybe when this code was written copying > 4 billion rows was just a
far-off dream, but with today's hardware, it really didn't take that
long to see this actually happen for real.
postgres=# create unlogged table t(a int);
CREATE TABLE
Time: 1.339 ms
postgres=# insert into t select 0 from generate_series(1, 4300000000);
INSERT 0 4300000000
Time: 2128367.019 ms (35:28.367)
postgres=# copy t to '/home/ubuntu/t.csv' with (format csv, header);
COPY 4300000000
Time: 2294331.494 ms (38:14.331)
postgres=# truncate t;
TRUNCATE TABLE
Time: 30.367 ms
postgres=# copy t from '/home/ubuntu/t.csv' with (format csv, header);
COPY 4299999999
Time: 2693186.475 ms (44:53.186)
A patch to fix is attached.
(I just made the variable 64-bit)
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
0001-Fix-COPY-FROM-not-to-skip-a-tuple-every-2-32-tuples.patchapplication/octet-stream; name=0001-Fix-COPY-FROM-not-to-skip-a-tuple-every-2-32-tuples.patchDownload
From 409ed5f55adf7d5fc070fa96d9169ac49d91f0cd Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Tue, 22 May 2018 14:23:04 +0000
Subject: [PATCH] Fix COPY FROM not to skip a tuple every 2^32 tuples
This was due to the variable tracking the line number of the copy input being
only 32 bits wide. A check was performed on this variable and an input line
was skipped when its value was 0. The code assumed that this must be the
header line, but in actual fact it could also have been any line evenly
divisible by 2^32 due to the variable wrapping back to 0 again. Only COPY FROM
WITH HEADER is affected. No skipping was performed unless the header option
was specified.
The fix is to widen the type tracking the line number to 64 bits.
---
src/backend/commands/copy.c | 38 +++++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 770c75f..0a1706c 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -139,7 +139,7 @@ typedef struct CopyStateData
/* these are just for error messages, see CopyFromErrorCallback */
const char *cur_relname; /* table name for error messages */
- int cur_lineno; /* line number for error messages */
+ uint64 cur_lineno; /* line number for error messages */
const char *cur_attname; /* current att for error messages */
const char *cur_attval; /* current att value for error messages */
@@ -308,7 +308,7 @@ static void CopyFromInsertBatch(CopyState cstate, EState *estate,
ResultRelInfo *resultRelInfo, TupleTableSlot *myslot,
BulkInsertState bistate,
int nBufferedTuples, HeapTuple *bufferedTuples,
- int firstBufferedLineNo);
+ uint64 firstBufferedLineNo);
static bool CopyReadLine(CopyState cstate);
static bool CopyReadLineText(CopyState cstate);
static int CopyReadAttributesText(CopyState cstate);
@@ -2192,17 +2192,21 @@ void
CopyFromErrorCallback(void *arg)
{
CopyState cstate = (CopyState) arg;
+ char curlineno_str[32];
+
+ snprintf(curlineno_str, sizeof(curlineno_str), UINT64_FORMAT,
+ cstate->cur_lineno);
if (cstate->binary)
{
/* can't usefully display the data */
if (cstate->cur_attname)
- errcontext("COPY %s, line %d, column %s",
- cstate->cur_relname, cstate->cur_lineno,
+ errcontext("COPY %s, line %s, column %s",
+ cstate->cur_relname, curlineno_str,
cstate->cur_attname);
else
- errcontext("COPY %s, line %d",
- cstate->cur_relname, cstate->cur_lineno);
+ errcontext("COPY %s, line %s",
+ cstate->cur_relname, curlineno_str);
}
else
{
@@ -2212,16 +2216,16 @@ CopyFromErrorCallback(void *arg)
char *attval;
attval = limit_printout_length(cstate->cur_attval);
- errcontext("COPY %s, line %d, column %s: \"%s\"",
- cstate->cur_relname, cstate->cur_lineno,
+ errcontext("COPY %s, line %s, column %s: \"%s\"",
+ cstate->cur_relname, curlineno_str,
cstate->cur_attname, attval);
pfree(attval);
}
else if (cstate->cur_attname)
{
/* error is relevant to a particular column, value is NULL */
- errcontext("COPY %s, line %d, column %s: null input",
- cstate->cur_relname, cstate->cur_lineno,
+ errcontext("COPY %s, line %s, column %s: null input",
+ cstate->cur_relname, curlineno_str,
cstate->cur_attname);
}
else
@@ -2242,14 +2246,14 @@ CopyFromErrorCallback(void *arg)
char *lineval;
lineval = limit_printout_length(cstate->line_buf.data);
- errcontext("COPY %s, line %d: \"%s\"",
- cstate->cur_relname, cstate->cur_lineno, lineval);
+ errcontext("COPY %s, line %s: \"%s\"",
+ cstate->cur_relname, curlineno_str, lineval);
pfree(lineval);
}
else
{
- errcontext("COPY %s, line %d",
- cstate->cur_relname, cstate->cur_lineno);
+ errcontext("COPY %s, line %s",
+ cstate->cur_relname, curlineno_str);
}
}
}
@@ -2320,7 +2324,7 @@ CopyFrom(CopyState cstate)
#define MAX_BUFFERED_TUPLES 1000
HeapTuple *bufferedTuples = NULL; /* initialize to silence warning */
Size bufferedTuplesSize = 0;
- int firstBufferedLineNo = 0;
+ uint64 firstBufferedLineNo = 0;
Assert(cstate->rel);
@@ -2903,11 +2907,11 @@ CopyFromInsertBatch(CopyState cstate, EState *estate, CommandId mycid,
int hi_options, ResultRelInfo *resultRelInfo,
TupleTableSlot *myslot, BulkInsertState bistate,
int nBufferedTuples, HeapTuple *bufferedTuples,
- int firstBufferedLineNo)
+ uint64 firstBufferedLineNo)
{
MemoryContext oldcontext;
int i;
- int save_cur_lineno;
+ uint64 save_cur_lineno;
/*
* Print error context information correctly, if one of the operations
--
2.7.4
David Rowley <david.rowley@2ndquadrant.com> writes:
while it might not look too scary by itself, it gets a bit more so
when you learn that the cur_lineno is only 32 bits wide. This will
result in skipping a tuple every time the 32-bit variable wraps back
around to 0 again.
Hm, so why is the correct rowcount returned --- are we running
a separate counter for that purpose, and if so why?
regards, tom lane
On 2018-05-22 11:55:26 -0400, Tom Lane wrote:
David Rowley <david.rowley@2ndquadrant.com> writes:
while it might not look too scary by itself, it gets a bit more so
when you learn that the cur_lineno is only 32 bits wide. This will
result in skipping a tuple every time the 32-bit variable wraps back
around to 0 again.Hm, so why is the correct rowcount returned --- are we running
a separate counter for that purpose, and if so why?
Yes, it's a local counter in CopyFrom/CopyTo. It's probably not
entirely trivial to unify the two. The batching etc makes us modify
cur_lineno in a bit weird ways at times. It's noteworthy that the
comment for cur_lineno says: /* line number for error messages */
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2018-05-22 11:55:26 -0400, Tom Lane wrote:
Hm, so why is the correct rowcount returned --- are we running
a separate counter for that purpose, and if so why?
Yes, it's a local counter in CopyFrom/CopyTo. It's probably not
entirely trivial to unify the two. The batching etc makes us modify
cur_lineno in a bit weird ways at times.
OK, we'll just do it like David suggests then. I haven't checked the
patch in detail yet, but it seemed generally sane if we're just going
to widen the duplicate counter.
regards, tom lane
Thanks for pushing.
On 23 May 2018 at 03:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Hm, so why is the correct rowcount returned --- are we running
a separate counter for that purpose, and if so why?
I thought the output I pasted was clearly showing it not to be the
same. 4299999999 vs 4300000000.
Did I misunderstand you?
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 22/05/18 23:04, David Rowley wrote:
Thanks for pushing.
On 23 May 2018 at 03:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Hm, so why is the correct rowcount returned --- are we running
a separate counter for that purpose, and if so why?I thought the output I pasted was clearly showing it not to be the
same. 4299999999 vs 4300000000.Did I misunderstand you?
I think Tom was wondering why it isn't showing 5032703.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On 23 May 2018 at 09:16, Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
I think Tom was wondering why it isn't showing 5032703.
You'll need to explain that one. The number just looks like nonsense to me.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 2018-05-23 09:04:35 +1200, David Rowley wrote:
Thanks for pushing.
On 23 May 2018 at 03:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Hm, so why is the correct rowcount returned --- are we running
a separate counter for that purpose, and if so why?I thought the output I pasted was clearly showing it not to be the
same. 4299999999 vs 4300000000.Did I misunderstand you?
Well, the row-returned counter is obviously wide enough, otherwise
4299999999 couldn't be returned. Tom's point, as I understood it, is
that we obviously have one wide enough counter - why can't we reuse that
for the one you made wider. And it doesn't seem entirely trivial to do
so, so your patch is easier.
Greetings,
Andres Freund
On 23 May 2018 at 09:31, Andres Freund <andres@anarazel.de> wrote:
On 23 May 2018 at 03:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Hm, so why is the correct rowcount returned --- are we running
a separate counter for that purpose, and if so why?I thought the output I pasted was clearly showing it not to be the
same. 4299999999 vs 4300000000.Did I misunderstand you?
Well, the row-returned counter is obviously wide enough, otherwise
4299999999 couldn't be returned. Tom's point, as I understood it, is
that we obviously have one wide enough counter - why can't we reuse that
for the one you made wider. And it doesn't seem entirely trivial to do
so, so your patch is easier.
*moment of realisation*
Oh, this makes sense now.
They can't be the same. One tracks the line number in the COPY FROM
input, the other tracks the number of rows inserted. You'd only have
to add a BEFORE INSERT ROW trigger which blocks some rows to
understand why they need to be separate.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@anarazel.de> writes:
On 2018-05-23 09:04:35 +1200, David Rowley wrote:
I thought the output I pasted was clearly showing it not to be the
same. 4299999999 vs 4300000000.
Well, the row-returned counter is obviously wide enough, otherwise
4299999999 couldn't be returned. Tom's point, as I understood it, is
that we obviously have one wide enough counter - why can't we reuse that
for the one you made wider. And it doesn't seem entirely trivial to do
so, so your patch is easier.
Right. Obviously there was a 64-bit counter someplace, but it wasn't
being used for this purpose.
I think after looking at the code that the cur_lineno counter is
counting input *lines* whereas the other thing counts finished *rows*,
so unifying them would be a bad idea anyway.
regards, tom lane