Re: refactoring - share str2*int64 functions

Bonjour Michaël,

> FWIW, I was looking forward to putting my hands on this patch and try
> to get it merged so as we can get rid of those duplications. Here are
> some comments.
>
> +#ifdef FRONTEND
> + fprintf(stderr,
> + "invalid input syntax for type %s: \"%s\"\n",
> "bigint", str);
> +#else
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
> + errmsg("invalid input syntax for type %s: \"%s\"",
> + "bigint", str)));
> +#endif
> Have you looked at using the wrapper pg_log_error() here?

I have not.

I have simply merged the two implementations (pgbench & backend) as they
were.

> +extern bool pg_scanint8(const char *str, bool errorOK, int64 *result);
> +extern uint64 pg_strtouint64(const char *str, char **endptr, int base);

> Hmm. With this patch we have strtoint and pg_strtouint64, which makes
> the whole set inconsistent.

I understand that you mean bits vs bytes? Indeed it can bite!

> +
> #endif /* COMMON_STRING_H */
> Noise diff..

Indeed.

> numutils.c also has pg_strtoint16 and pg_strtoint32, so the locations
> become rather inconsistent with inconsistent APIs for the manipulation
> of int2 and int4 fields, and scanint8 is just a derivative of the same
> logic. We have two categories of routines here:

Yep, but the int2/4 functions are not used elsewhere.

> - The wrappers on top of strtol and strtoul & co, which are named
> respectively strtoint and pg_strtouint64 with the patch. The naming
> part is inconsistent, and we only handle uint64 and int32. We don't
> need to worry about int64 and uint32 because they are not used?

Indeed, it seems that they are not needed/used by client code, AFAICT.

> That's fine by me, but at least let's have a consistent naming.

Ok.

> Prefixing the functions with pg_* is a better practice in my opinion
> as we will unlikely run into conflicts this way.

Ok.

> - The str->integer conversion routines, which actually have very
> similar characteristics to the strtol families as they remove trailing
> whitespaces first, check for a sign, etc, except that they work only
> on base 10. And here we get into a state where pg_scanint8 should be
> actually called pg_strtoint64,

I just removed that:-)

ISTM that the issue is that the error handling of these functions is
pretty different.

> with an interface inconsistent with its int32/int16 relatives now only
> in the backend.

We can, but I'm not at ease with changing the error handling approach.

> Could we consider more consolidation here please? Like moving the whole
> set to src/common/?

My initial plan was simply to remove direct code duplications between
front-end and back-end, not to re-engineer the full set of string to int
conversion functions:-)

On the re-engineering front: Given the various points on the thread, ISTM
that there should probably be two behaviors for str to signed/unsigned
int{16,32,64}, and having only one kind of signature for all types would
be definitely better.

One low-level one that does the conversion or return an error.

Another higher-level one which possibly adds an error message (stderr for
front-end, log for back-end).

One choice is whether there are two functions (the higher one calling the
lower one and adding the messages) or just one with a boolean to trigger
the messages. I do not have a strong opinion. Maybe one function would be
simpler. Andres boolean-compatible enum return looks like a good option.

Overall, this leads to something like:

enum { STRTOINT_OK, STRTOINT_OVERFLOW_ERROR, STRTOINT_SYNTAX_ERROR }
pg_strto{,u}int{8?,16,32,64}
(const char * string, const TYPE * result, const bool verbose);

--
Fabien.