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-02-16 08:53:59
Message-ID: alpine.DEB.2.10.1602160851030.29962@sto
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


Hello Robert,

> [...] But we can't have things that are logically part of patch #2 just
> tossed in with patch #1.

So you want integer functions without type in patch #1.

> I was in the middle of ripping that out of the patch when I realized
> that evalFunc() is pretty badly designed.

Probably, we are just at v30:-)

> What you've done is made it the job of each particular function to
> evaluate its arguments.

Yep.

I did that for the multiple issues you raise below, and some you did not
yet foresee: handling a variable number of arguments (0, 1, 2, 3, n),
avoiding dynamic structures or arbitrary limitations, checking for a valid
number of arguments, and also the fact that the evaluation call was not
too horrible (2 lines per evaluation, factored out by groups of functions
[operators, min/max, randoms, ...], it is not fully repeated).

There are 5 sub-expression evaluation in the function, totalling 10 LOCs.

TEN.

> I don't think that's a good plan.

The one you suggest does not strike me as intrinsically better: it is a
trade between some code ugliness (5 repeated evals = 10 lines, a little
more with the double functions, probably 20 lines) to other uglinesses
(number of args limit or dynamic allocation, array length to manage and
test somehow, list to array conversion code, things that will mean far
more than the few lines of repeated code under discussion).

So I think that it is just a choice between two plans, really, the better
of which is debatable.

> I experimented with trying to do this and ran into a problem:

Yep. There are other problems, all of which solvable obviously, but which
means that a lot more that 10 lines will be added to avoid the 5*2 lines
repetition.

My opinion is that the submitted version is simple and fine for the
purpose, and the plan you suggest replaces 5*2 repetitions by a lot of
code and complexity, which is not desirable and should be avoided.

However, for obvious reasons the committer opinion prevails:-)

After considering the various points I raised above, could you confirm
that you do still require the implementation of this array approach before
I spend time doing such a thing?

> I also went over your documentation changes.

Thanks, this looks better.

--
Fabien.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro HORIGUCHI 2016-02-16 09:25:27 Re: [PROPOSAL] VACUUM Progress Checker.
Previous Message Andres Freund 2016-02-16 08:50:33 Re: Remove or weaken hints about "effective resolution of sleep delays is 10 ms"?