Re: Change atoi to strtol in same place
> On 5 Mar 2020, at 06:06, Joe Nelson wrote: > > Daniel Gustafsson wrote: > >>> On 11 Feb 2020, at 17:54, Alvaro Herrera wrote: >>> >>> This patch doesn't currently apply; it has conflicts with at least >>> 01368e5d9da7 and 7e735035f208; even in 7e735035f208^ it applies with >>> fuzz. Please post an updated version so that it can move forward. >> >> Ping. With the 2020-03 CommitFest now under way, are you able to supply a >> rebased patch for consideration? > > My patch relies on another that was previously returned with feedback in > the 2019-11 CF: https://commitfest.postgresql.org/25/2272/ > > I've lost interest in continuing to rebase this. Someone else can take over > the > work if they are interested in it. Otherwise just close the CF entry. Ok, I'm marking this as withdrawn in the CF app, anyone interested can pick it up where this thread left off and re-submit. Thanks for working on it! cheers ./daniel
Re: Change atoi to strtol in same place
Daniel Gustafsson wrote: > > On 11 Feb 2020, at 17:54, Alvaro Herrera wrote: > > > > This patch doesn't currently apply; it has conflicts with at least > > 01368e5d9da7 and 7e735035f208; even in 7e735035f208^ it applies with > > fuzz. Please post an updated version so that it can move forward. > > Ping. With the 2020-03 CommitFest now under way, are you able to supply a > rebased patch for consideration? My patch relies on another that was previously returned with feedback in the 2019-11 CF: https://commitfest.postgresql.org/25/2272/ I've lost interest in continuing to rebase this. Someone else can take over the work if they are interested in it. Otherwise just close the CF entry.
Re: Change atoi to strtol in same place
> On 11 Feb 2020, at 17:54, Alvaro Herrera wrote: > > On 2019-Dec-06, Joe Nelson wrote: > >> I see this patch has been moved to the next commitfest. What's the next >> step, does it need another review? > > This patch doesn't currently apply; it has conflicts with at least > 01368e5d9da7 and 7e735035f208; even in 7e735035f208^ it applies with > fuzz. Please post an updated version so that it can move forward. Ping. With the 2020-03 CommitFest now under way, are you able to supply a rebased patch for consideration? cheers ./daniel
Re: Change atoi to strtol in same place
On 2019-Dec-06, Joe Nelson wrote: > I see this patch has been moved to the next commitfest. What's the next > step, does it need another review? This patch doesn't currently apply; it has conflicts with at least 01368e5d9da7 and 7e735035f208; even in 7e735035f208^ it applies with fuzz. Please post an updated version so that it can move forward. On the other hand, I doubt that patching pg_standby is productive. I would just leave that out entirely. See this thread from 2014 https://postgr.es/m/545946e9.8060...@gmx.net -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Change atoi to strtol in same place
On 2019-12-06 18:43, Joe Nelson wrote: I see this patch has been moved to the next commitfest. What's the next step, does it need another review? I think you need to promote your patch better. The thread subject and the commit fest entry title are somewhat nonsensical and no longer match what the patch actually does. The patch contains no commit message and no documentation or test changes, so it's not easy to make out what it's supposed to do or verify that it does so correctly. A reviewer would have to take this patch on faith or manually test every single command line option to see what it does. Moreover, a lot of this error message tweaking is opinion-based, so it's more burdensome to do an objective review. This patch is competing for attention against more than 200 other patches that have more going for them in these aspects. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Change atoi to strtol in same place
I see this patch has been moved to the next commitfest. What's the next step, does it need another review? -- Joe Nelson https://begriffs.com
Re: Change atoi to strtol in same place
At Fri, 11 Oct 2019 23:27:54 -0500, Joe Nelson wrote in > Here's v6 of the patch. > > [x] Rebase on 20961ceaf0 > [x] Don't call exit(1) after pg_fatal() > [x] Use Tom Lane's suggestion for %lld in _() string > [x] Allow full unsigned 16-bit range for ports (don't disallow ports 0-1023) > [x] Explicitly cast parsed values to smaller integers Thank you for the new version. By the way in the upthread, At Tue, 8 Oct 2019 01:46:51 -0500, Joe Nelson wrote in > > I see Michael's patch is adding this new return type, but really, is > > there a good reason why we need to do something special when the user > > does not pass in an integer? I agree to David in that it's better to avoid that kind of complexity if possible. The significant point of separating them was that you don't want to suggest a false value range for non-integer inputs. Looking the latest patch, the wrong suggestions and the complexity introduced by the %lld alternative are already gone. So I think we're reaching the simple solution where pg_strtoint64_range doesn't need to be involved in message building. " must be an integer in the range (mm .. xx)" Doesn't the generic message work for all kinds of failure here? # It is also easy for transators than the split message case. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Change atoi to strtol in same place
Here's v6 of the patch.
[x] Rebase on 20961ceaf0
[x] Don't call exit(1) after pg_fatal()
[x] Use Tom Lane's suggestion for %lld in _() string
[x] Allow full unsigned 16-bit range for ports (don't disallow ports 0-1023)
[x] Explicitly cast parsed values to smaller integers
--
Joe Nelson https://begriffs.com
diff --git a/contrib/pg_standby/Makefile b/contrib/pg_standby/Makefile
index 0bca2f8e9e..cb9292d0f4 100644
--- a/contrib/pg_standby/Makefile
+++ b/contrib/pg_standby/Makefile
@@ -6,6 +6,8 @@ PGAPPICON = win32
PROGRAM = pg_standby
OBJS = pg_standby.o $(WIN32RES)
+PG_LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 031b1b5cd5..c6405d8b94 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -33,6 +33,7 @@
#include "pg_getopt.h"
#include "access/xlog_internal.h"
+#include "fe_utils/option.h"
const char *progname;
@@ -678,6 +679,10 @@ main(int argc, char **argv)
while ((c = getopt(argc, argv, "cdk:lr:s:t:w:")) != -1)
{
+ pg_strtoint_status s;
+ int64 parsed;
+ char *parse_error;
+
switch (c)
{
case 'c': /* Use copy */
@@ -687,12 +692,15 @@ main(int argc, char **argv)
debug = true;
break;
case 'k': /* keepfiles */
-keepfiles = atoi(optarg);
-if (keepfiles < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -k keepfiles must be >= 0\n", progname);
+ fprintf(stderr, "%s: invalid argument for -k keepfiles: %s\n",
+ progname, parse_error);
exit(2);
}
+keepfiles = (int) parsed;
break;
case 'l': /* Use link */
@@ -706,31 +714,39 @@ main(int argc, char **argv)
#endif
break;
case 'r': /* Retries */
-maxretries = atoi(optarg);
-if (maxretries < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -r maxretries must be >= 0\n", progname);
+ fprintf(stderr, "%s: invalid argument for -r maxretries: %s\n",
+ progname, parse_error);
exit(2);
}
+maxretries = (int) parsed;
break;
case 's': /* Sleep time */
-sleeptime = atoi(optarg);
-if (sleeptime <= 0 || sleeptime > 60)
+s = pg_strtoint64_range(optarg, , 1, 60, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -s sleeptime incorrectly set\n", progname);
+ fprintf(stderr, "%s: invalid argument for -s sleeptime: %s\n",
+ progname, parse_error);
exit(2);
}
+sleeptime = (int) parsed;
break;
case 't': /* Trigger file */
triggerPath = pg_strdup(optarg);
break;
case 'w': /* Max wait time */
-maxwaittime = atoi(optarg);
-if (maxwaittime < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -w maxwaittime incorrectly set\n", progname);
+ fprintf(stderr, "%s: invalid argument for -w maxwaittime: %s\n",
+ progname, parse_error);
exit(2);
}
+maxwaittime = (int) parsed;
break;
default:
fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 55ef13926d..2c0fbbc721 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -32,6 +32,7 @@
#include "common/logging.h"
#include "common/string.h"
#include "fe_utils/recovery_gen.h"
+#include "fe_utils/option.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
@@ -2073,6 +2074,10 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
long_options, _index)) != -1)
{
+ pg_strtoint_status s;
+ int64 parsed;
+ char *parse_error;
+
switch (c)
{
case 'C':
@@ -2157,12 +2162,13 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
-compresslevel = atoi(optarg);
-if (compresslevel < 0 || compresslevel > 9)
+s = pg_strtoint64_range(optarg, , 0, 9, _error);
+if (s != PG_STRTOINT_OK)
{
- pg_log_error("invalid compression level \"%s\"", optarg);
+ pg_log_error("invalid compression level: %s", parse_error);
exit(1);
}
+compresslevel = (int) parsed;
break;
case 'c':
if (pg_strcasecmp(optarg, "fast") == 0)
@@ -2195,12 +2201,14 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
-standby_message_timeout = atoi(optarg) * 1000;
-if (standby_message_timeout < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX / 1000, _error);
+if (s != PG_STRTOINT_OK)
{
- pg_log_error("invalid status interval \"%s\"", optarg);
+
Re: Change atoi to strtol in same place
David Rowley writes: > On Tue, 8 Oct 2019 at 19:46, Joe Nelson wrote: >> David Rowley wrote: >>> The translation string here must be consistent over all platforms. I >>> think this will cause issues if the translation string uses %ld and >>> the platform requires %lld? >> A very good and subtle point. I'll change it to %lld so that a single >> format string will work everywhere. > The way to do this is to make a temp buffer and snprintf into that > buffer then use %s. We have done it that way in the past, but it was mainly because we couldn't be sure that snprintf was on board with %lld. I think that the new consensus is that forcing use of "long long" is a less messy solution (unless you need to back-patch the code). See commit 6a1cd8b92 for recent precedent. regards, tom lane
Re: Change atoi to strtol in same place
On Tue, 8 Oct 2019 at 19:46, Joe Nelson wrote:
>
> David Rowley wrote:
> > The translation string here must be consistent over all platforms. I
> > think this will cause issues if the translation string uses %ld and
> > the platform requires %lld?
>
> A very good and subtle point. I'll change it to %lld so that a single
> format string will work everywhere.
The way to do this is to make a temp buffer and snprintf into that
buffer then use %s.
See basebackup.c where it does:
char buf[64];
snprintf(buf, sizeof(buf), INT64_FORMAT, total_checksum_failures);
ereport(WARNING,
(errmsg("%s total checksum verification failures", buf)));
as an example.
> > I think you should maybe aim for 2 patches here.
> >
> > Patch 1: ...
> >
> > Patch 2: Add additional validation where we don't currently do
> > anything. e.g pg_dump -j
> >
> > We can then see if there's any merit in patch 2 of if it's adding more
> > complexity than is really needed.
>
> Are you saying that my current patch adds extra constraints for
> pg_dump's -j argument, or that in the future we could do that? Because I
> don't believe the current patch adds any appreciable complexity for that
> particular argument, other than ensuring the value is positive, which
> doesn't seem too contentious.
> Maybe we can see whether we can reach consensus on the current
> parse-and-check combo patch, and if discussion drags on much longer then
> try to split it up?
I just think you're more likely to get a committer onside if you made
it so they didn't have to consider if throwing errors where we
previously didn't would be a bad thing. It's quite common to get core
infrastructure in first then followup with code that uses it. This
would be core infrastructure plus some less controversial usages of
it, then follow up with more. This was really just a suggestion. I
didn't dig into the patch in enough detail to decide on how many
places could raise an error that would have silently just have done
something unexpected beforehand.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Re: Change atoi to strtol in same place
David Rowley wrote: > It's not for this patch to decide what ports clients can connect to > PostgreSQL on. As far as I'm aware Windows has no restrictions on what > ports unprivileged users can listen on. I think we're likely going to > upset a handful of people if we block the client tools from connecting > to ports < 1024. Makes sense. We can instead allow any port number and if it errors at connection time then the user will find out at that point. > This part seems over-complex to me. What's wrong with just returning a > bool and if the caller gets "false", then just have it emit the error, > such as: > > "compression level must be between %d and %d" > > I see Michael's patch is adding this new return type, but really, is > there a good reason why we need to do something special when the user > does not pass in an integer? Displaying the range when given a non-integer input could be misleading. For instance, suppose we put as little restriction on the port number range as possible, enforcing only that it's positive, between 1 and INT_MAX. If someone passes a non-integer value, they would get a message like: invalid port number: must be an integer in range 1..2147483647 Sure, the parsing code will accept such a big number, but we don't want to *suggest* the number. Notice if the user had passed a well-formed number for the port it's unlikely to be greater than INT_MAX, so they wouldn't have to see this weird message. Perhaps you weren't suggesting to remove the upper limit from port checking, just to change the lower limit from 1024 back to 1. In that case we could keep it capped at 65535 and the error message above would be OK. Other utilities do have command line args that are allowed the whole non-negative (but signed) int range, and their error message would show the big number. It's not misleading in that case, but a little ostentatious. > Current patch: > $ pg_dump -Z blah > invalid compression level: could not parse 'blah' as integer > > I propose: > $ pg_dump -Z blah > compression level must be an integer in range 0..9 > > This might save a few round trips, e.g the current patch will do: I do see your reasoning that we're teasing people with a puzzle they have to solve with multiple invocations. On the other hand, passing a non-number for the compression level is pretty strange, and perhaps explicitly calling out the mistake might make someone look more carefully at what they -- or their script -- is doing. > The translation string here must be consistent over all platforms. I > think this will cause issues if the translation string uses %ld and > the platform requires %lld? A very good and subtle point. I'll change it to %lld so that a single format string will work everywhere. > I think you should maybe aim for 2 patches here. > > Patch 1: ... > > Patch 2: Add additional validation where we don't currently do > anything. e.g pg_dump -j > > We can then see if there's any merit in patch 2 of if it's adding more > complexity than is really needed. Are you saying that my current patch adds extra constraints for pg_dump's -j argument, or that in the future we could do that? Because I don't believe the current patch adds any appreciable complexity for that particular argument, other than ensuring the value is positive, which doesn't seem too contentious. Maybe we can see whether we can reach consensus on the current parse-and-check combo patch, and if discussion drags on much longer then try to split it up? > I also think some compilers won't like: > > + compressLevel = parsed; > > given that "parsed" is int64 and "compressLevel" is int, surely some > compilers will warn of possible truncation? An explicit cast to int > should fix those Good point, I bet some compilers (justly) warn about truncation. We've checked the range so I'll add an explicit cast. > or you could consider just writing a version of the function for int32 > and int64 and directly passing in the variable to be set. One complication is that the destination values are often int rather than int32, and I don't know their width in general (probably 32, maybe 16, but *possibly* 64?). The pg_strtoint64_range() function with range argument of INT_MAX is flexible enough to handle whatever situation we encounter. Am I overthinking this part? -- Joe Nelson https://begriffs.com
Re: Change atoi to strtol in same place
On Mon, Oct 7, 2019 at 11:05 AM David Rowley wrote: > > On Mon, 7 Oct 2019 at 18:27, Ashutosh Sharma wrote: > > AFAIU from the information given in the wiki page -[1], the port > > numbers in the range of 1-1023 are for the standard protocols and > > services. And there is nowhere mentioned that it is only true for some > > OS and not for others. But, having said that I've just verified it on > > Linux so I'm not aware of the behaviour on Windows. > > > > [1] - https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers > > Here are the results of a quick test on a windows machine: > > L:\Projects\Postgres\install\bin>echo test > c:\windows\test.txt > Access is denied. > > L:\Projects\Postgres\install\bin>cat ../data/postgresql.conf | grep "port = " > port = 543 # (change requires restart) > > L:\Projects\Postgres\install\bin>psql -p 543 postgres > psql (11.5) > WARNING: Console code page (850) differs from Windows code page (1252) > 8-bit characters might not work correctly. See psql reference > page "Notes for Windows users" for details. > Type "help" for help. > > postgres=# > Oh then that means all the unused ports (be it dedicated for some particular protocol or service) can be used on Windows. I just tried using port number 21 and 443 for postgres on my old Windows setup and it worked. See below, C:\Users\ashu\git-clone-postgresql\postgresql\TMP\test\bin>.\pg_ctl -D ..\data -c -w -l logfile -o " -p 21" start waiting for server to start done server started C:\Users\ashu\git-clone-postgresql\postgresql\TMP\test\bin>.\psql -d postgres -p 21 psql (10.5) WARNING: Console code page (437) differs from Windows code page (1252) 8-bit characters might not work correctly. See psql reference page "Notes for Windows users" for details. Type "help" for help. postgres=# \q C:\Users\ashu\git-clone-postgresql\postgresql\TMP\test\bin>.\pg_ctl -D ..\data -c -w -l logfile stop waiting for server to shut down done server stopped C:\Users\ashu\git-clone-postgresql\postgresql\TMP\test\bin>.\pg_ctl -D ..\data -c -w -l logfile -o " -p 443" start waiting for server to start done server started C:\Users\ashu\git-clone-postgresql\postgresql\TMP\test\bin>.\psql -d postgres -p 443 psql (10.5) WARNING: Console code page (437) differs from Windows code page (1252) 8-bit characters might not work correctly. See psql reference page "Notes for Windows users" for details. Type "help" for help. This looks a weird behaviour to me. I think this is probably one reason why people don't prefer using Windows. Anyways, thanks David for putting that point, it was really helpful. -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com
Re: Change atoi to strtol in same place
On Mon, 7 Oct 2019 at 18:27, Ashutosh Sharma wrote: > AFAIU from the information given in the wiki page -[1], the port > numbers in the range of 1-1023 are for the standard protocols and > services. And there is nowhere mentioned that it is only true for some > OS and not for others. But, having said that I've just verified it on > Linux so I'm not aware of the behaviour on Windows. > > [1] - https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers Here are the results of a quick test on a windows machine: L:\Projects\Postgres\install\bin>echo test > c:\windows\test.txt Access is denied. L:\Projects\Postgres\install\bin>cat ../data/postgresql.conf | grep "port = " port = 543 # (change requires restart) L:\Projects\Postgres\install\bin>psql -p 543 postgres psql (11.5) WARNING: Console code page (850) differs from Windows code page (1252) 8-bit characters might not work correctly. See psql reference page "Notes for Windows users" for details. Type "help" for help. postgres=# -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Change atoi to strtol in same place
On Mon, Oct 7, 2019 at 10:40 AM David Rowley
wrote:
>
> On Mon, 7 Oct 2019 at 13:21, Joe Nelson wrote:
> >
> > Ashutosh Sharma wrote:
> > > One suggestion: The start value for port number is set to 1, however
> > > it seems like the port number that falls in the range of 1-1023 is
> > > reserved and can't be used. So, is it possible to have the start value
> > > as 1024 instead of 1 ?
> >
> > Good idea -- changed it. I also created macros FE_UTILS_PORT_{MIN,MAX}
> > so the range can be updated in one place for all utilities.
>
> (I've only had a very quick look at this, and FWIW, here's my opinion)
>
> It's not for this patch to decide what ports clients can connect to
> PostgreSQL on. As far as I'm aware Windows has no restrictions on what
> ports unprivileged users can listen on. I think we're likely going to
> upset a handful of people if we block the client tools from connecting
> to ports < 1024.
>
AFAIU from the information given in the wiki page -[1], the port
numbers in the range of 1-1023 are for the standard protocols and
services. And there is nowhere mentioned that it is only true for some
OS and not for others. But, having said that I've just verified it on
Linux so I'm not aware of the behaviour on Windows.
[1] - https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers
> > > Further, I encountered one syntax error (INT_MAX undeclared) as the
> > > header file "limits.h" has not been included in postgres_fe.h or
> > > option.h
> >
> > Oops. Added limits.h now in option.h. The Postgres build accidentally
> > worked on my system without explicitly including this header because
> > __has_builtin(__builtin_isinf) is true for me so src/include/port.h
> > pulled in math.h with an #if which pulled in limits.h.
> >
> > > Alvaro Herrera wrote:
> > > > The wording is a bit strange. How about something like pg_standy:
> > > > invalid argument to -k: %s
> >
> > I updated the error messages in pg_standby.
> >
> > > > > *error = psprintf(_("could not parse '%s' as integer"), str);
> > > >
> > > > ... except that they would rather be more explicit about what the
> > > > problem is. "insufficient digits" or "extraneous character", etc.
>
> This part seems over-complex to me. What's wrong with just returning a
> bool and if the caller gets "false", then just have it emit the error,
> such as:
>
> "compression level must be between %d and %d"
>
> I see Michael's patch is adding this new return type, but really, is
> there a good reason why we need to do something special when the user
> does not pass in an integer?
>
> Current patch:
> $ pg_dump -Z blah
> invalid compression level: could not parse 'blah' as integer
>
> I propose:
> $ pg_dump -Z blah
> compression level must be an integer in range 0..9
>
> This might save a few round trips, e.g the current patch will do:
> $ pg_dump -Z blah
> invalid compression level: could not parse 'blah' as integer
> $ pg_dump -Z 12345
> invalid compression level: 12345 is outside range 0..9
> $ ...
>
> Also:
>
> + case PG_STRTOINT_RANGE_ERROR:
> + *error = psprintf(_("%s is outside range "
> + INT64_FORMAT ".." INT64_FORMAT),
> + str, min, max);
>
> The translation string here must be consistent over all platforms. I
> think this will cause issues if the translation string uses %ld and
> the platform requires %lld?
>
> I think what this patch should be really aiming for is to simplify the
> client command-line argument parsing and adding what benefits it can.
> I don't think there's really a need to make anything more complex than
> it is already here.
>
> I think you should maybe aim for 2 patches here.
>
> Patch 1: Add new function to validate range and return bool indicating
> if the string is an integer within range. Set output argument to the
> int value if it is valid. Modify all locations where we currently
> validate the range of the input arg to use the new function.
>
> Patch 2: Add additional validation where we don't currently do
> anything. e.g pg_dump -j
>
> We can then see if there's any merit in patch 2 of if it's adding more
> complexity than is really needed.
>
> I also think some compilers won't like:
>
> + compressLevel = parsed;
>
> given that "parsed" is int64 and "compressLevel" is int, surely some
> compilers will warn of possible truncation? An explicit cast to int
> should fix those or you could consider just writing a version of the
> function for int32 and int64 and directly passing in the variable to
> be set.
>
> --
> David Rowley http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
Re: Change atoi to strtol in same place
On Mon, 7 Oct 2019 at 13:21, Joe Nelson wrote:
>
> Ashutosh Sharma wrote:
> > One suggestion: The start value for port number is set to 1, however
> > it seems like the port number that falls in the range of 1-1023 is
> > reserved and can't be used. So, is it possible to have the start value
> > as 1024 instead of 1 ?
>
> Good idea -- changed it. I also created macros FE_UTILS_PORT_{MIN,MAX}
> so the range can be updated in one place for all utilities.
(I've only had a very quick look at this, and FWIW, here's my opinion)
It's not for this patch to decide what ports clients can connect to
PostgreSQL on. As far as I'm aware Windows has no restrictions on what
ports unprivileged users can listen on. I think we're likely going to
upset a handful of people if we block the client tools from connecting
to ports < 1024.
> > Further, I encountered one syntax error (INT_MAX undeclared) as the
> > header file "limits.h" has not been included in postgres_fe.h or
> > option.h
>
> Oops. Added limits.h now in option.h. The Postgres build accidentally
> worked on my system without explicitly including this header because
> __has_builtin(__builtin_isinf) is true for me so src/include/port.h
> pulled in math.h with an #if which pulled in limits.h.
>
> > Alvaro Herrera wrote:
> > > The wording is a bit strange. How about something like pg_standy:
> > > invalid argument to -k: %s
>
> I updated the error messages in pg_standby.
>
> > > > *error = psprintf(_("could not parse '%s' as integer"), str);
> > >
> > > ... except that they would rather be more explicit about what the
> > > problem is. "insufficient digits" or "extraneous character", etc.
This part seems over-complex to me. What's wrong with just returning a
bool and if the caller gets "false", then just have it emit the error,
such as:
"compression level must be between %d and %d"
I see Michael's patch is adding this new return type, but really, is
there a good reason why we need to do something special when the user
does not pass in an integer?
Current patch:
$ pg_dump -Z blah
invalid compression level: could not parse 'blah' as integer
I propose:
$ pg_dump -Z blah
compression level must be an integer in range 0..9
This might save a few round trips, e.g the current patch will do:
$ pg_dump -Z blah
invalid compression level: could not parse 'blah' as integer
$ pg_dump -Z 12345
invalid compression level: 12345 is outside range 0..9
$ ...
Also:
+ case PG_STRTOINT_RANGE_ERROR:
+ *error = psprintf(_("%s is outside range "
+ INT64_FORMAT ".." INT64_FORMAT),
+ str, min, max);
The translation string here must be consistent over all platforms. I
think this will cause issues if the translation string uses %ld and
the platform requires %lld?
I think what this patch should be really aiming for is to simplify the
client command-line argument parsing and adding what benefits it can.
I don't think there's really a need to make anything more complex than
it is already here.
I think you should maybe aim for 2 patches here.
Patch 1: Add new function to validate range and return bool indicating
if the string is an integer within range. Set output argument to the
int value if it is valid. Modify all locations where we currently
validate the range of the input arg to use the new function.
Patch 2: Add additional validation where we don't currently do
anything. e.g pg_dump -j
We can then see if there's any merit in patch 2 of if it's adding more
complexity than is really needed.
I also think some compilers won't like:
+ compressLevel = parsed;
given that "parsed" is int64 and "compressLevel" is int, surely some
compilers will warn of possible truncation? An explicit cast to int
should fix those or you could consider just writing a version of the
function for int32 and int64 and directly passing in the variable to
be set.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Re: Change atoi to strtol in same place
Ashutosh Sharma wrote:
> One suggestion: The start value for port number is set to 1, however
> it seems like the port number that falls in the range of 1-1023 is
> reserved and can't be used. So, is it possible to have the start value
> as 1024 instead of 1 ?
Good idea -- changed it. I also created macros FE_UTILS_PORT_{MIN,MAX}
so the range can be updated in one place for all utilities.
> Further, I encountered one syntax error (INT_MAX undeclared) as the
> header file "limits.h" has not been included in postgres_fe.h or
> option.h
Oops. Added limits.h now in option.h. The Postgres build accidentally
worked on my system without explicitly including this header because
__has_builtin(__builtin_isinf) is true for me so src/include/port.h
pulled in math.h with an #if which pulled in limits.h.
> Alvaro Herrera wrote:
> > The wording is a bit strange. How about something like pg_standy:
> > invalid argument to -k: %s
I updated the error messages in pg_standby.
> > > *error = psprintf(_("could not parse '%s' as integer"), str);
> >
> > ... except that they would rather be more explicit about what the
> > problem is. "insufficient digits" or "extraneous character", etc.
Sadly pg_strtoint64 returns the same error code for both cases. So we
could either petition for more detailed errors in the thread for that
other patch, or examine the string ourselves to check. Maybe it's not
needed since "could not parse 'abc' as integer" kind of does show the
problem.
> > I hope that no callers would like to have the messages not translated,
> > because that seems like it would become a mess.
That's true... I think it should be OK though, since we return the
pg_strtoint_status so callers can inspect that rather than relying on certain
words being in the error string. I'm guessing the translated string would be
most appropriate for end users.
--
Joe Nelson https://begriffs.com
diff --git a/contrib/pg_standby/Makefile b/contrib/pg_standby/Makefile
index 0bca2f8e9e..cb9292d0f4 100644
--- a/contrib/pg_standby/Makefile
+++ b/contrib/pg_standby/Makefile
@@ -6,6 +6,8 @@ PGAPPICON = win32
PROGRAM = pg_standby
OBJS = pg_standby.o $(WIN32RES)
+PG_LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 031b1b5cd5..9ce736249b 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -33,6 +33,7 @@
#include "pg_getopt.h"
#include "access/xlog_internal.h"
+#include "fe_utils/option.h"
const char *progname;
@@ -678,6 +679,10 @@ main(int argc, char **argv)
while ((c = getopt(argc, argv, "cdk:lr:s:t:w:")) != -1)
{
+ pg_strtoint_status s;
+ int64 parsed;
+ char *parse_error;
+
switch (c)
{
case 'c': /* Use copy */
@@ -687,12 +692,15 @@ main(int argc, char **argv)
debug = true;
break;
case 'k': /* keepfiles */
-keepfiles = atoi(optarg);
-if (keepfiles < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -k keepfiles must be >= 0\n", progname);
+ fprintf(stderr, "%s: invalid argument for -k keepfiles: %s\n",
+ progname, parse_error);
exit(2);
}
+keepfiles = parsed;
break;
case 'l': /* Use link */
@@ -706,31 +714,39 @@ main(int argc, char **argv)
#endif
break;
case 'r': /* Retries */
-maxretries = atoi(optarg);
-if (maxretries < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -r maxretries must be >= 0\n", progname);
+ fprintf(stderr, "%s: invalid argument for -r maxretries: %s\n",
+ progname, parse_error);
exit(2);
}
+maxretries = parsed;
break;
case 's': /* Sleep time */
-sleeptime = atoi(optarg);
-if (sleeptime <= 0 || sleeptime > 60)
+s = pg_strtoint64_range(optarg, , 1, 60, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -s sleeptime incorrectly set\n", progname);
+ fprintf(stderr, "%s: invalid argument for -s sleeptime: %s\n",
+ progname, parse_error);
exit(2);
}
+sleeptime = parsed;
break;
case 't': /* Trigger file */
triggerPath = pg_strdup(optarg);
break;
case 'w': /* Max wait time */
-maxwaittime = atoi(optarg);
-if (maxwaittime < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -w maxwaittime incorrectly set\n", progname);
+ fprintf(stderr, "%s: invalid argument for -w maxwaittime: %s\n",
+ progname, parse_error);
exit(2);
}
+maxwaittime = parsed;
break;
default:
fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
diff --git
Re: Change atoi to strtol in same place
Hi Joe,
On a quick look, the patch seems to be going in a good direction
although there are quite some pending work to be done.
One suggestion:
The start value for port number is set to 1, however it seems like the
port number that falls in the range of 1-1023 is reserved and can't be
used. So, is it possible to have the start value as 1024 instead of 1
?
Further, I encountered one syntax error (INT_MAX undeclared) as the
header file "limits.h" has not been included in postgres_fe.h or
option.h
--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
On Fri, Oct 4, 2019 at 9:04 PM Alvaro Herrera wrote:
>
> On 2019-Oct-03, Joe Nelson wrote:
>
> > Kyotaro Horiguchi wrote:
> > > > pg_standby: -k keepfiles could not parse 'hoge' as integer
> > >
> > > I didn't checked closely, but -k of pg_standby's message looks
> > > somewhat strange. Needs a separator?
> >
> > Good point, how about this:
> >
> > pg_standby: -k keepfiles:
>
> The wording is a bit strange. How about something like
> pg_standy: invalid argument to -k: %s
>
> where the %s is the error message produced like you propose:
>
> > I could have pg_strtoint64_range() wrap its error messages in _() so
> > that translators could customize the messages prior to concatenation.
> >
> > *error = psprintf(_("could not parse '%s' as integer"), str);
>
> ... except that they would rather be more explicit about what the
> problem is. "insufficient digits" or "extraneous character", etc.
>
> > Would this suffice?
>
> I hope that no callers would like to have the messages not translated,
> because that seems like it would become a mess.
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
Re: Change atoi to strtol in same place
On 2019-Oct-03, Joe Nelson wrote:
> Kyotaro Horiguchi wrote:
> > > pg_standby: -k keepfiles could not parse 'hoge' as integer
> >
> > I didn't checked closely, but -k of pg_standby's message looks
> > somewhat strange. Needs a separator?
>
> Good point, how about this:
>
> pg_standby: -k keepfiles:
The wording is a bit strange. How about something like
pg_standy: invalid argument to -k: %s
where the %s is the error message produced like you propose:
> I could have pg_strtoint64_range() wrap its error messages in _() so
> that translators could customize the messages prior to concatenation.
>
> *error = psprintf(_("could not parse '%s' as integer"), str);
... except that they would rather be more explicit about what the
problem is. "insufficient digits" or "extraneous character", etc.
> Would this suffice?
I hope that no callers would like to have the messages not translated,
because that seems like it would become a mess.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Change atoi to strtol in same place
Kyotaro Horiguchi wrote:
> > pg_standby: -k keepfiles could not parse 'hoge' as integer
>
> I didn't checked closely, but -k of pg_standby's message looks
> somewhat strange. Needs a separator?
Good point, how about this:
pg_standby: -k keepfiles:
> Building a sentense just concatenating multiple nonindependent
> (or incomplete) subphrases makes translation harder.
I could have pg_strtoint64_range() wrap its error messages in _() so
that translators could customize the messages prior to concatenation.
*error = psprintf(_("could not parse '%s' as integer"), str);
Would this suffice?
Re: Change atoi to strtol in same place
At Tue, 01 Oct 2019 19:32:08 +0900 (Tokyo Standard Time), Kyotaro Horiguchi wrote in <20191001.193208.264851337.horikyota@gmail.com> > Hello. > > At Sun, 29 Sep 2019 23:51:23 -0500, Joe Nelson wrote in > <20190930045123.gc68...@begriffs.com> > > Alvaro Herrera wrote: > > > ... can we have a new patch? > > > > OK, I've attached v4. It works cleanly on 55282fa20f with > > str2int-16.patch applied. My patch won't compile without the other one > > applied too. > > > > Changed: > > [x] revert my changes in common/Makefile > > [x] rename arg_utils.[ch] to option.[ch] > > [x] update @pgfeutilsfiles in Mkvcbuild.pm > > [x] pgindent everything > > [x] get rid of atoi() in more utilities I didn't checked closely, but -k of pg_standby's message looks somewhat strange. Needs a separator? > pg_standby: -k keepfiles could not parse 'hoge' as integer Building a sentense just concatenating multiple nonindependent (or incomplete) subphrases makes translation harder. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Change atoi to strtol in same place
Hello. At Sun, 29 Sep 2019 23:51:23 -0500, Joe Nelson wrote in <20190930045123.gc68...@begriffs.com> > Alvaro Herrera wrote: > > ... can we have a new patch? > > OK, I've attached v4. It works cleanly on 55282fa20f with > str2int-16.patch applied. My patch won't compile without the other one > applied too. > > Changed: > [x] revert my changes in common/Makefile > [x] rename arg_utils.[ch] to option.[ch] > [x] update @pgfeutilsfiles in Mkvcbuild.pm > [x] pgindent everything > [x] get rid of atoi() in more utilities Compiler complained as "INT_MAX undeclared" (gcc 7.3 / CentOS7.6). > One question about how the utilities parse port numbers. I currently > have it check that the value can be parsed as an integer, and that its > range is within 1 .. (1<<16)-1. I wonder if the former restriction is > (un)desirable, because ultimately getaddrinfo() takes a "service name > description" for the port, which can be a name such as found in > '/etc/services' as well as the string representation of a number. If > desired, I *could* treat only range errors as a failure for ports, and > allow integer parse errors. We could do that, but perhaps no use for our usage. We are not likely to use named ports other than 'postgres', if any. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Change atoi to strtol in same place
Alvaro Herrera wrote:
> ... can we have a new patch?
OK, I've attached v4. It works cleanly on 55282fa20f with
str2int-16.patch applied. My patch won't compile without the other one
applied too.
Changed:
[x] revert my changes in common/Makefile
[x] rename arg_utils.[ch] to option.[ch]
[x] update @pgfeutilsfiles in Mkvcbuild.pm
[x] pgindent everything
[x] get rid of atoi() in more utilities
One question about how the utilities parse port numbers. I currently
have it check that the value can be parsed as an integer, and that its
range is within 1 .. (1<<16)-1. I wonder if the former restriction is
(un)desirable, because ultimately getaddrinfo() takes a "service name
description" for the port, which can be a name such as found in
'/etc/services' as well as the string representation of a number. If
desired, I *could* treat only range errors as a failure for ports, and
allow integer parse errors.
--
Joe Nelson https://begriffs.com
diff --git a/contrib/pg_standby/Makefile b/contrib/pg_standby/Makefile
index 0bca2f8e9e..cb9292d0f4 100644
--- a/contrib/pg_standby/Makefile
+++ b/contrib/pg_standby/Makefile
@@ -6,6 +6,8 @@ PGAPPICON = win32
PROGRAM = pg_standby
OBJS = pg_standby.o $(WIN32RES)
+PG_LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 031b1b5cd5..56ac7fd726 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -33,6 +33,7 @@
#include "pg_getopt.h"
#include "access/xlog_internal.h"
+#include "fe_utils/option.h"
const char *progname;
@@ -678,6 +679,10 @@ main(int argc, char **argv)
while ((c = getopt(argc, argv, "cdk:lr:s:t:w:")) != -1)
{
+ pg_strtoint_status s;
+ int64 parsed;
+ char *parse_error;
+
switch (c)
{
case 'c': /* Use copy */
@@ -687,12 +692,15 @@ main(int argc, char **argv)
debug = true;
break;
case 'k': /* keepfiles */
-keepfiles = atoi(optarg);
-if (keepfiles < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -k keepfiles must be >= 0\n", progname);
+ fprintf(stderr, "%s: -k keepfiles %s\n",
+ progname, parse_error);
exit(2);
}
+keepfiles = parsed;
break;
case 'l': /* Use link */
@@ -706,31 +714,39 @@ main(int argc, char **argv)
#endif
break;
case 'r': /* Retries */
-maxretries = atoi(optarg);
-if (maxretries < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -r maxretries must be >= 0\n", progname);
+ fprintf(stderr, "%s: -r maxretries %s\n",
+ progname, parse_error);
exit(2);
}
+maxretries = parsed;
break;
case 's': /* Sleep time */
-sleeptime = atoi(optarg);
-if (sleeptime <= 0 || sleeptime > 60)
+s = pg_strtoint64_range(optarg, , 1, 60, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -s sleeptime incorrectly set\n", progname);
+ fprintf(stderr, "%s: -s sleeptime %s\n",
+ progname, parse_error);
exit(2);
}
+sleeptime = parsed;
break;
case 't': /* Trigger file */
triggerPath = pg_strdup(optarg);
break;
case 'w': /* Max wait time */
-maxwaittime = atoi(optarg);
-if (maxwaittime < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -w maxwaittime incorrectly set\n", progname);
+ fprintf(stderr, "%s: -w maxwaittime %s\n",
+ progname, parse_error);
exit(2);
}
+maxwaittime = parsed;
break;
default:
fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 55ef13926d..7869c8cf9a 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -32,6 +32,7 @@
#include "common/logging.h"
#include "common/string.h"
#include "fe_utils/recovery_gen.h"
+#include "fe_utils/option.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
@@ -2073,6 +2074,10 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
long_options, _index)) != -1)
{
+ pg_strtoint_status s;
+ int64 parsed;
+ char *parse_error;
+
switch (c)
{
case 'C':
@@ -2157,12 +2162,13 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
-compresslevel = atoi(optarg);
-if (compresslevel < 0 || compresslevel > 9)
+s = pg_strtoint64_range(optarg, , 0, 9, _error);
+if (s != PG_STRTOINT_OK)
{
- pg_log_error("invalid compression level \"%s\"", optarg);
+ pg_log_error("invalid compression level: %s",
Re: Change atoi to strtol in same place
On Fri, Sep 27, 2019 at 09:35:53PM -0500, Joe Nelson wrote: > Note that it requires functions from str2int-10.patch, and will not > compile when applied to master by itself. I didn't want to duplicate > functionality from that other uncommitted patch in mine. If we have a dependency between both threads, perhaps more people could comment there? Here is the most relevant update: https://www.postgresql.org/message-id/20190917022913.gb1...@paquier.xyz -- Michael signature.asc Description: PGP signature
Re: Change atoi to strtol in same place
Alvaro Herrera wrote: > ... can we have a new patch? Not only because there seems to have > been some discussion points that have gone unaddressed (?) Yes, I'll work on it over the weekend. > but also because Appveyor complains really badly about this one. > https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.58672 Note that it requires functions from str2int-10.patch, and will not compile when applied to master by itself. I didn't want to duplicate functionality from that other uncommitted patch in mine.
Re: Change atoi to strtol in same place
... can we have a new patch? Not only because there seems to have been some discussion points that have gone unaddressed (?), but also because Appveyor complains really badly about this one. https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.58672 -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Change atoi to strtol in same place
On Tue, Sep 10, 2019 at 11:54 PM Michael Paquier wrote: > On Tue, Sep 10, 2019 at 08:03:32AM -0400, Robert Haas wrote: > > -1. I think it's very useful to have routines for this sort of thing > > that return an error message rather than emitting an error report > > directly. That gives the caller a lot more control. > > Please let me counter-argue here. OK, but on the other hand, Joe's example of a custom message "invalid compression level: 10 is outside range 0..9" is a world better than "invalid compression level: %s". We might even be able to do better "argument to -Z must be a compression level between 0 and 9". In backend error-reporting, it's often important to show the misguided value, because it may be coming from a complex query where it's hard to find the source of the problematic value. But if the user types -Z42 or -Zborked, I'm not sure it's important to tell them that the problem is with "42" or "borked". It's more important to explain the concept, or such would be my judgement. Also, consider an option where the value must be an integer between 1 and 100 or one of several fixed strings (e.g. think of recovery_target_timeline). The user clearly can't use the generic error message in that case. Perhaps the answer is to say that such users shouldn't use the provided range-checking function but rather implement the logic from scratch. But that seems a bit limiting. Also, suppose the user doesn't want to pg_log_error(). Maybe it's a warning. Maybe it doesn't even need to be logged. What this boils down to in the end is that putting more of the policy decisions into the function helps ensure consistency and save code when the function is used, but it also results in the function being used less often. Reasonable people can differ on the merits of different approaches, but for me the down side of returning the error message appears minor at most, and the up sides seem significant. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Change atoi to strtol in same place
Robert Haas wrote: > > -1. I think it's very useful to have routines for this sort of thing > > that return an error message rather than emitting an error report > > directly. That gives the caller a lot more control. Michael Paquier wrote: > 1) Consistency with the error messages makes less work for translators, > who already have a lot to deal with. Agreed that the messages can be slightly inconsistent. I tried to make the new messages match the styles of other messages in their respective utilities. Maybe the bigger issue here is inconsistent output styles across the utilities in general: pg_standby.c includes flag names %s: -r maxretries %s pg_basebackup.c writes the settings out in words invalid compression level: %s Note that the final %s in those examples will expand to a more detailed message. For example passing "-Z 10" to pg_dump in the current patch will output: pg_dump: error: invalid compression level: 10 is outside range 0..9 > 2) A centralized error message can provide the same level of details. Even assuming we standardize the message format, different callers have different means to handle the messages. The front-end utilities affected in my patch use calls as varied as fprintf, pg_log_error, write_stderr and pg_fatal. Thus pg_strtoint64_range needs more flexibility than calling pg_log_error internally. > 3) I think that we should not expose directly the status values of > pg_strtoint_status in pg_strtoint64_range(), that's less for module > authors to worry about, and that would be the same approach as we are > using for the wrappers of pg_strto[u]intXX() in the patch of the other > thread (see pg_strto[u]intXX_check for example in [1]). The pg_strto[u]intXX_check functions can return the integer directly only because they handle errors with ereport(ERROR, ...). However, as I mentioned earlier, this is not always what the front-end utilities need to do. -- Joe Nelson https://begriffs.com
Re: Change atoi to strtol in same place
Michael Paquier wrote: > Using a wrapper in src/fe_utils makes sense. I would have used > options.c for the new file, or would that be too much generic? Sure, options.c sounds fine -- it doesn't seem any more generic than "arg_utils" and is a little simpler. The only other question I have is if the function inside -- with some adjustment in its interface -- might be useful in other contexts beyond front-end args checking. > The code indentation is weird, particularly the switch/case in > pg_strtoint64_range(). I did run pgindent... Do I need to tell it about the existence of the new file? > The error handling is awkward. Let's continue this point in your follow-up <20190911035356.ge1...@paquier.xyz>. > Why do you need to update common/Makefile? Thought I needed to do this so that other parts would link properly, but just removed the changes there and stuff still links OK, so I'll remove that change. > The builds on Windows are broken. You are adding one file into > fe_utils, but forgot to update @pgfeutilsfiles in Mkvcbuild.pm. -- Thanks for the tip, I'm still learning about the build process. Is there a service I can use to test my patches across multiple platforms? I'd rather not bother reviewers with build problems that I can catch in a more automated way. -- Joe Nelson https://begriffs.com
Re: Change atoi to strtol in same place
On Tue, Sep 10, 2019 at 08:03:32AM -0400, Robert Haas wrote:
> -1. I think it's very useful to have routines for this sort of thing
> that return an error message rather than emitting an error report
> directly. That gives the caller a lot more control.
Please let me counter-argue here. There are a couple of reasons to
not do as the patch proposes:
1) Consistency with the error messages makes less work for translators,
who already have a lot to deal with. The patch is awkward in this
sense, to give some examples:
+ if (s != PG_STRTOINT_OK)
{
- pg_log_error("invalid status interval \"%s\"", optarg);
+ pg_log_error("invalid status interval: %s", parse_error);
}
[...]
- pg_log_error("invalid compression level \"%s\"", optarg);
+ pg_log_error("invalid compression level: %s", parse_error);
2) A centralized error message can provide the same level of details.
Here are suggestions for each error status:
pg_log_error("could not parse value for option %s", optname);
pg_log_error("invalid value for option %s", optname);
optname should be defined by the caller with strings like
"-t/--timeout" or such. Then, if ranges are specified and the error
is on a range, I think that we should just add a second error message
to provide a hint to the user, if wanted by the caller of
pg_strtoint64_range() so an extra argument could do handle that:
pg_log_error("value must be in range %d..%d")
3) I think that we should not expose directly the status values of
pg_strtoint_status in pg_strtoint64_range(), that's less for module
authors to worry about, and that would be the same approach as we are
using for the wrappers of pg_strto[u]intXX() in the patch of the other
thread (see pg_strto[u]intXX_check for example in [1]).
[1]: https://www.postgresql.org/message-id/20190910030525.ga22...@paquier.xyz
--
Michael
signature.asc
Description: PGP signature
Re: Change atoi to strtol in same place
On Tue, Sep 10, 2019 at 1:36 AM Michael Paquier wrote: > The error handling is awkward. I think that you should just call > pg_log_error in pg_strtoint64_range instead of returning an error > string as you do. You could do that by passing down the option name > to the routine, and generate a new set of error messages using that. -1. I think it's very useful to have routines for this sort of thing that return an error message rather than emitting an error report directly. That gives the caller a lot more control. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Change atoi to strtol in same place
On Mon, Sep 09, 2019 at 11:02:49PM -0500, Joe Nelson wrote: > Alvaro Herrera from 2ndQuadrant wrote: > > Okay, so who is submitting a new version here? Surafel, Joe? > > I've attached a revision that uses pg_strtoint64 from str2int-10.patch > (posted in ). My patch is > based off that one and c5bc7050af. > > It covers the same front-end utilities as atoi-to-strtol-v2.patch. Other > utilities in the pg codebase still have atoi inside, but I thought I'd > share my progress to see if people like the direction. If so, I can > update the rest of the utils. > > I added a helper function to a new file in src/fe_utils, but had to > modify Makefiles in ways that might not be too clean. Maybe there's a > better place for the function. Using a wrapper in src/fe_utils makes sense. I would have used options.c for the new file, or would that be too much generic? The code indentation is weird, particularly the switch/case in pg_strtoint64_range(). The error handling is awkward. I think that you should just call pg_log_error in pg_strtoint64_range instead of returning an error string as you do. You could do that by passing down the option name to the routine, and generate a new set of error messages using that. Why do you need to update common/Makefile? The builds on Windows are broken. You are adding one file into fe_utils, but forgot to update @pgfeutilsfiles in Mkvcbuild.pm. -- Michael signature.asc Description: PGP signature
Re: Change atoi to strtol in same place
Alvaro Herrera from 2ndQuadrant wrote:
> Okay, so who is submitting a new version here? Surafel, Joe?
I've attached a revision that uses pg_strtoint64 from str2int-10.patch
(posted in ). My patch is
based off that one and c5bc7050af.
It covers the same front-end utilities as atoi-to-strtol-v2.patch. Other
utilities in the pg codebase still have atoi inside, but I thought I'd
share my progress to see if people like the direction. If so, I can
update the rest of the utils.
I added a helper function to a new file in src/fe_utils, but had to
modify Makefiles in ways that might not be too clean. Maybe there's a
better place for the function.
--
Joe Nelson https://begriffs.com
diff --git a/contrib/pg_standby/Makefile b/contrib/pg_standby/Makefile
index 0bca2f8e9e..cb9292d0f4 100644
--- a/contrib/pg_standby/Makefile
+++ b/contrib/pg_standby/Makefile
@@ -6,6 +6,8 @@ PGAPPICON = win32
PROGRAM = pg_standby
OBJS = pg_standby.o $(WIN32RES)
+PG_LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 031b1b5cd5..ce2735f3ae 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -33,6 +33,7 @@
#include "pg_getopt.h"
#include "access/xlog_internal.h"
+#include "fe_utils/arg_utils.h"
const char *progname;
@@ -678,6 +679,10 @@ main(int argc, char **argv)
while ((c = getopt(argc, argv, "cdk:lr:s:t:w:")) != -1)
{
+ pg_strtoint_status s;
+ int64 parsed;
+ char *parse_error;
+
switch (c)
{
case 'c': /* Use copy */
@@ -687,12 +692,15 @@ main(int argc, char **argv)
debug = true;
break;
case 'k': /* keepfiles */
-keepfiles = atoi(optarg);
-if (keepfiles < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -k keepfiles must be >= 0\n", progname);
+ fprintf(stderr, "%s: -k keepfiles %s\n",
+ progname, parse_error);
exit(2);
}
+keepfiles = parsed;
break;
case 'l': /* Use link */
@@ -706,31 +714,39 @@ main(int argc, char **argv)
#endif
break;
case 'r': /* Retries */
-maxretries = atoi(optarg);
-if (maxretries < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -r maxretries must be >= 0\n", progname);
+ fprintf(stderr, "%s: -r maxretries %s\n",
+ progname, parse_error);
exit(2);
}
+maxretries = parsed;
break;
case 's': /* Sleep time */
-sleeptime = atoi(optarg);
-if (sleeptime <= 0 || sleeptime > 60)
+s = pg_strtoint64_range(optarg, , 1, 60, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -s sleeptime incorrectly set\n", progname);
+ fprintf(stderr, "%s: -s sleeptime %s\n",
+ progname, parse_error);
exit(2);
}
+sleeptime = parsed;
break;
case 't': /* Trigger file */
triggerPath = pg_strdup(optarg);
break;
case 'w': /* Max wait time */
-maxwaittime = atoi(optarg);
-if (maxwaittime < 0)
+s = pg_strtoint64_range(optarg, ,
+ 0, INT_MAX, _error);
+if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "%s: -w maxwaittime incorrectly set\n", progname);
+ fprintf(stderr, "%s: -w maxwaittime %s\n",
+ progname, parse_error);
exit(2);
}
+maxwaittime = parsed;
break;
default:
fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 7986872f10..45ec25c6e0 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -31,6 +31,7 @@
#include "common/file_utils.h"
#include "common/logging.h"
#include "common/string.h"
+#include "fe_utils/arg_utils.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
@@ -2229,6 +2230,10 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
long_options, _index)) != -1)
{
+ pg_strtoint_status s;
+ int64 parsed;
+ char *parse_error;
+
switch (c)
{
case 'C':
@@ -2313,12 +2318,13 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
-compresslevel = atoi(optarg);
-if (compresslevel < 0 || compresslevel > 9)
+s = pg_strtoint64_range(optarg, , 0, 9, _error);
+if (s != PG_STRTOINT_OK)
{
- pg_log_error("invalid compression level \"%s\"", optarg);
+ pg_log_error("invalid compression level: %s", parse_error);
exit(1);
}
+compresslevel = parsed;
break;
case 'c':
if (pg_strcasecmp(optarg, "fast") == 0)
@@ -2351,12 +2357,14 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
-
Re: Change atoi to strtol in same place
Alvaro Herrera from 2ndQuadrant wrote: > Okay, so who is submitting a new version here? Surafel, Joe? Sure, I'll look into it over the weekend.
Re: Change atoi to strtol in same place
On 2019-Jul-24, Michael Paquier wrote: > The conclusion that we are reaching on the thread is to remove more > dependencies on strtol that we have in the code, and replace it with > our own, more performant wrappers. This thread makes me wondering > that we had better wait before doing this move. Okay, so who is submitting a new version here? Surafel, Joe? Waiting on Author. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Change atoi to strtol in same place
On 2019-07-24 16:57:42 +1200, David Rowley wrote: > On Wed, 24 Jul 2019 at 16:02, Joe Nelson wrote: > > > In general, I think it's a good idea to fix those places, but I wonder > > > if we need to change the error messages too. > > > > I'll leave that decision for the community to debate. I did, however, > > remove the newlines for the new error messages being passed to > > pg_log_error(). > > I'd like to put my vote not to add this complex code to each option > validation that requires an integer number. I'm not sure there > currently is a home for it, but if there was, wouldn't it be better > writing a function that takes a lower and upper bound and sets some > output param with the value and returns a bool to indicate if it's > within range or not? +many
Re: Change atoi to strtol in same place
On Wed, Jul 24, 2019 at 04:57:42PM +1200, David Rowley wrote: > I'd like to put my vote not to add this complex code to each option > validation that requires an integer number. I'm not sure there > currently is a home for it, but if there was, wouldn't it be better > writing a function that takes a lower and upper bound and sets some > output param with the value and returns a bool to indicate if it's > within range or not? Perhaps. When I see this patch calling strtol basically only for 10 as base, this reminds me of Fabien Coelho's patch refactor all the strtoint routines we have in the code: https://commitfest.postgresql.org/23/2099/ The conclusion that we are reaching on the thread is to remove more dependencies on strtol that we have in the code, and replace it with our own, more performant wrappers. This thread makes me wondering that we had better wait before doing this move. -- Michael signature.asc Description: PGP signature
Re: Change atoi to strtol in same place
On Wed, 24 Jul 2019 at 16:02, Joe Nelson wrote: > > In general, I think it's a good idea to fix those places, but I wonder > > if we need to change the error messages too. > > I'll leave that decision for the community to debate. I did, however, > remove the newlines for the new error messages being passed to > pg_log_error(). I'd like to put my vote not to add this complex code to each option validation that requires an integer number. I'm not sure there currently is a home for it, but if there was, wouldn't it be better writing a function that takes a lower and upper bound and sets some output param with the value and returns a bool to indicate if it's within range or not? -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Change atoi to strtol in same place
> Surafel Temesgen wrote:
> > we use atoi for user argument processing in same place which return
> > zero for both invalid input and input value zero. [...] in same
> > place where we accept zero as valued input it case a problem by
> > preceding for invalid input as input value zero. strtol which can
> > handle invalid input
Not only that, but atoi causes Undefined Behavior on erroneous input.
The C99 standard says this:
7.20.1 Numeric conversion functions
The functions atof, atoi, atol, and atoll need not affect the value of the
integer expression errno on an error. If the value of the result cannot be
represented, the behavior is undefined.
Tomas Vondra wrote:
> This seems to have bit-rotted (due to minor changes to pg_basebackup).
> Can you fix that and post an updated version?
I adjusted the patch to apply cleanly on a0555ddab9.
> In general, I think it's a good idea to fix those places, but I wonder
> if we need to change the error messages too.
I'll leave that decision for the community to debate. I did, however,
remove the newlines for the new error messages being passed to
pg_log_error().
As discussed in message [0], the logging functions in common/logging.c
now contain an assertion that messages do not end in newline:
Assert(fmt[strlen(fmt) - 1] != '\n');
(in pg_log_error via pg_log_generic via pg_log_generic_v)
I also added limits.h to some places it was missing, so the patch would
build.
0: https://postgr.es/m/6a609b43-4f57-7348-6480-bd022f924...@2ndquadrant.com
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 23f706b21d..2bcacbfbb5 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
@@ -638,6 +639,14 @@ sigquit_handler(int sig)
int
main(int argc, char **argv)
{
+ char *keepfilesendptr;
+ char *maxretriesendptr;
+ char *sleeptimeendptr;
+ char *maxwaittimeendptr;
+ long numkeepfiles;
+ long nummaxretries;
+ long numsleeptime;
+ long nummaxwaittime;
int c;
progname = get_progname(argv[0]);
@@ -688,12 +697,17 @@ main(int argc, char **argv)
debug = true;
break;
case 'k': /* keepfiles */
-keepfiles = atoi(optarg);
-if (keepfiles < 0)
+errno = 0;
+numkeepfiles = strtol(optarg, , 10);
+
+if (keepfilesendptr == optarg || *keepfilesendptr != '\0' ||
+ numkeepfiles < 0 || numkeepfiles > INT_MAX ||
+ errno == ERANGE)
{
- fprintf(stderr, "%s: -k keepfiles must be >= 0\n", progname);
+ fprintf(stderr, "%s: -k keepfiles must be in range %d..%d\n", progname, 0, INT_MAX);
exit(2);
}
+keepfiles = (int) numkeepfiles;
break;
case 'l': /* Use link */
@@ -707,31 +721,46 @@ main(int argc, char **argv)
#endif
break;
case 'r': /* Retries */
-maxretries = atoi(optarg);
-if (maxretries < 0)
+errno = 0;
+nummaxretries = strtol(optarg, , 10);
+
+if (maxretriesendptr == optarg || *maxretriesendptr != '\0' ||
+ nummaxretries < 0 || nummaxretries > INT_MAX ||
+ errno == ERANGE)
{
- fprintf(stderr, "%s: -r maxretries must be >= 0\n", progname);
+ fprintf(stderr, "%s: -r maxretries must be in range %d..%d\n", progname, 0, INT_MAX);
exit(2);
}
+maxretries = (int) nummaxretries;
break;
case 's': /* Sleep time */
-sleeptime = atoi(optarg);
-if (sleeptime <= 0 || sleeptime > 60)
+errno = 0;
+numsleeptime = strtol(optarg, , 10);
+
+if (sleeptimeendptr == optarg || *sleeptimeendptr != '\0' ||
+ numsleeptime <= 0 || numsleeptime > 60 ||
+ errno == ERANGE)
{
- fprintf(stderr, "%s: -s sleeptime incorrectly set\n", progname);
+ fprintf(stderr, "%s: -s sleeptime must be in range %d..%d\n", progname, 1, 59);
exit(2);
}
+sleeptime = (int) numsleeptime;
break;
case 't': /* Trigger file */
triggerPath = pg_strdup(optarg);
break;
case 'w': /* Max wait time */
-maxwaittime = atoi(optarg);
-if (maxwaittime < 0)
+errno = 0;
+nummaxwaittime = strtol(optarg, , 10);
+
+if (maxwaittimeendptr == optarg || *maxwaittimeendptr != '\0' ||
+ nummaxwaittime < 0 || nummaxwaittime > INT_MAX ||
+ errno == ERANGE)
{
- fprintf(stderr, "%s: -w maxwaittime incorrectly set\n", progname);
+ fprintf(stderr, "%s: -w maxwaittime must be in range %d..%d\n", progname, 0, INT_MAX);
exit(2);
}
+maxwaittime = (int) nummaxwaittime;
break;
default:
fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 77a7c148ba..6ed523fdd6 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -15,6 +15,7 @@
#include
#include
+#include
#include
#include
#include
@@ -2197,6 +2198,10 @@ main(int argc, char **argv)
Re: Change atoi to strtol in same place
Hi Surafel, On Mon, Jul 01, 2019 at 08:48:27PM +0300, Surafel Temesgen wrote: Hello, we use atoi for user argument processing in same place which return zero for both invalid input and input value zero. In most case its ok because we error out with appropriate error message for input zero but in same place where we accept zero as valued input it case a problem by preceding for invalid input as input value zero. The attached patch change those place to strtol which can handle invalid input regards Surafel This seems to have bit-rotted (due to minor changes to pg_basebackup). Can you fix that and post an updated version? In general, I think it's a good idea to fix those places, but I wonder if we need to change the error messages too. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
