Re: [HACKERS] libpq COPY handling
On Thu, Apr 25, 2013 at 1:56 PM, Tom Lane t...@sss.pgh.pa.us wrote: However, the documentation in libpq.sgml is a bit bogus too, because it counsels trying the PQputCopyEnd() call again, which will not work (since we already changed the asyncStatus). We could make that say a zero result is informational, you might want to try PQflush() later. The trouble with this, though, is that any existing callers that were coded to the old spec would now be broken. It might be better to consider that the code is correct and fix the documentation. I notice that the other places in fe-exec.c that call pqFlush() generally say In nonblock mode, don't complain if we're unable to send it all, which is pretty much the spirit of what this is doing though it lacks that comment. Changing the meaning of a 0 return code seems like a bad idea. However, not ever returning 0 isn't great either: someone could be forgiven for writing code that calls PQputCopyData/End() until they get a 0 result, then waits for the socket to become write-OK before continuing. They might be surprised to find that this results in libpq's internal buffer expanding until malloc fails. I'm gathering that what I should probably do is something like this. Send some data using PQputCopyData() or an end-of-copy using PQputCopyEnd(). Then, call PQflush(). If the latter returns 1, then wait for the socket to become write-OK and try again. Once it returns 0, send the next hunk of copy-data. This is a bit of an exasperating interface because when the socket buffer fills up, PQputCopyData() will do a write() but we won't know what happened; we'll have to call PQflush() and try a second write of the same data without an intervening wait to find out that the buffer is full. If this worked as currently documented, that wouldn't be necessary. But it seems tolerable. Also, I noticed that there are a few places in fe-protocol3.c that seem not to know about COPY-BOTH mode. I'm not sure that any of these are actually bugs right now given the current very limited use of COPY-BOTH mode, but I'm wondering whether it wouldn't be better to minimize the chance of future surprises. Patch attached. +1 for these changes, anyway. OK, committed, thanks. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq COPY handling
Robert Haas robertmh...@gmail.com writes: On Thu, Apr 25, 2013 at 1:56 PM, Tom Lane t...@sss.pgh.pa.us wrote: However, the documentation in libpq.sgml is a bit bogus too, because it counsels trying the PQputCopyEnd() call again, which will not work (since we already changed the asyncStatus). We could make that say a zero result is informational, you might want to try PQflush() later. The trouble with this, though, is that any existing callers that were coded to the old spec would now be broken. Changing the meaning of a 0 return code seems like a bad idea. However, not ever returning 0 isn't great either: someone could be forgiven for writing code that calls PQputCopyData/End() until they get a 0 result, then waits for the socket to become write-OK before continuing. Anybody who tried that would have already discovered that it doesn't work, so that argument seems pretty hollow. What I'm suggesting is that we fix the documentation to match what the code actually does, ie 1 and -1 are the only return codes, but in nonblock mode it may not actually have flushed all the data. I do not see how that can break any code that works now. An alternative possibility is to document the zero return case as can't happen now, but defined for possible future expansion, which I rather suspect was the thought process last time this was looked at. The trouble with that is that, if we ever did try to actually return zero, we'd more than likely break some code that should have been checking for the case and wasn't. Anyway, in view of the lack of complaints from the field, I think changing the behavior of this code would be much more likely to cause problems than fix them. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq COPY handling
On Fri, Apr 26, 2013 at 10:48 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Thu, Apr 25, 2013 at 1:56 PM, Tom Lane t...@sss.pgh.pa.us wrote: However, the documentation in libpq.sgml is a bit bogus too, because it counsels trying the PQputCopyEnd() call again, which will not work (since we already changed the asyncStatus). We could make that say a zero result is informational, you might want to try PQflush() later. The trouble with this, though, is that any existing callers that were coded to the old spec would now be broken. Changing the meaning of a 0 return code seems like a bad idea. However, not ever returning 0 isn't great either: someone could be forgiven for writing code that calls PQputCopyData/End() until they get a 0 result, then waits for the socket to become write-OK before continuing. Anybody who tried that would have already discovered that it doesn't work, so that argument seems pretty hollow. What I'm suggesting is that we fix the documentation to match what the code actually does, ie 1 and -1 are the only return codes, but in nonblock mode it may not actually have flushed all the data. I do not see how that can break any code that works now. An alternative possibility is to document the zero return case as can't happen now, but defined for possible future expansion, which I rather suspect was the thought process last time this was looked at. The trouble with that is that, if we ever did try to actually return zero, we'd more than likely break some code that should have been checking for the case and wasn't. Anyway, in view of the lack of complaints from the field, I think changing the behavior of this code would be much more likely to cause problems than fix them. Sure, I don't think we can change the behavior now; I just think it's a shame that the documented behavior was never implemented. But at this point we don't have much choice but to live with it. By the by, might it be time to start thinking about deprecating protocol version 2, at least for COPY? It seems that supporting it requires nontrivial contortions of both the front-end and back-end code, some of which seem possibly relevant to performance. Obviously we're not going to do anything to 9.3 at this point, but maybe for the next release... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq COPY handling
On 27/04/13 02:48, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On Thu, Apr 25, 2013 at 1:56 PM, Tom Lane t...@sss.pgh.pa.us wrote: However, the documentation in libpq.sgml is a bit bogus too, because it counsels trying the PQputCopyEnd() call again, which will not work (since we already changed the asyncStatus). We could make that say a zero result is informational, you might want to try PQflush() later. The trouble with this, though, is that any existing callers that were coded to the old spec would now be broken. Changing the meaning of a 0 return code seems like a bad idea. However, not ever returning 0 isn't great either: someone could be forgiven for writing code that calls PQputCopyData/End() until they get a 0 result, then waits for the socket to become write-OK before continuing. Anybody who tried that would have already discovered that it doesn't work, so that argument seems pretty hollow. What I'm suggesting is that we fix the documentation to match what the code actually does, ie 1 and -1 are the only return codes, but in nonblock mode it may not actually have flushed all the data. I do not see how that can break any code that works now. An alternative possibility is to document the zero return case as can't happen now, but defined for possible future expansion, which I rather suspect was the thought process last time this was looked at. The trouble with that is that, if we ever did try to actually return zero, we'd more than likely break some code that should have been checking for the case and wasn't. Anyway, in view of the lack of complaints from the field, I think changing the behavior of this code would be much more likely to cause problems than fix them. regards, tom lane The original usage of returns codes was as an offset for the Operating System to jump to in a list of addresses to execute, after program completion. Zero offset was to the first address for normal completion, then 4 for the next address... Addresses were stored in 4 bytes. Hence an historical tendency to define return codes in multiple of 4. This dates back to the Mainframe days, pre UNIX, even! Personally I would prefer zero to indicate an error, on the basis that is the default value for memory, but historical usage makes that impractical! So we are stuck with zero being interpreted as the return code indicating 'Normal' completion - bash, and other Linux shells, are designed on this assumption. I first came across this return code convention when I was programming Mainframes in the early 1970's. Cheers, Gavin
Re: [HACKERS] libpq COPY handling
Robert Haas robertmh...@gmail.com writes: Noah Misch pointed out something interesting to me: /* * PQputCopyEnd - send EOF indication to the backend during COPY IN * * After calling this, use PQgetResult() to check command completion status. * * Returns 1 if successful, 0 if data could not be sent (only possible * in nonblock mode), or -1 if an error occurs. */ The comment alleges that 0 is a possible return value, but the only return statements in the code for that function return literal values of either 1 or -1. I'm not sure whether that's a bug in the code or the documentation. Hm. pqFlush has a three-way return value which PQputCopyEnd is only checking as a boolean. Probably the intent was to return data not sent if pqFlush reports that. However, the documentation in libpq.sgml is a bit bogus too, because it counsels trying the PQputCopyEnd() call again, which will not work (since we already changed the asyncStatus). We could make that say a zero result is informational, you might want to try PQflush() later. The trouble with this, though, is that any existing callers that were coded to the old spec would now be broken. It might be better to consider that the code is correct and fix the documentation. I notice that the other places in fe-exec.c that call pqFlush() generally say In nonblock mode, don't complain if we're unable to send it all, which is pretty much the spirit of what this is doing though it lacks that comment. Also, I noticed that there are a few places in fe-protocol3.c that seem not to know about COPY-BOTH mode. I'm not sure that any of these are actually bugs right now given the current very limited use of COPY-BOTH mode, but I'm wondering whether it wouldn't be better to minimize the chance of future surprises. Patch attached. +1 for these changes, anyway. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] libpq COPY handling
Noah Misch pointed out something interesting to me: /* * PQputCopyEnd - send EOF indication to the backend during COPY IN * * After calling this, use PQgetResult() to check command completion status. * * Returns 1 if successful, 0 if data could not be sent (only possible * in nonblock mode), or -1 if an error occurs. */ The comment alleges that 0 is a possible return value, but the only return statements in the code for that function return literal values of either 1 or -1. I'm not sure whether that's a bug in the code or the documentation. Also, I noticed that there are a few places in fe-protocol3.c that seem not to know about COPY-BOTH mode. I'm not sure that any of these are actually bugs right now given the current very limited use of COPY-BOTH mode, but I'm wondering whether it wouldn't be better to minimize the chance of future surprises. Patch attached. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company libpq-copy-both.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
