pgsql: Move scanint8() to numutils.c
Move scanint8() to numutils.c
Move scanint8() to numutils.c and rename to pg_strtoint64(). We
already have a "16" and "32" version of that, and the code inside the
functions was aligned, so this move makes all three versions
consistent. The API is also changed to no longer provide the errorOK
case. Users that need the error checking can use strtoi64().
Reviewed-by: John Naylor <john.naylor@enterprisedb.com>
Discussion: /messages/by-id/b239564c-cad0-b23e-c57e-166d883cb97d@enterprisedb.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/cfc7191dfea330dd7a71e940d59de78129bb6175
Modified Files
--------------
src/backend/parser/parse_node.c | 12 +++-
src/backend/replication/pgoutput/pgoutput.c | 10 ++--
src/backend/utils/adt/int8.c | 90 +----------------------------
src/backend/utils/adt/numutils.c | 84 +++++++++++++++++++++++++++
src/bin/pgbench/pgbench.c | 4 +-
src/include/utils/builtins.h | 1 +
src/include/utils/int8.h | 25 --------
7 files changed, 104 insertions(+), 122 deletions(-)
On 2/14/22 16:18, Peter Eisentraut wrote:
Move scanint8() to numutils.c
Move scanint8() to numutils.c and rename to pg_strtoint64(). We
already have a "16" and "32" version of that, and the code inside the
functions was aligned, so this move makes all three versions
consistent. The API is also changed to no longer provide the errorOK
case. Users that need the error checking can use strtoi64().Reviewed-by: John Naylor <john.naylor@enterprisedb.com>
Discussion: /messages/by-id/b239564c-cad0-b23e-c57e-166d883cb97d@enterprisedb.com
(moving to hackers)
I guess shame on me for not noticing the thread, but I don't see any
discussion about the potential for breakage to external projects.
scanint8() is exported, and this change breaks at least two extensions I
maintain.
A quick scan (no pun intended ;-)) of github shows other potential
breakage, including at least older (still open source) versions of
pglogical.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
On Tue, Feb 15, 2022 at 10:39 AM Joe Conway <mail@joeconway.com> wrote:
(moving to hackers)
I guess shame on me for not noticing the thread, but I don't see any
discussion about the potential for breakage to external projects.scanint8() is exported, and this change breaks at least two extensions I
maintain.A quick scan (no pun intended ;-)) of github shows other potential
breakage, including at least older (still open source) versions of
pglogical.
I don't have a particularly strong view on whether the underlying
change was a good idea here, but the breakage you're talking about
seems pretty easy to fix, unless I'm missing something? I think it
would be a bad idea to make such a change in a minor release without
concern for extension breakage, but in a new major release it doesn't
seem like a problem to me.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 2/15/22 13:47, Robert Haas wrote:
On Tue, Feb 15, 2022 at 10:39 AM Joe Conway <mail@joeconway.com> wrote:
(moving to hackers)
I guess shame on me for not noticing the thread, but I don't see any
discussion about the potential for breakage to external projects.scanint8() is exported, and this change breaks at least two extensions I
maintain.A quick scan (no pun intended ;-)) of github shows other potential
breakage, including at least older (still open source) versions of
pglogical.I don't have a particularly strong view on whether the underlying
change was a good idea here, but the breakage you're talking about
seems pretty easy to fix, unless I'm missing something? I think it
would be a bad idea to make such a change in a minor release without
concern for extension breakage, but in a new major release it doesn't
seem like a problem to me.
I'm not saying it is hard to fix, but it seems a bit cavalier to not
even discuss the potential for breakage. If nothing else we should flag
this as something for the release notes.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Feb 15, 2022 at 10:39 AM Joe Conway <mail@joeconway.com> wrote:
scanint8() is exported, and this change breaks at least two extensions I
maintain.
I don't have a particularly strong view on whether the underlying
change was a good idea here, but the breakage you're talking about
seems pretty easy to fix, unless I'm missing something? I think it
would be a bad idea to make such a change in a minor release without
concern for extension breakage, but in a new major release it doesn't
seem like a problem to me.
Yeah, this seems well within our expectations for major-release
changes.
regards, tom lane
On 15.02.22 20:00, Joe Conway wrote:
I'm not saying it is hard to fix, but it seems a bit cavalier to not
even discuss the potential for breakage. If nothing else we should flag
this as something for the release notes.
I don't think we have ever systematically release-noted backend API
changes. I don't know whether that would be useful, but a complete
treatment would be a significant effort (speaking from experience of
porting the mentioned pglogical between major releases).
On Wed, Feb 16, 2022 at 6:09 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
I don't think we have ever systematically release-noted backend API
changes. I don't know whether that would be useful, but a complete
treatment would be a significant effort (speaking from experience of
porting the mentioned pglogical between major releases).
Personally, I don't think I would ever have used such a thing if it
had existed, because looking through the git history seems more
efficient to me. The release notes can be wrong or can fail to contain
enough information to fix whatever issue I've encountered, but the
offending commit always tells the real story. It sounds like Joe may
feel differently which is fair enough; I can only speak to my own
experience.
--
Robert Haas
EDB: http://www.enterprisedb.com
Hi,
On Wed, Feb 16, 2022 at 08:24:31AM -0500, Robert Haas wrote:
On Wed, Feb 16, 2022 at 6:09 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:I don't think we have ever systematically release-noted backend API
changes. I don't know whether that would be useful, but a complete
treatment would be a significant effort (speaking from experience of
porting the mentioned pglogical between major releases).Personally, I don't think I would ever have used such a thing if it
had existed, because looking through the git history seems more
efficient to me. The release notes can be wrong or can fail to contain
enough information to fix whatever issue I've encountered, but the
offending commit always tells the real story. It sounds like Joe may
feel differently which is fair enough; I can only speak to my own
experience.
Agreed. I have been maintaining extensions for quite some time and the commit
log (and possibly the referenced discussions) always contains everything needed
to fix whatever code is broken with all the important details. I also try to
rebuild my extensions regularly against the current HEAD, so at the time the
release notes are written they wouldn't be of any use anyway.
The only reason I see to have something in the release notes would be to warn
about a problematic API change, which doesn't result in hard compilation
failure or something mostly immediate like that.