Re: refactoring - share str2*int64 functions

Hi,

On 2019-09-09 20:57:46 +0900, Michael Paquier wrote:
> > 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
>
> I am not completely sure if we should do that either anyway. Another
> approach would be to try to make the callers of the routines generate
> their own error messages. The errors we have now are really linked to
> the data types we have in core for signed integers (smallint, int,
> bigint). In most cases do they really make sense (varlena.c, pqmq.c,
> etc.)?

I think there's plenty places that ought to use the checked functions,
even if they currently don't. And typically calling in the caller will
actually be slightly less efficient, than an out-of-line function like I
was proposing above, because it'll be in-line code for infrequent code.

But ISTM all of them ought to just use the C types, rather than the SQL
types however. Since in the above proposal the caller determines the
type names, if you want a different type - like the SQL input routines -
can just invoke pg_strtoint_error() themselves (or just have it open
coded).

> And for errors which should never happen we could just use
> elog(). For the input functions of int2/4/8 we still need the
> existing errors of course.

Right, there it makes sense to continue to refer the SQL level types.

> >> 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"?
>
> I was referring to the error messages we have on HEAD in scanint8()
> and pg_strtoint16() for bad inputs and overflows.

Not the right direction imo.

> >> 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.
>
> I am not sure but what are you suggesting here?

I'm saying that we shouldn't need the whole logic of trying to parse the
string as an int, and then fail to float if it's not that. But that it's
not this patchset's task to fix this.

> >> + /* 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.
>
> Not sure about that. I would keep the scope of the patch simple as of
> now, where we make sure that we have the right interface for
> everything. There are a couple of extra improvements which could be
> done afterwards, and if we move everything in the same place that
> should be easier to move on with more improvements. Hopefully.

The only reason for thinking about it now is that we'd then avoid
changing the API twice.

Greetings,

Andres Freund