Re: refactoring - share str2*int64 functions

Hi,

On 2019-09-09 14:28:14 +0900, Michael Paquier wrote:
> On Thu, Sep 05, 2019 at 03:52:48PM +0900, Michael Paquier wrote:
> > Right, there was this part.  This brings also the point of having one
> > interface for the backend as all the error messages for the backend
> > are actually the same, with the most simple name being that:
> > pg_strtoint(value, size, error_ok).

I *VEHEMENTLY* oppose the introduction of any future pseudo-generic
routines taking the type width as a parameter. They're weakening the
type system and they're unnecessarily inefficient.


I don't really buy that saving a few copies of a strings is worth that
much. But if you really want to do that, the right approach imo would be
to move the error reporting into a separate function. I.e. something

void pg_attribute_noreturn()
pg_strtoint_error(pg_strtoint_status status, const char *input, const char *type)

that you'd call in small wrappers. Something like

static inline int32
pg_strtoint32_check(const char* s)
{
    int32 result;
    pg_strtoint_status status = pg_strtoint32(s, &result);

    if (unlikely(status == PG_STRTOINT_OK))
       pg_strtoint_error(status, s, "int32");
    return result;
}


> So, it seems to me that if we want to have a consolidation of
> everything, we basically need to have the generic error messages for
> the backend directly into common/string.c where the routines are
> refactored, and I think that we should do the following based on the
> patch attached:

> - Just remove pg_strtoint(), and move the error generation logic in
> common/string.c.

I'm not quite sure what you mean by moving the "error generation logic"?


> - Add an error_ok flag for all the pg_strto[u]int{16|32|64} routines,
> which generates errors only for FRONTEND is not defined.

I think this is a bad idea.


> - Use pg_log_error() for the frontend errors.
>
> If those errors are added everywhere, we would basically have no code
> paths in the frontend or the backend (for the unsigned part only)
> which use them yet.  Another possibility would be to ignore the
> error_ok flag in those cases, but that's inconsistent.

Yea, ignoring it would be terrible idea.

> diff --git a/src/backend/libpq/pqmq.c b/src/backend/libpq/pqmq.c
> index a9bd47d937..593a5ef54e 100644
> --- a/src/backend/libpq/pqmq.c
> +++ b/src/backend/libpq/pqmq.c
> @@ -286,10 +286,10 @@ pq_parse_errornotice(StringInfo msg, ErrorData *edata)
>                  edata->hint = pstrdup(value);
>                  break;
>              case PG_DIAG_STATEMENT_POSITION:
> -                edata->cursorpos = pg_strtoint32(value);
> +                edata->cursorpos = pg_strtoint(value, 4);
>                  break;

I'd be really upset if this type of change went in.



>  #include "fmgr.h"
>  #include "miscadmin.h"
> +#include "common/string.h"
>  #include "nodes/extensible.h"
>  #include "nodes/parsenodes.h"
>  #include "nodes/plannodes.h"
> @@ -80,7 +81,7 @@
>  #define READ_UINT64_FIELD(fldname) \
>      token = pg_strtok(&length);        /* skip :fldname */ \
>      token = pg_strtok(&length);        /* get field value */ \
> -    local_node->fldname = pg_strtouint64(token, NULL, 10)
> +    (void) pg_strtouint64(token, &local_node->fldname)

Seems like these actually could just ought to use the error-checked
variants. And I think it ought to change all of
READ_{INT,UINT,LONG,UINT64,OID}_FIELD, rather than just redirecting one
of them to the new routines.


>  static void pcb_error_callback(void *arg);
> @@ -496,7 +496,7 @@ make_const(ParseState *pstate, Value *value, int location)
>
>          case T_Float:
>              /* could be an oversize integer as well as a float ... */
> -            if (scanint8(strVal(value), true, &val64))
> +            if (pg_strtoint64(strVal(value), &val64) == PG_STRTOINT_OK)
>              {
>                  /*
>                   * It might actually fit in int32. Probably only INT_MIN can

Not for this change, but we really ought to move away from this crappy
logic. It's really bonkers to have T_Float represent large integers and
floats.



> +/*
> + * pg_strtoint16
> + *
> + * Convert input string to a signed 16-bit integer.  Allows any number of
> + * leading or trailing whitespace characters.
> + *
> + * NB: Accumulate input as a negative number, to deal with two's complement
> + * representation of the most negative number, which can't be represented as a
> + * positive number.
> + *
> + * The function returns immediately if the conversion failed with a status
> + * value to let the caller handle the error.  On success, the result is
> + * stored in "*result".
> + */
> +pg_strtoint_status
> +pg_strtoint16(const char *s, int16 *result)
> +{
> +    const char *ptr = s;
> +    int16        tmp = 0;
> +    bool        neg = false;
> +
> +    /* skip leading spaces */
> +    while (likely(*ptr) && isspace((unsigned char) *ptr))
> +        ptr++;
> +
> +    /* handle sign */
> +    if (*ptr == '-')
> +    {
> +        ptr++;
> +        neg = true;
> +    }
> +    else if (*ptr == '+')
> +        ptr++;

> +    /* require at least one digit */
> +    if (unlikely(!isdigit((unsigned char) *ptr)))
> +        return PG_STRTOINT_SYNTAX_ERROR;

Wonder if there's an argument for moving this behaviour to someplace
else - in most cases we really don't expect whitespace, and checking for
it is unnecessary overhead.

Greetings,

Andres Freund