Redux: Fix hangup of execution of queries like "COPY TO STDOUT" in asynchronous mode (now with tests)

[esabol]

This is an updated version of @true-alex's pull request #101. It is meant to address issue #98.

$dbh->do("COPY (select * from unnest(ARRAY[1,2,3,4,5,6,7,8]::int[]) as mytable(a)) TO STDOUT", {pg_async => PG_ASYNC}); still hangs with DBD::Pg 3.19.0.

I have applied @true-alex's proposed fix to the latest master branch and added tests to t/08async.t.

The proposed fix indeed prevents hanging the process, but I don't think the return value of $dbh->pg_result() is correct. Once $dbh->pg_ready becomes true after executing $dbh->do("COPY (select * from unnest(ARRAY[1,2,3,4,5,6,7,8]::int[]) as mytable(a)) TO STDOUT", {pg_async => PG_ASYNC});, $dbh->pg_result() currently returns -1, but I think it should return 0 (the number of rows affected). Am I right about that, @turnstep ?

Marking PR as a draft for now.

[turnstep]

Yes, it should return 0

[true-alex]

Thanks for adding the tests. I tried adding them myself, but haven't had any success yet.

pg_result returns the number of affected rows, just like do. When executing async copy, we don't know the number of affected rows, so the call returns -1. This behavior is similar to the do method:
returns -1 if the number of rows is unknown or not available.

[esabol]

@true-alex : Sure, that makes sense while $dbh->pg_ready returns false, but shouldn't $dbh->pg_result return zero once $dbh->pg_ready returns true?

[esabol]

I think I just realized the title of the pull request was wrong? I just changed "COPY TO STDIN" to "COPY TO STDOUT".

[jjn1056]

Chiming in on the return value question. After tracing through the code, I think -1 is correct and consistent. Here's why:

Synchronous COPY already returns -1

When you run COPY synchronously, dbd_st_execute hits the PGRES_COPY_OUT/IN/BOTH case and returns -1 (dbdimp.c line 3897). DBI's do() passes that through:

# Synchronous — returns -1
my $ret = $dbh->do("COPY mytable TO STDOUT");
# $ret is -1

If pg_result() returned 0 for async COPY, the two paths would give different answers for the same operation:

# Async — would return 0 (inconsistent)
$dbh->do("COPY mytable TO STDOUT", {pg_async => PG_ASYNC});
# ... pg_ready() ...
my $ret = $dbh->pg_result();
# $ret would be 0, but sync do() returns -1 for the same statement?

-1 has the right DBI semantics

DBI defines -1 as "number of rows is unknown or not available." That's precisely the situation — a COPY statement starts a data transfer, it doesn't affect rows. The actual row count only becomes known after draining with pg_getcopydata. Returning 0 would mean "zero rows affected," which isn't accurate either.

The code is already consistent

pg_db_result already sets rows = -1 for COPY results in its switch statement (dbdimp.c line 5735). The 3-line fix in this PR correctly lets that value escape the PQgetResult loop instead of hanging. No additional changes needed.

Suggested test fix

The TODO test should just assert -1:

is ($res, -1, "pg_result() after async COPY TO STDOUT returns -1 (rows unknown during COPY)");

[esabol]

Thanks for your thoughts on this, @jjn1056. I found your points persuasive, especially this:

DBI defines -1 as "number of rows is unknown or not available." That's precisely the situation — a COPY statement starts a data transfer, it doesn't affect rows. The actual row count only becomes known after draining with pg_getcopydata. Returning 0 would mean "zero rows affected," which isn't accurate either.

What do you think, @turnstep ?

[turnstep]

Yeah, that makes sense now that we see it all in context.

[esabol]

OK, I have rebased and squashed commits. CI pass is clean. Ready for review.

@turnstep : Let me know if you want any changes to the tests.