[PATCH v1] PQputCopyEnd never returns 0, fix the inaccurate comment

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