|From:||Michael Paquier <michael(at)paquier(dot)xyz>|
|To:||Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>|
|Cc:||alvherre(at)alvh(dot)no-ip(dot)org, bharath(dot)rupireddyforpostgres(at)gmail(dot)com, pgsql-hackers(at)lists(dot)postgresql(dot)org|
|Subject:||Re: Incorrect usage of strtol, atoi for non-numeric junk inputs|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On Wed, Jul 21, 2021 at 05:02:29PM +0900, Kyotaro Horiguchi wrote:
> The difference is your suggestion is making the function output the
> message within. I guess that the reason for the original proposal is
> different style of message is required in several places.
That's one step toward having a maximum number of frontend tools to
use the central logging APIs of src/common/.
> 1. Some "bare" options (that is not preceded by a hyphen option) like
> PID of pg_ctl kill doesn't fit the format. \pset parameters of
> pg_ctl is the same.
Yep. I was reviewing this one, but I have finished by removing it.
The argument 2 just below also came into my mind.
> 2. pg_ctl, pg_upgrade use their own error reporting mechanisms.
Yeah, for this reason I don't think that it is a good idea to switch
those areas to use the parsing of option_utils.c. Perhaps we should
consider switching pg_upgrade to have a better logging infra, but
there are also reasons behind what we have now. pg_ctl is out of
scope as it needs to cover WIN32 event logging.
> 3. parameters that take real numbers doesn't fit the scheme specifying
> range borders. For example boundary values may or may not be included
> in the range.
This concerns only pgbench, which I'd be fine to let as-is.
> 4. Most of the errors are PG_LOG_ERROR, but a few ones are
I would take it that pgbench is inconsistent with the rest. Note that
pg_dump uses fatal(), but that's just a wrapper to pg_log_error().
> loglevel specifies the loglevel to use to emit error messages. If it
> is the newly added item PG_LOG_OMIT, the function never emits an error
> message. Addition to that, the return type is changed to an enum which
> indicates what trouble the given string has. The caller can print
> arbitrary error messages consulting the value. (killproc in pg_ctl.c)
I am not much a fan of that. If we do so, what's the point in having
a dependency to logging.c anyway in option_utils.c? This OMIT option
only exists to bypass the specific logging needs where this gets
added. That does not seem a design adapted to me in the long term,
neither am I a fan of specific error codes for a code path that's just
going to be used to parse command options.
> I added two more similar functions option_parse_long/double. The
> former is a clone of _int. The latter doesn't perform explicit range
> checks for the reason described above.
These have a limited impact, so I would limit things to int32 for
> Maybe we need to make pg_upgrade use the common-logging features
> instead of its own, but it is not included in this patch.
Maybe. That would be good in the long term, though its case is very
> The attached patch needs more polish but should be enough to tell what
> I have in my mind.
This breaks some of the TAP tests of pgbench and pg_dump, at short
The checks for the port value in pg_receivewal and pg_recvlogical is
strange to have. We don't care about that in any other tools.
The number of checks for --jobs and workers could be made more
consistent across the board, but I have let that out for now.
Hacking on that, I am finishing with the attached. It is less
ambitious, still very useful as it removes a dozen of custom error
messages in favor of the two ones introduced in option_utils.c. On
top of that this reduces a bit the code:
15 files changed, 156 insertions(+), 169 deletions(-)
What do you think?
|Next Message||Ronan Dunklau||2021-07-21 12:28:30||Re: ORDER BY pushdowns seem broken in postgres_fdw|
|Previous Message||John Naylor||2021-07-21 11:44:06||Re: add 'noError' to euc_tw_and_big5.c|