Re: extend pgbench expressions with functions

From: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL Developers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: extend pgbench expressions with functions
Date: 2016-03-09 08:09:27
Message-ID: alpine.DEB.2.10.1603090822170.25393@sto
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


Hello Robert,

Here is a v35 b & c.

> This is not acceptable:
>
> + /* guess double type (n for "inf",
> "-inf" and "nan") */
> + if (strchr(var, '.') != NULL ||
> strchr(var, 'n') != NULL)
> + {
> + double dv;
> + sscanf(var, "%lf", &dv);
> + setDoubleValue(retval, dv);
> + }
> + else
> + setIntValue(retval, strtoint64(var));
>
> That totally breaks the error handling which someone carefully established here.

I'm not sure what is "not acceptable" as it "totally breaks the error
handling" in the above code.

I assumed that you want to check that sscanf can read what sprintf
generated when handling "\set". I'd guess that libc would be broken if it
was the case. I've made pgbench check that it is not.

If it was something else, you have to spell it out for me.

> Extra space. [...] Another extra space.

Indeed.

> - int nargs = 0;
> - int64 iargs[MAX_FARGS];
> - PgBenchExprLink *l = args;
> + int nargs = 0;
> + PgBenchValue vargs[MAX_FARGS];
> + PgBenchExprLink *l = args;
>
> Completely unnecessary reindentation of the first and third lines.

It just looked better to me.

> + setIntValue(retval, i < 0? -i: i);
>
> Not project style.

Indeed.

> Please fix the whitespace damage git diff --check complains about,

"git diff -check" does not seem to complain on the full v35-b.

> and check for other places where you haven't followed project style.

I've found some more instances of "& ref".

--
Fabien.

Attachment Content-Type Size
pgbench-funcs-35-b.patch text/x-diff 35.2 KB
pgbench-funcs-35-c.patch text/x-diff 8.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Etsuro Fujita 2016-03-09 08:20:26 Re: Minor documentation tweak to GetForeignPlan documentation
Previous Message pokurev 2016-03-09 07:54:27 Re: [PROPOSAL] VACUUM Progress Checker.