Re: refactoring - share str2*int64 functions

On 2019-09-10 12:05:25 +0900, Michael Paquier wrote:
> On Mon, Sep 09, 2019 at 05:27:04AM -0700, Andres Freund wrote:
> > On 2019-09-09 20:57:46 +0900, Michael Paquier wrote:
> > 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).
>
> Yep, that was my line of thoughts.
>
> >> 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.
>
> Actually, I found your suggestion of using a noreturn function for the
> error reporting to be a very clean alternative. I didn't know though
> that gcc is not able to detect that a function does not return if you
> don't have a default in the switch for all the status codes. And this
> even if all the values of the enum for the switch are listed.

As I proposed they'd be in different translation units, so the compiler
wouldn't see the definition of the function, just the declaration.

> >> 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.
>
> What I think we would be looking for here is an extra argument for the
> low-level routines to control the behavior of the function in an
> extensible way, say a bits16 for a set of flags, with one flag to
> ignore checks for trailing and leading whitespace.

That'd probably be a bad idea, for performance reasons.

> Attached is an updated patch? How does it look? I have left the
> parts of readfuncs.c for now as there are more issues behind that than
> doing a single switch, short reads are one, long reads a second.

Hm? I don't know what you mean by those issues.

> And the patch already does a lot. There could be also an argument for
> having extra _check wrappers for the unsigned portions but these would
> be mostly unused in the backend code, so I have left that out on
> purpose.

I'd value consistency higher here.

> diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
> index 2c0ae395ba..8e75d52b06 100644
> --- a/src/backend/executor/spi.c
> +++ b/src/backend/executor/spi.c
> @@ -21,6 +21,7 @@
> #include "catalog/heap.h"
> #include "catalog/pg_type.h"
> #include "commands/trigger.h"
> +#include "common/string.h"
> #include "executor/executor.h"
> #include "executor/spi_priv.h"
> #include "miscadmin.h"
> @@ -2338,8 +2339,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
> CreateTableAsStmt *ctastmt = (CreateTableAsStmt *) stmt->utilityStmt;
>
> if (strncmp(completionTag, "SELECT ", 7) == 0)
> - _SPI_current->processed =
> - pg_strtouint64(completionTag + 7, NULL, 10);
> + (void) pg_strtouint64(completionTag + 7, &_SPI_current->processed);

I'd just use the checked version here, seems like a good thing to check
for, and I can't imagine it matters performance wise.

> @@ -63,8 +63,16 @@ Datum
> int2in(PG_FUNCTION_ARGS)
> {
> char *num = PG_GETARG_CSTRING(0);
> + int16 res;
> + pg_strtoint_status status;
>
> - PG_RETURN_INT16(pg_strtoint16(num));
> + /* Use a custom set of error messages here adapted to the data type */
> + status = pg_strtoint16(num, &res);

I don't know what that comment is supposed to mean?

> +/*
> + * pg_strtoint64_check
> + *
> + * Convert input string to a signed 64-bit integer.
> + *
> + * This throws ereport() upon bad input format or overflow.
> + */
> +int64
> +pg_strtoint64_check(const char *s)
> +{
> + int64 result;
> + pg_strtoint_status status = pg_strtoint64(s, &result);
> +
> + if (unlikely(status != PG_STRTOINT_OK))
> + pg_strtoint_error(status, s, "int64");
> + return result;
> +}

I think I'd just put these as inlines in the header.

Greetings,

Andres Freund