tuple count and v3 functions in psql for COPY

Volkan YAZICI wrote:

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.

I'll apply this today or tomorrow, barring any objections.

BTW, if we're using an int64 counter for the # of rows modified by COPY,
I wonder if it's also worth converting INSERT's rowcount to use an
int64. Any objections to doing that?

-Neil

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

On Dec 22 01:52, Tom Lane wrote:

! {
! 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);
! }

At the beginning I used the style as above, but after looking at the
source code for INT64_FORMAT usage, saw that nearly all of them use
a seperate buffer.

Furthermore, in the source code (for instance
backend/commands/sequence.c) nearly every buffer size used for
INT64_FORMAT is 100. AFAIK, 20+1 is enough for a 64 bit integer.

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.

8K limit is caused by a mis-understanding of me. Thanks so much for the
review, I'll try to fix above missing parts ASAP.

! for (c = p; isdigit((int) *c); ++c)

Wrong. Use (unsigned char).

I found above from "The Open Group Base Specifications Issue 6". I'll
fix that too.

Regards.

--
"We are the middle children of history, raised by television to believe
that someday we'll be millionaires and movie stars and rock stars, but
we won't. And we're just learning this fact," Tyler said. "So don't
fuck with us."

Here's a new try. This one touches to pg_dump side too - for v3
COPY functions usage instead of deprecated ones.

I'd be so appreciated if somebody can review attached patch.

Regards.

I've just realized an exception in the sending data while using
PQsendCopyData(). Function's blocking behaviour is not guaranteed,
therefore a

if (PQisnonblocking())
PQsetnonblocking(conn, 0)

kind of phrase required before using PQsendCopyData(). But after a
small investigation, found that both psql and pg_dump runs in
blocking mode - without any PQsetnonblocking() call. Thus, AFAIC,
above phrase isn't necessary.

FYI. Just wanted to inform. Any comments are welcome.

Regards.

Volkan YAZICI wrote:

Here's a new try. This one touches to pg_dump side too - for v3
COPY functions usage instead of deprecated ones.

I'd be so appreciated if somebody can review attached patch.

I have updated the patch to match CVS (attached), but am seeing the
following regression differences where the COPY error messages are now
missing. Any idea what is causing that?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Feb 11 11:24, Bruce Momjian wrote:

I have updated the patch to match CVS (attached), but am seeing the
following regression differences where the COPY error messages are now
missing.

I've revised the patch (attached) and now it seems to be ok - passes
regression tests too. Any other comments are welcome.

Regards.

Applied by Tom Lane. Thanks.

---------------------------------------------------------------------------

Volkan YAZICI wrote:

On Feb 11 11:24, Bruce Momjian wrote:

I have updated the patch to match CVS (attached), but am seeing the
following regression differences where the COPY error messages are now
missing.

I've revised the patch (attached) and now it seems to be ok - passes
regression tests too. Any other comments are welcome.

Regards.

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

--
Bruce Momjian http://candle.pha.pa.us
SRA OSS, Inc. http://www.sraoss.com

+ If your life is a hard drive, Christ can be your backup. +