Volkan YAZICI <yazicivo@ttnet.net.tr> writes:
> I tried to prepare a patch for these TODO items:
> - Have COPY return the number of rows loaded/unloaded?
> - Update [pg_dump and] psql to use the new COPY libpq API.
> ! cstate->raw_buf_index = cstate->raw_buf_len = 0;
> ! cstate->raw_buf_index = cstate->raw_buf_len = cstate->processed = 0;
Minor stylistic gripe: processed is unrelated to those two other
variables and not even the same datatype. It'd be better to
zero it in a separate statement.
> ! {
> ! uint64 processed = DoCopy((CopyStmt *) parsetree);
> ! char buf[21];
> !
> ! snprintf(buf, sizeof(buf), UINT64_FORMAT, processed);
> ! snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
> ! "COPY %s", buf);
> ! }
This is ugly and unnecessary. Why not
> ! {
> ! uint64 processed = DoCopy((CopyStmt *) parsetree);
> !
> ! snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
> ! "COPY " UINT64_FORMAT, processed);
> ! }
Also, I think you've broken the psql-side handling of COPY IN. There's
no check for I/O error on the input, and the test for terminator (\.\n)
mistakenly assumes that the terminator could only appear at the start
of the buffer, not to mention assuming that it couldn't span across two
bufferloads.
The check for \r is wrong too (counterexample: first input line exceeds
8K), but actually you should just dispense with that entirely, because
if you're going to use PQputCopyEnd then it is not your responsibility
to send the terminator.
But the *real* problem with this coding is that it will fetch data past
the \., which breaks files that include both COPY data and SQL. Thus
for example this patch breaks pg_dump files.
! for (c = p; isdigit((int) *c); ++c)
Wrong. Use (unsigned char).
regards, tom lane
