Re: PQputCopyEnd doesn't adhere to its API contract

On Mon, Sep 8, 2014 at 6:20 PM, David G Johnston
<david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
> On Mon, Sep 8, 2014 at 11:45 AM, Robert Haas [via PostgreSQL] <[hidden
> email]> wrote:
>>
>> On Thu, Sep 4, 2014 at 6:38 PM, David Johnston
>> <[hidden email]> wrote:
>>
>> > One of the trade-offs I mentioned...its more style than anything but
>> > removing the parenthetical (if there is not error in the command) and
>> > writing it more directly seemed preferable in an overview such as this.
>> >
>> > Better: The function will either throw an error or return a PGresult
>> > object[...]
>>
>> Nope. This is not C++, nor is it the backend. It will not throw
>> anything.
>>
> The current sentence reads:
> "The response to this (if there is no error in the command) will be a
> PGresult object bearing a status code of PGRES_COPY_OUT or PGRES_COPY_IN
> (depending on the specified copy direction)."
>
> Maybe this is taken for granted by others, and thus can be excluded here,
> but I'm trying to specify what happens if there is an error in the
> command... Apparently one does not get back a PGresult object...

Well, there's the point of confusion, because the function ALWAYS
returns a PGresult, except maybe in some obscure corner case where it
can't allocate one and therefore returns NULL. What differs is the
result status.

> Ignoring style and such did anything I wrote help to clarify your
> understanding of why pgPutCopyEnd does what it does? As I say this and
> start to try and read the C code I think that it is a good source for
> understanding novice assumptions but there is a gap between the docs and the
> code - though I'm not sure you've identified the only (or even proper)
> location.

Honestly, not really. I thought the language I previously discussed
with Tom was adequate for that; and I'm a little confused as to why
we've gotten on what seems to be somewhat of a digression into nearby
but otherwise-unrelated documentation issues.

> Unlike PQputCopyData there is no explicit logic (pqIsnonblocking(conn)" in
> PQputCopyEnd for non-blocking mode (there is an implicit one via pqFlush).
> This does seem like an oversight - if a minor one since the likihood of not
> being able to add the EOF to the connection's buffer seems highly unlikely -
> but I would expect on the basis of symmetry alone - for both of them to have
> buffer filled testing logic. And, depending on how large *errormsg is, the
> risk of being unable to place the data in the buffer isn't as small and the
> expected EOF case.

Yeah, I think that's a bug in PQputCopyEnd(). It should have the same
kind of pqCheckOutBufferSpace() check that PQputCopyData() does.

> I'm getting way beyond my knowledge level here but my assumption from the
> documentation was that the async mode behavior of returning zero revolved
> around retrying in order to place the data into the buffer - not retrying to
> make sure the buffer is empty. The caller deals with that on their own
> based upon their blocking mode decision. Though we would want to call
> pqFlush for blocking mode callers here since this function should block
> until the buffer is clear.

PQputCopyEnd() already does flush; and PQputCopyData() shouldn't flush.

> So, I thought I agreed with your suggestion that if the final pqFlush
> returns a 1 that pqPutCopyEnd should return 0.

Well, Tom set me straight on that; so I don't think we're considering
any such change. I think you need to make sure you understand the
previous discussion in detail before proposing how to adjust the
documentation (or the code).

> Additionally, if in
> non-blocking mode, and especially if *errormsg is not NULL, the available
> connection buffer length logic in pqPutCopyData should be evaluated here as
> well.

Yes. See the comment three up from this one.

> The most useful and compatible solution is to make pqPutCopyEnd synchronous
> regardless of the user selected blocking mode - which it mostly is now but
> the final pqFlush should be in a loop - and adjust the documentation in the
> areas noted in my patch-email to accommodate that fact.

Uh, no. If you do that, you'll break the code that I spent so much
time writing, which led to my original post. I wouldn't be using
non-blocking mode if it were OK for it to block.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company