Thread: [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
Attachment
Re: [PATCH v1] PQputCopyEnd never returns 0, fix the inaccurate comment
From
Aleksander Alekseev
Date:
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
Attachment
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
Re: [PATCH v1] PQputCopyEnd never returns 0, fix the inaccurate comment
From
Aleksander Alekseev
Date:
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