Re: refactoring - share str2*int64 functions - Mailing list pgsql-hackers

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