|From:||Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|To:||Teodor Sigaev <teodor(at)sigaev(dot)ru>|
|Cc:||Raúl Marín Rodríguez <rmrodriguez(at)carto(dot)com>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, PostgreSQL Developers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: [HACKERS] pgbench more operators & functions|
|Views:||Raw Message | Whole Thread | Download mbox|
> Huh, you are fast. Rebase patch during half an hour.
Hmmm... just lucky, and other after lunch tasks were more demanding.
> I haven't objection about patch idea, but I see some gotchas in coding.
> 1) /* Try to convert variable to numeric form; return false on failure */
> static bool
> makeVariableValue(Variable *var)
> as now, makeVariableValue() convert value of eny type, not only numeric
Indeed, the comments need updating. I found a few instances.
> 2) In makeVariableValue():
> if (pg_strcasecmp(var->svalue, "null") == 0)
> else if (pg_strncasecmp(var->svalue, "true", slen)
> mixing of pg_strcasecmp and pg_strNcasecmp. And, IMHO, result of
> pg_strncasecmp("tru", "true", 1) will be 0.
Yep, but it cannot be called like that because slen ==
> It may be good for 't' of 'f' but it seems too free grammar to accept
> 'tr' or 'fa' or even 'o' which actually not known to be on or off.
Yes, it really works like that. I tried to make something clearer than
"src/bin/psql/variable.c". Maybe I did not succeed.
I have added a comment and use some shortened versions in tests, with
the -D option.
> 3) It seems to me that Variable.has_value could be eliminated and then new
> PGBT_NOT_SET is added to PgBenchValueType enum as a first (=0) value. This
> allows to shorten code and make it more readable, look
> setBoolValue(&var->value, true);
> var->has_value = true;
> The second line actually doesn't needed. Although I don't insist to fix that.
I agree that the redundancy should be avoided. I tried to keep
"is_numeric" under some form, but there is no added value doing that.
> I actually prefer PGBT_NOT_SET instead of kind of PGBT_UNDEFINED, because I
I used "PGBT_NO_VALUE" which seemed clearer otherwise a variable may be
set and its value would not "not set" which would look strange.
> 4) valueTypeName()
> it checks all possible PgBenchValue.type but believes that field could
> contain some another value. In all other cases it treats as error or assert.
Ok. Treated as an internal error with an assert.
|Next Message||Robert Haas||2017-12-14 14:58:43||Re: PATCH: Exclude unlogged tables from base backups|
|Previous Message||Tom Lane||2017-12-14 14:38:39||Re: procedures and plpgsql PERFORM|