Thread: pgbench - test whether a variable exists
Hello devs, While investigating moving pgbench expressions to fe_utils so that they can be shared with psql (\if ..., \let ?), I figure out that psql's \if has a syntax to test whether a variable exists, which is not yet available to pgbench. This patch adds the same syntax to pgbench expression so that they can represent this expression, already accepted by psql's \if. Note that it is not really that useful for benchmarking, although it does not harm. -- Fabien.
Attachment
Hi, On 2018-02-19 19:23:04 +0100, Fabien COELHO wrote: > Note that it is not really that useful for benchmarking, although it does > not harm. That seems plenty reason to plainly reject this patch? If we end up unifying it'll be added via that, no? - Andres
>> Note that it is not really that useful for benchmarking, although it does >> not harm. > > That seems plenty reason to plainly reject this patch? If we end up > unifying it'll be added via that, no? On the contrary, my point is to add the feature beforehand so that it is not intermixed in the large refactoring/renamings/interfacing involved in moving pgbench expression (lexer, parser, execution) engine to front-end utils. -- Fabien.
re. pg_strndup() the following places could use it (skim of a very quick grep for pg_strdup): src/bin/pg_waldump/pg_waldump.c: *dir = pg_strdup(path); src/bin/pg_waldump/pg_waldump.c- (*dir)[(sep - path) + 1] = '\0'; /* no strndup */ src/bin/psql/prompt.c: name = pg_strdup(p + 1); src/bin/psql/prompt.c- nameend = strcspn(name, ":"); src/bin/psql/prompt.c- name[nameend] = '\0'; src/bin/scripts/common.c: *table = pg_strdup(spec); src/bin/scripts/common.c- (*table)[cp - spec] = '\0'; /* no strndup */ Not a lot. It takes more code to add pg_strndup() that we would save by adding it. .oO(I wonder why strcspn and not strrchr ...) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> While investigating moving pgbench expressions to fe_utils so that they can > be shared with psql (\if ..., \let ?), I figure out that psql's \if has a > syntax to test whether a variable exists, which is not yet available to > pgbench. > > This patch adds the same syntax to pgbench expression so that they can > represent this expression, already accepted by psql's \if. > > Note that it is not really that useful for benchmarking, although it does not > harm. Patch v2 is a rebase. -- Fabien.
Attachment
> Patch v2 is a rebase. Patch v3 is also a rebase. -- Fabien.
Attachment
On Tue, Mar 27, 2018 at 02:08:31PM +0200, Fabien COELHO wrote: >> Patch v2 is a rebase. > > Patch v3 is also a rebase. This has rotten for half a year, so I am marking it as returned with feedback. There have been comments from Alvaro and Andres as well... -- Michael
Attachment
Bonjour Michaël,
>> Patch v3 is also a rebase.
>
> This has rotten for half a year, so I am marking it as returned with
> feedback. There have been comments from Alvaro and Andres as well...
Attached a v4. I'm resurrecting this small patch, after "\aset" has been
added to pgbench (9d8ef988).
Alvaro's feedback was about the lack of "pg_strndup" availability, but he
concluded that it would seldom be used if available, so it was not worth
the effort to add it for the sake of this small patch.
About arguments for this patch:
First, this syntax is already available in "psql", and I think that
keeping pgbench/psql in sync is better for users' ease of mind. That is
the initial (weak) argument about which Andres objected.
Second, it is on the path to move pgbench expression as a front-end util
that can be used by psql, which is still a project of mine, although I
have not started much on that yet. For that pgbench expressions must be
able to do what psql can do before merging, including this test. Other
things needed before this is stated are a working free on expression trees
(trivial), merging variable management to some extent (at least the same
API, possibly the same implementation would save quite a few lines of
code), having string values, support for :'var' and :"var" string escapes…
no very big deals, but some work anyway.
Third, I have a practical pgbench-specific use case, which motivates the
resurrection right now: I'd like to be able to run a benchmark with a mix
of SELECT, UPDATE, DELETE and INSERT commands, that is expected in a
normal functioning database system.
For INSERT, I think I have a few ideas for possible and simple solutions,
but it still need some thoughts so this is for later. The key issue is
how to handle a varying number of rows.
Under DELETE, some SELECT and UPDATE scripts may fail because the data are
not there anymore, hence the ability to check whether a variable is empty
comes handy:
Low probability delete script:
\set uid random(...)
DELETE FROM Operations WHERE oid IN (... :uid) \;
DELETE FROM Accounts WHERE aid IN (... :uid) \;
DELETE FROM Client WHERE ... :uid;
Parallel running update script:
-- a pseudo random client arrives
\set uname random(...)
SELECT uid FROM Client WHERE uname = :uname::TEXT \aset
-- remainder must be skipped if no client was found
\if :{?uid}
SELECT SUM(abalance) FROM Account WHERE uid = :uid ...
-- if the balance is right, withdrawing is possible...
...
\else
-- skip silently, the client has left!
\endif
--
Fabien.
Attachment
On Mon, Apr 13, 2020 at 09:54:01AM +0200, Fabien COELHO wrote:
> Attached a v4. I'm resurrecting this small patch, after "\aset" has been
> added to pgbench (9d8ef988).
Hmm. It seems to me that this stuff needs to be more careful with the
function handling? For example, all those cases fail but they
directly pass down a variable that may not be defined, so shouldn't
those results be undefined as well instead of failing:
\set g double(:{?no_such_variable})
\set g exp(:{?no_such_variable})
\set g greatest(:{?no_such_variable}, :{?no_such_variable})
\set g int(:{?no_such_variable})
It seems to me that there could be a point in having the result of any
function to become undefined if using at least one undefined argument
(the point could be made as well that things like greatest just ignore
conditioned variables), so I was surprised to not see the logic more
linked with ENODE_VARIABLE. If your intention is to keep this
behavior, it should at least be tested I guess. Please note that this
patch will have to wait until v14 opens for business for more
comments. :p
--
Michael
Attachment
Bonjour Michaël,
> Hmm. It seems to me that this stuff needs to be more careful with the
> function handling? For example, all those cases fail but they directly
> pass down a variable that may not be defined, so shouldn't those results
> be undefined as well instead of failing:
> \set g double(:{?no_such_variable})
> \set g exp(:{?no_such_variable})
> \set g greatest(:{?no_such_variable}, :{?no_such_variable})
> \set g int(:{?no_such_variable})
I do not understand: All the above examples are type errors, as ":{?var}"
is a boolean, that cannot be cast to double, be exponentiated etc. So
failing just seems appropriate.
Maybe casting boolean to int should be allowed, though, as pg does it.
> It seems to me that there could be a point in having the result of any
> function to become undefined if using at least one undefined argument
> (the point could be made as well that things like greatest just ignore
> conditioned variables), so I was surprised to not see the logic more
> linked with ENODE_VARIABLE.
Hmmm… :var (ENODE_VARIABLE) replaces de variable by its value, and it
fails if the variable is not defined, which is the intention, hence the
point of having the ability to test whether a variable exists, and its new
expression node type.
It could replace it by NULL when not existing, but ISTM that a script can
wish to distinguish NULL and undefined, and it departs from SQL which just
fails on a undefined alias or column or whatever.
If someone wants to go back to having a self propagating NULL, they can
simply
\if :{?var}
\set var NULL
\endif
Or
\set var CASE WHEN :{?var} THEN :var ELSE NULL END
to get it.
Having some special UNDEF value looks unattractive, as it would depart for
SQL even further.
> If your intention is to keep this behavior, it should at least be tested
> I guess.
My intention is to keep failing on type errors, but maybe I'm missing
something of your point.
> Please note that this patch will have to wait until v14 opens
> for business for more.
Sure. I put it in the July CF, and I do not expect to it to be processed
on the first CF it appears in.
--
Fabien.
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: tested, failed
Documentation: not tested
I am not very convinced to have that, but I have performed some testing on master ().
I found a crash
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `pgbench postgres -T 60 -f a.sql'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x000055bfb572a4b2 in expr_yylex (yylval_param=0x7fff29a60870, yyscanner=0x55bfb6867a90) at exprscan.c:1107
1107 *yy_cp = yyg->yy_hold_char;
(gdb) bt
#0 0x000055bfb572a4b2 in expr_yylex (yylval_param=0x7fff29a60870, yyscanner=0x55bfb6867a90) at exprscan.c:1107
#1 0x000055bfb572cb63 in expr_lex_one_word (state=0x55bfb6867a10, word_buf=0x7fff29a608d0, offset=0x7fff29a608a8) at
exprscan.l:340
#2 0x000055bfb57358fd in process_backslash_command (sstate=0x55bfb6867a10, source=0x55bfb68653a0 "a.sql") at
pgbench.c:4540
#3 0x000055bfb5736528 in ParseScript (
script=0x55bfb68655f0 "\\set nbranches :scale\n\\set ntellers 10 * :scale\n\\set naccounts 100000 *
:scale\n\\setrandomaid 1 :naccounts\n\\setrandom bid 1 :nbranches\n\\setrandom tid 1 :ntellers\n\\setrandom delta -5000
5000\nBEGIN;\nUPD"...,desc=0x55bfb68653a0 "a.sql", weight=1) at pgbench.c:4826
#4 0x000055bfb5736967 in process_file (filename=0x55bfb68653a0 "a.sql", weight=1) at pgbench.c:4962
#5 0x000055bfb5738628 in main (argc=6, argv=0x7fff29a610d8) at pgbench.c:5641
The new status of this patch is: Waiting on Author
On 20.10.2020 15:22, Ibrar Ahmed wrote: > The following review has been posted through the commitfest application: > make installcheck-world: not tested > Implements feature: not tested > Spec compliant: tested, failed > Documentation: not tested > > I am not very convinced to have that, but I have performed some testing on master (). > > I found a crash > > [Thread debugging using libthread_db enabled] > Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". > Core was generated by `pgbench postgres -T 60 -f a.sql'. > Program terminated with signal SIGSEGV, Segmentation fault. > #0 0x000055bfb572a4b2 in expr_yylex (yylval_param=0x7fff29a60870, yyscanner=0x55bfb6867a90) at exprscan.c:1107 > 1107 *yy_cp = yyg->yy_hold_char; > (gdb) bt > #0 0x000055bfb572a4b2 in expr_yylex (yylval_param=0x7fff29a60870, yyscanner=0x55bfb6867a90) at exprscan.c:1107 > #1 0x000055bfb572cb63 in expr_lex_one_word (state=0x55bfb6867a10, word_buf=0x7fff29a608d0, offset=0x7fff29a608a8) at exprscan.l:340 > #2 0x000055bfb57358fd in process_backslash_command (sstate=0x55bfb6867a10, source=0x55bfb68653a0 "a.sql") at pgbench.c:4540 > #3 0x000055bfb5736528 in ParseScript ( > script=0x55bfb68655f0 "\\set nbranches :scale\n\\set ntellers 10 * :scale\n\\set naccounts 100000 * :scale\n\\setrandomaid 1 :naccounts\n\\setrandom bid 1 :nbranches\n\\setrandom tid 1 :ntellers\n\\setrandom delta -5000 5000\nBEGIN;\nUPD"...,desc=0x55bfb68653a0 "a.sql", weight=1) at pgbench.c:4826 > #4 0x000055bfb5736967 in process_file (filename=0x55bfb68653a0 "a.sql", weight=1) at pgbench.c:4962 > #5 0x000055bfb5738628 in main (argc=6, argv=0x7fff29a610d8) at pgbench.c:5641 > > The new status of this patch is: Waiting on Author This patch was inactive during the commitfest, so I am going to mark it as "Returned with Feedback". Fabien, are you planning to continue working on it? -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
>> The new status of this patch is: Waiting on Author > > This patch was inactive during the commitfest, so I am going to mark it as > "Returned with Feedback". > Fabien, are you planning to continue working on it? Not in the short term, but probably for the next CF. Can you park it there? -- Fabien.
On Mon, Nov 30, 2020 at 12:53:20PM +0100, Fabien COELHO wrote: > Not in the short term, but probably for the next CF. Can you park it there? IMO, I think that it would be better to leave this item marked as RwF for now if you are not sure, and register it again in the CF once there is an actual new patch. This approach makes for less bloat in the CF app. -- Michael
Attachment
On 30.11.2020 14:53, Fabien COELHO wrote: > >>> The new status of this patch is: Waiting on Author >> >> This patch was inactive during the commitfest, so I am going to mark >> it as "Returned with Feedback". >> Fabien, are you planning to continue working on it? > > Not in the short term, but probably for the next CF. Can you park it > there? > Sure, I'll move it to the next CF then. I also noticed, that the first message mentions the idea of refactoring to use some code it in both pgbench and psql code. Can you, please, share a link to the thread, if it exists? -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
> Sure, I'll move it to the next CF then. I also noticed, that the first > message mentions the idea of refactoring to use some code it in both > pgbench and psql code. Can you, please, share a link to the thread, if > it exists? Unsure. I'll try to find it if it exists. -- Fabien.
Bonjour Michaël, > On Mon, Nov 30, 2020 at 12:53:20PM +0100, Fabien COELHO wrote: >> Not in the short term, but probably for the next CF. Can you park it there? > > IMO, I think that it would be better to leave this item marked as RwF > for now if you are not sure, and register it again in the CF once > there is an actual new patch. This approach makes for less bloat in > the CF app. Yep. Although I was under water this Fall, I should have more time available in January, so I think I'll do something about it. Now if it is RWF by then, I'll resubmit it. -- Fabien.
On 2020-Apr-13, Fabien COELHO wrote: > Alvaro's feedback was about the lack of "pg_strndup" availability, but he > concluded that it would seldom be used if available, so it was not worth the > effort to add it for the sake of this small patch. Hello. Please note that as is often the case, I was wrong about this point; I ended up adding pnstrdup in commit 0b9466fce2cf so you should be able to use it for this now.