Re: POC: rational number type (fractions)
> > I think we mark this as rejected. Stephen Frost wrote: > The more we reject new things, the less appealing our community ends > up being. For what it's worth, I'm not disheartened if my rational patch is rejected. I can appreciate that postgres wants to avoid what might be feature creep, especially if aspects of the implementation are arbitrary or subject to change later on. It might be more productive for me to investigate other ways to contribute, like SQL:2016 features/conformance. That would increase our harmony with other databases, rather than adding idiosyncrasies like a new numeric type.
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: POC: rational number type (fractions)
Joe Nelson wrote: > where the denominator is made positive whenever possible (not possible > when it's -INT_MAX). (I meant INT_MIN rather than -INT_MAX.) Another more-than-one-way-to-do-it task is converting a float to a fraction. I translated John Kennedy's method [0] to C, but Github user adegert sent an alternative [1] that matches the way the CPython implementation works. 0: https://begriffs.com/pdf/dec2frac.pdf 1: https://github.com/begriffs/pg_rational/pull/13
Re: POC: rational number type (fractions)
floats eats up precision pretty quickly. 2: https://en.wikipedia.org/wiki/Mediant_(mathematics) > * Can you discuss how cross-type comparisons and conversions should be > handled (e.g. int8, numeric, float8)? Good point, I don't have tests for that. Would implicit casts do the trick? So '1/2'::rational < 1 would cast 1 to '1/1' and compare? I have currently included these casts: integer -> rational, float8 <-> rational. Don't have one for numeric yet. > Regards, Thank you for taking the time to raise those questions. -- Joe Nelson https://begriffs.com
POC: rational number type (fractions)
Hi hackers, attached is a proof of concept patch adding a new base type
called "rational" to represent fractions. It includes arithmetic,
simplification, conversion to/from float, finding intermediates with a
stern-brocot tree, custom aggregates, and btree/hash indices.
The primary motivation was as a column type to support user-defined
ordering of rows (with the ability to dynamically rearrange rows). The
postgres wiki has a page [0] about this using pairs of integers to
represent fractions, but it's not particularly elegant.
I wrote about options for implementing user-defined order in an article
[1] and ended up creating a postgres extension, pg_rational [2], to
provide the new type. People have been using the extension, but told me
they wished they could use it on hosted platforms like Amazon RDS which
have a limited set of whitelisted extensions. Thus I'm submitting this
patch to discuss getting the feature in core postgres.
For usage, see the included regression test. To see how it works for the
user-defined order use case see my article. I haven't included docs in
the patch since the interface may change with community feedback.
0: https://wiki.postgresql.org/wiki/User-specified_ordering_with_fractions
1: https://begriffs.com/posts/2018-03-20-user-defined-order.html
2: https://github.com/begriffs/pg_rational
--
Joe Nelson https://begriffs.com
diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
index 13efa9338c..10bdcff5dc 100644
--- a/src/backend/utils/adt/Makefile
+++ b/src/backend/utils/adt/Makefile
@@ -80,6 +80,7 @@ OBJS = \
rangetypes_selfuncs.o \
rangetypes_spgist.o \
rangetypes_typanalyze.o \
+ rational.o \
regexp.o \
regproc.o \
ri_triggers.o \
diff --git a/src/backend/utils/adt/rational.c b/src/backend/utils/adt/rational.c
new file mode 100644
index 00..ab91198da3
--- /dev/null
+++ b/src/backend/utils/adt/rational.c
@@ -0,0 +1,637 @@
+/*-
+ *
+ * rational.c
+ * Rational number data type for the Postgres database system
+ *
+ * Copyright (c) 1998-2020, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * src/backend/utils/adt/rational.c
+ *
+ *-
+ */
+
+#include "postgres.h"
+#include "fmgr.h"
+#include "access/hash.h"
+#include "common/int.h" /* portable overflow detection */
+#include "libpq/pqformat.h" /* send/recv functions */
+#include
+#include
+
+PG_MODULE_MAGIC;
+
+typedef struct
+{
+ int32 numer;
+ int32 denom;
+} Rational;
+
+static int32 gcd(int32, int32);
+static bool simplify(Rational *);
+static int32 cmp(Rational *, Rational *);
+static void neg(Rational *);
+static Rational * create(long long, long long);
+static Rational * add(Rational *, Rational *);
+static Rational * mul(Rational *, Rational *);
+static void mediant(Rational *, Rational *, Rational *);
+
+/*
+ * IO **
+ */
+
+PG_FUNCTION_INFO_V1(rational_in);
+PG_FUNCTION_INFO_V1(rational_in_float);
+PG_FUNCTION_INFO_V1(rational_out);
+PG_FUNCTION_INFO_V1(rational_out_float);
+PG_FUNCTION_INFO_V1(rational_recv);
+PG_FUNCTION_INFO_V1(rational_create);
+PG_FUNCTION_INFO_V1(rational_in_integer);
+PG_FUNCTION_INFO_V1(rational_send);
+
+Datum
+rational_in(PG_FUNCTION_ARGS)
+{
+ char *s = PG_GETARG_CSTRING(0),
+ *after;
+ long long n,
+d;
+
+ if (!isdigit(*s) && *s != '-')
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("Missing or invalid numerator")));
+
+ n = strtoll(s, , 10);
+
+ if (*after == '\0')
+ {
+ /* if just a number and no slash, interpret as an int */
+ d = 1;
+ }
+ else
+ {
+ /* otherwise look for denominator */
+ if (*after != '/')
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("Expecting '/' after number but found '%c'", *after)));
+ if (*(++after) == '\0')
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("Expecting value after '/' but got '\\0'")));
+
+ d = strtoll(after, , 10);
+ if (*after != '\0')
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("Expecting '\\0' but found '%c'", *after)));
+ }
+ PG_RETURN_POINTER(create(n, d));
+}
+
+/*
+ This function taken from John Kennedy's paper, "Algorithm To Convert a
+ Decimal to a Fraction." Translated from Pascal.
+*/
+Datum
+rational_in_float(PG_FUNCTION_ARGS)
+{
+ float8 target = PG_GETARG_FLOAT8(0),
+z,
+fnumer,
+fdenom,
+error;
+ int32 prev_denom,
+sign;
+ Rational *result = palloc(sizeof(Rational));
+
+ if (target == (int32) target)
+ {
+ result->numer = (int32) target;
+ result->denom = 1;
+ PG_RETURN_POINTER(result);
+ }
+
+ sign = target < 0.0 ? -1 : 1;
+ target = fabs(target);
+
+ if
Re: refactoring - standardize integer parsing in front-end utilities
Joe Nelson wrote: > > I see this patch has been moved to the next commitfest. What's the next > > step, does it need another review? Peter Eisentraut wrote: > I think you need to promote your patch better. Thanks for taking the time to revive this thread. Quick sales pitch for the patch: * Standardizes bounds checking and error message format in utilities pg_standby, pg_basebackup, pg_receivewal, pg_recvlogical, pg_ctl, pg_dump, pg_restore, pg_upgrade, pgbench, reindexdb, and vacuumdb * Removes Undefined Behavior caused by atoi() as described in the C99 standard, section 7.20.1 * Unifies integer parsing between the front- and back-end using functions provided in https://commitfest.postgresql.org/25/2272/ In reality I doubt my patch is fixing any pressing problem, it's just a small refactor. > The thread subject and the commit fest entry title are somewhat > nonsensical and no longer match what the patch actually does. I thought changing the subject line might be discouraged, but since you suggest doing it, I just did. Updated the title of the commitfest entry https://commitfest.postgresql.org/26/2197/ as well. > The patch contains no commit message Does this list not accept plain patches, compatible with git-apply? (Maybe your point is that I should make it as easy for committers as possible, and asking them to invent a commit message is just extra work.) > and no documentation or test changes The interfaces of the utilities remain the same. Same flags. The only change would be the error messages produced for incorrect values. The tests I ran passed successfully, but perhaps there were others I didn't try running and should have. > 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. True. I think the code looks nicer and the errors are more informative, but I'll leave it in the community's hands to determine if this is something they want. Once again, I appreciate your taking the time to help me with this process.
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: Proposal: Make use of C99 designated initialisers for nulls/values arrays
> Is it possible to define the macro to be {} where supported and {0}
> where needed? Something like:
If it's being put behind a macro then *stylistically* it shouldn't
matter whether {} or {0} is chosen, right? In which case {0} would
be a better choice because it's supported everywhere.
Re: Proposal: Make use of C99 designated initialisers for nulls/values arrays
> > I don't understand why this is an issue worth deviating from the
> > standard for.
>
> Because this use and the way the standard is defined in this case is
> confusing and could lead later hackers to misunderstand what's going on
> and end up creating bugs-
The two possible misunderstandings seem to be:
1. how 0 is interpreted in various contexts such as bool
2. that the x in {x} applies to only the first element
IMHO we should expect people to be familiar with (1), and we have the
INIT_ALL_ELEMS_ZERO macro to avoid (2). However the more I look at the
code using that macro the less I like it. The {0} initializer is more
idiomatic.
My vote would be to use {0} everywhere and avoid constructions like
{false} which might exacerbate misunderstanding (2).
> I figured it was common knowledge that gcc/clang supported it just fine,
> which covers something like 90% of the buildfarm. I haven't got easy
> access to check others.
As Amit pointed out, {} doesn't work with MSVC-2017, nor is there any
reason it should, given that it isn't part of the C standard.
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;
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
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:
Re: Proposal: Make use of C99 designated initialisers for nulls/values arrays
Amit Kapila wrote:
> > How about I just define them both the same?
> > #define INIT_ALL_ELEMS_ZERO {0}
> > #define INIT_ALL_ELEMS_FALSE{0}
>
> I think using one define would be preferred, but you can wait and see
> if others prefer defining different macros for the same thing.
+1 on using INIT_ALL_ELEMS_ZERO everywhere, even for bool[]. You may
worry, "Should we really be assigning 0 to something of type bool? Is
that the wrong type or a detail that varies by implementation?" It's
safe though, the behavior is guaranteed to be correct by section 7.16 of
the C99 spec, which says that bool, true, and false are always macros
for _Bool, 1, and 0 respectively.
One might argue that INIT_ALL_ELEMS_FALSE as a synonym for
INIT_ALL_ELEMS_ZERO is good for readability in the same way that "false"
is for 0. However I want to avoid creating the impression that there is,
or can be, a collection of INIT_ALL_ELEMS_xxx macros invoking different
initializer behavior.
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: Proposal: Make use of C99 designated initialisers for nulls/values arrays
> If so, I don't suppose it's possible to give empty braces:
>
> bool nulls[Natts_pg_attribute] = {};
GNU does add this capability as a nonstandard language extension, but
according to the C99 standard, no.
Re: Proposal: Make use of C99 designated initialisers for nulls/values arrays
Isaac Morland wrote:
> I hope you'll forgive a noob question. Why does the "After"
> initialization for the boolean array have {0} rather than {false}?
I think using a value other than {0} potentially gives the incorrect
impression that the value is used for *all* elements of the
array/structure, whereas it is only used for the first element. "The
remainder of the aggregate shall be initialized implicitly the same as
objects that have static storage duration."
The rest of the elements are being initialized to zero as interpreted by
their types (so NULL for pointers, 0.0 for floats, even though neither
of them need be bitwise zero). Setting the first item to 0 matches that
exactly.
Using {false} may encourage the unwary to try
bool foo[2] = {true};
which will not set all elements to true.
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(op
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
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
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_erro
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: Optimization of some jsonb functions
Thomas Munro wrote:
> This doesn't apply -- to attract reviewers, could we please have a rebase?
To help the review go forward, I have rebased the patch on 27cd521e6e.
It passes `make check` for me, but that's as far as I've verified the
correctness.
I squashed the changes into a single patch, sorry if that makes it
harder to review than the original set of five patch files...
--
Joe Nelson https://begriffs.com
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index 69f41ab455..8dced4ef6c 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -1873,9 +1873,7 @@ jsonb_object_agg_finalfn(PG_FUNCTION_ARGS)
bool
JsonbExtractScalar(JsonbContainer *jbc, JsonbValue *res)
{
- JsonbIterator *it;
- JsonbIteratorToken tok PG_USED_FOR_ASSERTS_ONLY;
- JsonbValue tmp;
+ JsonbValue *scalar PG_USED_FOR_ASSERTS_ONLY;
if (!JsonContainerIsArray(jbc) || !JsonContainerIsScalar(jbc))
{
@@ -1884,25 +1882,8 @@ JsonbExtractScalar(JsonbContainer *jbc, JsonbValue *res)
return false;
}
- /*
- * A root scalar is stored as an array of one element, so we get the array
- * and then its first (and only) member.
- */
- it = JsonbIteratorInit(jbc);
-
- tok = JsonbIteratorNext(, , true);
- Assert(tok == WJB_BEGIN_ARRAY);
- Assert(tmp.val.array.nElems == 1 && tmp.val.array.rawScalar);
-
- tok = JsonbIteratorNext(, res, true);
- Assert(tok == WJB_ELEM);
- Assert(IsAJsonbScalar(res));
-
- tok = JsonbIteratorNext(, , true);
- Assert(tok == WJB_END_ARRAY);
-
- tok = JsonbIteratorNext(, , true);
- Assert(tok == WJB_DONE);
+ scalar = getIthJsonbValueFromContainer(jbc, 0, res);
+ Assert(scalar);
return true;
}
diff --git a/src/backend/utils/adt/jsonb_op.c b/src/backend/utils/adt/jsonb_op.c
index a64206eeb1..82c4b0b2cb 100644
--- a/src/backend/utils/adt/jsonb_op.c
+++ b/src/backend/utils/adt/jsonb_op.c
@@ -24,6 +24,7 @@ jsonb_exists(PG_FUNCTION_ARGS)
Jsonb *jb = PG_GETARG_JSONB_P(0);
text *key = PG_GETARG_TEXT_PP(1);
JsonbValue kval;
+ JsonbValue vval;
JsonbValue *v = NULL;
/*
@@ -38,7 +39,7 @@ jsonb_exists(PG_FUNCTION_ARGS)
v = findJsonbValueFromContainer(>root,
JB_FOBJECT | JB_FARRAY,
- );
+ , );
PG_RETURN_BOOL(v != NULL);
}
@@ -59,6 +60,7 @@ jsonb_exists_any(PG_FUNCTION_ARGS)
for (i = 0; i < elem_count; i++)
{
JsonbValue strVal;
+ JsonbValue valVal;
if (key_nulls[i])
continue;
@@ -69,7 +71,7 @@ jsonb_exists_any(PG_FUNCTION_ARGS)
if (findJsonbValueFromContainer(>root,
JB_FOBJECT | JB_FARRAY,
- ) != NULL)
+ , ) != NULL)
PG_RETURN_BOOL(true);
}
@@ -92,6 +94,7 @@ jsonb_exists_all(PG_FUNCTION_ARGS)
for (i = 0; i < elem_count; i++)
{
JsonbValue strVal;
+ JsonbValue valVal;
if (key_nulls[i])
continue;
@@ -102,7 +105,7 @@ jsonb_exists_all(PG_FUNCTION_ARGS)
if (findJsonbValueFromContainer(>root,
JB_FOBJECT | JB_FARRAY,
- ) == NULL)
+ , ) == NULL)
PG_RETURN_BOOL(false);
}
diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index ac04c4a57b..05e1c18472 100644
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -57,6 +57,8 @@ static void appendValue(JsonbParseState *pstate, JsonbValue *scalarVal);
static void appendElement(JsonbParseState *pstate, JsonbValue *scalarVal);
static int lengthCompareJsonbStringValue(const void *a, const void *b);
static int lengthCompareJsonbPair(const void *a, const void *b, void *arg);
+static int lengthCompareJsonbString(const char *val1, int len1,
+ const char *val2, int len2);
static void uniqueifyJsonbObject(JsonbValue *object);
static JsonbValue *pushJsonbValueScalar(JsonbParseState **pstate,
JsonbIteratorToken seq,
@@ -297,6 +299,102 @@ compareJsonbContainers(JsonbContainer *a, JsonbContainer *b)
return res;
}
+/* Find scalar element in Jsonb array and return it. */
+static JsonbValue *
+findJsonbElementInArray(JsonbContainer *container, JsonbValue *elem,
+ JsonbValue *res)
+{
+ JsonbValue *result;
+ JEntry *children = container->children;
+ int count = JsonContainerSize(container);
+ char *baseAddr = (char *) (children + count);
+ uint32 offset = 0;
+ int i;
+
+ result = res ? res : palloc(sizeof(*result));
+
+ for (i = 0; i < count; i++)
+ {
+ fillJsonbValue(container, i, baseAddr, offset, result);
+
+ if (elem->type == result->type &&
+ equalsJsonbScalarValue(elem, result))
+ return result;
+
+ JBE_ADVANCE_OFFSET(offset, children[i]);
+ }
+
+ if (!res)
+ pfree(result);
+
+ return NULL;
+}
+
+/* Find value by key in Jsonb object and fetch it into 'res'. */
+JsonbValue *
+findJsonbKeyInObject(JsonbContainer *container, const char *keyVal, int keyLen,
+ JsonbValue *res)
+{
+ JEntry *children = container->children;
+ JsonbValue *result = res;
+ int count = JsonContainerSize(container);
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)
