Re: PQputCopyEnd doesn't adhere to its API contract - Mailing list pgsql-hackers
| From | David Johnston |
|---|---|
| Subject | Re: PQputCopyEnd doesn't adhere to its API contract |
| Date | |
| Msg-id | CAKFQuwZk+BrXj+bmYo7CYDiwJ3YLVb2G2xKd2drYfHVvPwUDJw@mail.gmail.com Whole thread Raw |
| In response to | Re: PQputCopyEnd doesn't adhere to its API contract (Robert Haas <robertmhaas@gmail.com>) |
| List | pgsql-hackers |
On Mon, Sep 8, 2014 at 6:20 PM, David G Johnston
<david.g.johnston@gmail.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:
>>
> 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.
My theory here is that if you discover a single point of confusion that cannot by attributed to a typo (or something basic like that) then why not go looking around and see if there are any other issues in nearby code/documentation. Furthermore, not being the writer of said code/documentation, a fresh perspective of the entire body of work - and not just the few lines you quoted - has value. For me, if nothing else I took it as a chance to learn and evaluate the status quo. And as noted below there are a couple of items to address in the nearby code even if the documentation itself is accurate - I just took a very indirect route to discover them.
Much of the current documentation is copy-and-pasted directly from the source code - with a few comments tacked on for clarity. I'm sure some layout and style concerns were present during the copy-and-paste but the user-facing documentation does not, and probably should not ideally, directly emulate the source code comments. Admittedly, in the libpq section this can be considerably relaxed since the target user is likely a C programmer.
> 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.
Anybody disagree here?
> 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.
My point being pgFlush is sensitive to whether the caller is in blocking mode.
Also, pgPutCopyData DOES FLUSH if the buffer is full...then attempts to resize the buffer...then returns 0 (or -1 in blocking mode). How absolute is Tom's
"Well, we certainly do NOT want a flush in PQputCopyData."
?
> 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).
That was largely what this exercise was about for me...having gone through all this and now re-reading Tom's wording I do understand and agree.
For surround documentation I think we are good since pqPutCopyData already tests the buffer and correctly returns 0 in non-blocking mode. The issue with pqPutCopyEnd seems to be a copy/paste error and an incorrect assumption regarding non-blocking mode.
This doesn't address the usage bug we've been propagating where someone very well might use non-blocking mode and, upon seeing a return value of 1 from pqPutCopyEnd, immediately call pqGetResult even if the buffer is not completed flushed. Our proposed documentation directs people to always pgFlush poll after calling pqPutCopyEnd while the current documentation is not so strict. Is there anything to say/do about that fact or - more importantly - is there any real risk here?
David J.
pgsql-hackers by date: