Re: extend pgbench expressions with functions
От | Fabien COELHO |
---|---|
Тема | Re: extend pgbench expressions with functions |
Дата | |
Msg-id | alpine.DEB.2.10.1602010857090.6603@sto обсуждение исходный текст |
Ответ на | Re: extend pgbench expressions with functions (Michael Paquier <michael.paquier@gmail.com>) |
Ответы |
Re: extend pgbench expressions with functions
|
Список | pgsql-hackers |
Hello Michaël, >> - remove the short macros (although IMO it is a code degradation) > > FWIW, I like this suggestion from Robert. I'm not especially found of macros, my main reserve is that because of the length of the function names this necessarily creates lines longer than 80 columns or awkward and repeated new lines within expressions, which are both ugly. > +make_op(const char *operator, PgBenchExpr *lexpr, PgBenchExpr *rexpr) > +{ > + return make_func(find_func(operator), > + /* beware that the list is > reversed in make_func */ > + make_elist(rexpr, > make_elist(lexpr, NULL))); > +} > I think that this should use as argument the function ID, aka PGBENCH_ADD > or similar instead of find_func() with an operator character. This saves a > couple of instructions. Not that simply: The number is the index in the array of functions, not the enum value, they are currently off by one, and there may be the same function with different names for some reason some day, so I do not think it would be a good idea to enforce the order so that they are identical. Also this minor search is only executed when parsing the script. > + * - nargs: number of arguments (-1 is a special value for min & max) > My fault perhaps, it may be better to mention here that -1 means that min > and max need at least one argument, and that the number of arguments is not > fixed. Ok for a better comment. > For mod() there is no need to have an error, returning 0 is fine. You can > actually do it unconditionally when rval == -1. Ok. > + setDoubleValue(retval, d < 0.0? -d: d); > Nitpick: lack of spaces between the question mark. Ok. > NONE is used nowhere, but I think that you could use it for an assertion > check here: [...] > Just replace the "none" message by Assert(type != PGBT_NONE) for example. I've added a use of the macro. > Another remaining item: should support for \setrandom be dropped? As the > patch is presented this remains intact. As you know my opinion is "yes", but I have not receive a clear go about that from a committer and I'm not motivated by removing and then re adding code to the patch. If nothing clear is said, I'll do a patch just for that one functions are added and submit it to the next CF. -- Fabien.
В списке pgsql-hackers по дате отправления: