Обсуждение: [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

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] and I assume it's safe to close
the corresponding CF entry [2].

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