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.
