[PATCH v1] PQputCopyEnd never returns 0, fix the inaccurate comment
PQputCopyEnd returns 1 or -1, never 0, I guess the comment was
copy/paste from PQputCopyData's comment, this should be fixed.
--
Regards
Junwang Zhao
Attachments:
v1-0001-PQputCopyEnd-never-returns-0-fix-the-inaccurate-c.patchapplication/octet-stream; name=v1-0001-PQputCopyEnd-never-returns-0-fix-the-inaccurate-c.patchDownload
From 606dbadd220170285cc91560d81a8ff671a72bde Mon Sep 17 00:00:00 2001
From: Zhao Junwang <zhjwpku@gmail.com>
Date: Mon, 28 Aug 2023 17:54:05 +0800
Subject: [PATCH v1] PQputCopyEnd never returns 0, fix the inaccurate comment
PQputCopyEnd returns 1 or -1, never 0, I guess the comment was
copy/paste from PQputCopyData's comment, this should be fixed.
Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
---
src/interfaces/libpq/fe-exec.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 974d462d4b..084d91b126 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -2709,8 +2709,7 @@ PQputCopyData(PGconn *conn, const char *buffer, int nbytes)
*
* 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.
+ * Returns 1 if successful, -1 if an error occurs.
*/
int
PQputCopyEnd(PGconn *conn, const char *errormsg)
--
2.41.0
Hi Junwang,
PQputCopyEnd returns 1 or -1, never 0, I guess the comment was
copy/paste from PQputCopyData's comment, this should be fixed.
The patch LGTM but I wonder whether we should also change all the
existing calls of PQputCopyEnd() from:
```
PQputCopyEnd(...) <= 0
```
... to:
```
PQputCopyEnd(...) < 0
```
Given the circumstances, checking for equality to zero seems to be at
least strange.
On top of that, none of the PQputCopyData() callers cares whether the
function returns 0 or -1, both are treated the same way. I suspect the
function does some extra work no one asked to do and no one cares
about. Perhaps this function should be refactored too for consistency.
Thoughts?
--
Best regards,
Aleksander Alekseev
On Mon, Aug 28, 2023 at 7:48 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
Hi Junwang,
PQputCopyEnd returns 1 or -1, never 0, I guess the comment was
copy/paste from PQputCopyData's comment, this should be fixed.The patch LGTM but I wonder whether we should also change all the
existing calls of PQputCopyEnd() from:```
PQputCopyEnd(...) <= 0
```... to:
```
PQputCopyEnd(...) < 0
```Given the circumstances, checking for equality to zero seems to be at
least strange.
Yeah, it makes sense to me, or maybe just `PQputCopyEnd(...) == -1`,
let's wait for some other opinions.
On top of that, none of the PQputCopyData() callers cares whether the
function returns 0 or -1, both are treated the same way. I suspect the
function does some extra work no one asked to do and no one cares
about. Perhaps this function should be refactored too for consistency.Thoughts?
--
Best regards,
Aleksander Alekseev
--
Regards
Junwang Zhao
On Mon, Aug 28, 2023 at 09:46:07PM +0800, Junwang Zhao wrote:
Yeah, it makes sense to me, or maybe just `PQputCopyEnd(...) == -1`,
let's wait for some other opinions.
One can argue that PQputCopyEnd() returning 0 could be possible in an
older version of libpq these callers are linking to, but this has
never existed from what I can see (just checked down to 8.2 now).
Anyway, changing these callers may create some backpatching conflicts,
so I'd let them as they are, and just fix the comment.
--
Michael
On Tue, Aug 29, 2023 at 6:40 AM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Aug 28, 2023 at 09:46:07PM +0800, Junwang Zhao wrote:
Yeah, it makes sense to me, or maybe just `PQputCopyEnd(...) == -1`,
let's wait for some other opinions.One can argue that PQputCopyEnd() returning 0 could be possible in an
older version of libpq these callers are linking to, but this has
never existed from what I can see (just checked down to 8.2 now).
Anyway, changing these callers may create some backpatching conflicts,
so I'd let them as they are, and just fix the comment.
sure, thanks for the explanation.
--
Michael
--
Regards
Junwang Zhao
Hi,
On Mon, Aug 28, 2023 at 09:46:07PM +0800, Junwang Zhao wrote:
Yeah, it makes sense to me, or maybe just `PQputCopyEnd(...) == -1`,
let's wait for some other opinions.One can argue that PQputCopyEnd() returning 0 could be possible in an
older version of libpq these callers are linking to, but this has
never existed from what I can see (just checked down to 8.2 now).
Anyway, changing these callers may create some backpatching conflicts,
so I'd let them as they are, and just fix the comment.sure, thanks for the explanation.
The patch was applied in 8bf7db02 [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=8bf7db0285dfbc4b505c8be4c34ab7386eb6297f and I assume it's safe to close
the corresponding CF entry [2]https://commitfest.postgresql.org/44/4521/.
Thanks, everyone.
[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=8bf7db0285dfbc4b505c8be4c34ab7386eb6297f
[2]: https://commitfest.postgresql.org/44/4521/
--
Best regards,
Aleksander Alekseev