Обсуждение: Re: pgbench: new feature allowing to launch shell commands
Here's a first round of review of the patch submitted at http://archives.postgresql.org/message-id/c64c5f8b0910252350w42f7ea13g52a6a88a86143374@mail.gmail.com This is moving along nicely, and is now working (with some hacking) for the one use case I wanted it for. High-level summary of what I think needs to get done before this is ready to commit: 1) Needs tab/space formatting cleaned up 2) "Execution of meta-command failed" errors are a small but serious problem 3) Should consider how :variable interpretation should work in a \[set]shell call 4) General code cleanup, including possible refactoring 5) Update pgbench docs to cover new calls. I hoped to find time to help with this, it looks like I'm not going to have it in the near future. 6) Should do basic performance regression testing to confirm this patch doesn't impact pgbench results that don't use the new feature. This I'll take care of, I'm not particularly worried about that based on what's been changed so far. Details: This revision is much improved over the earlier ones. No problems applying the patch, and the basics work as expected. One bit of code formatting nitpicking: there are some spots where the code doesn't line up horizontally as expected. I'm seeing lines spaced across with tabs mixed with ones where spaces were used, at least one line where I think you formatted presuming 8-character tabs. That's kind of painful to expect a committer to clean up. See http://wiki.postgresql.org/wiki/Developer_FAQ#What.27s_the_formatting_style_used_in_PostgreSQL_source_code.3F for more details about expectations here. Attached are the scripts I used for testing the feature: skewed-acct.pl : perl script that takes in the number of accounts and picks one with a skewed Pareto-ish distribution: 80% of the time, one from the first 20% of the accounts is picked. skewed-select.sql : pgbench script that calls the script skewed-test.sh : Little test program to confirm that the Perl code works as it's supposed to, for anyone who wants to hack on it for some reason (my Perl is miserable) Here's what I did to test: sudo ln -s `pwd`/skewed-acct.pl /usr/local/bin createdb pgbench pgbench -i -s 10 pgbench pgbench -T 10 -j 4 -c 8 -S pgbench pgbench -T 10 -j 4 -c 8 -f skewed-select.sql pgbench Baseline gives me about 20K selects/second, with the shell call in the middle that dropped to around 400/second. Since we know shell calls are expensive that's not too much of a surprise. It does make an interesting statement about the overhead here though--if you want to instrument something you expect to execute thousands of times a second, that's probably not going to be possible using these calls. I didn't try yet to see how small I could make the shell overhead smaller (using a simpler script instead of the current Perl one, minimizing the number of characters in the pgbench script) To watch what was happening, I needed to toggle on "log_statement=all". That will confirm that the skew was working properly, which is good to see because it didn't as I originally wrote it. There are two functional problems that jumped right out at me with the implementation. First, sometimes the call fails. On every run, I'm getting one or more of these (note that each of the 4 threads has two clients running against it, 0 and 1): setshell: error getting parameterClient 0 aborted in state 1. Execution of meta-command failed. setshell: error getting parameterClient 1 aborted in state 1. Execution of meta-command failed. Right near the end. Am guessing there's some last-transaction cleanup going awry. Don't know why that is, and will be curious if it can be reproduced. A much more insidious issues is that shell and setshell don't do interpretation of pgbench variables like ":naccounts". The way I originally wrote skewed-select.sql (which you can still see commented out in the attached version) it looked like this: \set naccounts 100000 * :scale \setshell aid skewed-acct.pl :naccounts SELECT abalance FROM pgbench_accounts WHERE aid = :aid; This didn't work through--that was calling the script with the literal text ":naccounts" rather than the value for that. Similarly, when I tried to add some debugging bits to the end like this: \shell echo :aid That just printed ":aid" rather than the value. I would like to see both of these work as written above (the versions commented out in the attached sample). Now, it's possible to work around that limitation in some cases--the attached version just hard-codes in the number of accounts given the scale I was testing at. This isn't really ideal though. Unfortunately for you, the way :variable decoding is handling inside pgbench right now doesn't even have to consider things like quoting. The variable decoding is done at the exact places it makes sense at, rather than being done more globally. That means the existing replacement logic can't be re-used for this feature. And we can certainly expect that shell commands might have a ":" in them anyway, so doing this right ends up leading toward things like making "::" be a ":" that doesn't get substituted. Maybe this whole issue can be avoided as just being more complicated than this feature justifies. I think people will be surprised, and find this not as useful as it could be, unless this is done though. Also, this doesn't work \setshell :aid skewed-acct.pl 1000000 Which I think will surprise people. If there's a : as the first character of the second field here, it should just get clipped off. As for more general code review, one thing that jumped out at me was this: if (fgets(res, 64, respipe) == NULL) This should use sizeof(res) instead of hard-coding its size like that here. So far as general structure goes, actually adding the variable substitution complexity to the shell/setshell code is going to make it even more complicated, and it already sticks out as badly fitting into doCustom as it is. I suspect this capability will make for a really unmanagable result. Maybe refactor the new shell stuff a bit into another subroutine now that you've finished the main guts? doCustom feels like it's grown way beyond its original purpose here with this patch, it was on the edge of that before you started--and these two new commands are by far the most complicated. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com \set naccounts 100000 * :scale -- This doesn't work: --\setshell aid skewed-acct.pl :naccounts -- Have to hard-code the value instead: \setshell :aid skewed-acct.pl 1000000 SELECT abalance FROM pgbench_accounts WHERE aid = :aid; -- This doesn't work as expected either -- \shell echo :aid
Вложения
Hi,
Sorry if you receive this email a second time, Greg, i didn't notice it has not been sent to the hackers ML
Thanks for your first review.
I tried to work on most of the issues you noticed
> 1) Needs tab/space formatting cleaned up
This one is done, I adapted my environment to the Postgresql formats. I hope there is nothing else more linked to that. Thanks for your first review.
I tried to work on most of the issues you noticed
> 1) Needs tab/space formatting cleaned up
> 2) "Execution of meta-command failed" errors are a small but serious problem
This error appears (n-1) times by using n threads with the j option. As you said in your previous email there is some thread cleanup when one is disconnected. This error still appears, I don't know yet which part of the code is the origin of that. I needs more investigation.
> 3) Should consider how :variable interpretation should work in a \[set]shell call
It is supported now. I implemented this, I made a test with your pearl script, my own tests and it worked, at least no error appeared :)
> 4) General code cleanup, including possible refactoring
I didn't modify too much the code, I just noticed a couple of variables unnecessary and some definitions not in adequacy with pgbench code. Btw, what I did is included in the patch.
> 5) Update pgbench docs to cover new calls. I hoped to find time to help with this, it looks like I'm not going to have it in the near future.
I tried to update the document writing a couple of lines describing simply the new possible calls setshell and shell. I am not that skilled at sgml though.
> 6) Should do basic performance regression testing to confirm this patch doesn't impact pgbench results that don't use the new feature. This I'll take care of, I'm not particularly worried about that based on what's been changed so far.
Do you have an idea of what kind of tests could be done? I don't have so much experience about common regression tests linked to pgbench.
I also added a second file including a couple of scripts written quickly generating numbers based on the gauss and pareto density functions. It cannot be used straightforwardly now, but still it can be a base for something linked to setshell.
Regards,
Michael Paquier
NIPPON TELEGRAPH AND
TELEPHONE CORPORATION
NTT Open Source Software Center
Вложения
I didn't send the good patch yesterday. => --;
Here is the latest version.
Regards,
--
Michael Paquier
NIPPON TELEGRAPH AND
TELEPHONE CORPORATION
NTT Open Source Software Center
Here is the latest version.
Regards,
--
Michael Paquier
NIPPON TELEGRAPH AND
TELEPHONE CORPORATION
NTT Open Source Software Center
Вложения
Michael Paquier wrote: > > 3) Should consider how :variable interpretation should work in a > \[set]shell call > It is supported now. I implemented this, I made a test with your perl > script, my own tests and it worked, at least no error appeared :) It looks like how you did this is a little less complicated than I had hoped for. Let me show you some examples of how I think this should work. Say naccounts = 1000000 and bid=123 already: \setshell aid skew.sh :naccounts runs "skew.sh 1000000" \setshell aid skew.sh a ::naccounts c runs "skew.sh a :naccounts c" - do not substitute the variable if "::" appears, just replace with ":". Otherwise, there's no way to include a real ":" in the command line \setshell aid skew.h aid :naccounts :bid runs "shew.sh 1000000 123" From looking at the code, I think that only case (1) is supported by the bits you added (haven't actually re-tested it since I know you're still working). Also, having that same substitution logic work for both shell and setshell should would be nice here. As far as reducing the amount of stuff in doCustom goes, I think what you want for this specific part is a subroutine you can pass a string that has some number of :variable references in it, returning a new string with them having the substituted values inserted in there. That's something I think it would be easier to get right as a standalone function first, and then both shell and setshell could use it. > Do you have an idea of what kind of tests could be done? I don't have so much experience> about common regression tests linked to pgbench. None of the regression tests use pgbench yet, partly because contrib modules like it aren't necessarily even built before the main regression tests are run. Also, it's hard to write a pgbench-based test using the current pg_regress framework. The complete non-determinism on the TPS scores for example makes it hard to avoid having a diff against any standard regression result provided. I think it would be nice to add a very complicated script that uses all these weird features for regression test purposes, but it's not so clear how we would enforce running it automatically to catch if a future change broke something. As far as the rest of your concerns, once we get this to code complete and all the bugs squashed, I can take a pass at cleaning up your docs and making sure there's not any performance regression as part of my final review. What you've added in there so far is good enough for now, I just didn't want to do the docs changes from scratch myself (and I thought it would be useful for you to get a bit of practice on that too, since they're usually required for patch submissions). If you can make the three examples above all work, and get rid of the threading bug, I'll be clear to take care of docs review/performanc tests and then pass this over for a committer to look at. It would be nice if the code was refactored a bit too first, just because it's less likely to be rejected for "the code is ugly" reasons if that's done first. That sort of rejection is always a real possibility with this project, particularly for something like this where it's not as obvious to everyone what the feature is useful for. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
Hi,
I took care of making the 3 cases you mentioned working in the new version of the patch attached. It worked in my case for both shell and setshell without any problem.
The code has also been reorganized so as to lighten the process in doCustom. It looks cleaner on this part.
The only remaining issues are the thread bug and the documentation.
For the bug, I am currently looking into it.
I should take a little bit of time, I don't really know yet from where it comes exactly...
For the documentation, I'll try to write it a little bit more once the code issues are solved.
Regards
--
Michael Paquier
NIPPON TELEGRAPH AND
TELEPHONE CORPORATION
NTT Open Source Software Center
I took care of making the 3 cases you mentioned working in the new version of the patch attached. It worked in my case for both shell and setshell without any problem.
The code has also been reorganized so as to lighten the process in doCustom. It looks cleaner on this part.
The only remaining issues are the thread bug and the documentation.
For the bug, I am currently looking into it.
I should take a little bit of time, I don't really know yet from where it comes exactly...
For the documentation, I'll try to write it a little bit more once the code issues are solved.
Regards
--
Michael Paquier
NIPPON TELEGRAPH AND
TELEPHONE CORPORATION
NTT Open Source Software Center
Вложения
The threading bug appears when a duration is set for pgbench tests.<br /> Instead of a duration, if a number of xacts isset, this error doesn't happen.<br /><br /> If i understood the problem well, when the alarm signal comes, all the threadshave to disconnect even the ones looking for a setshell parameter at this moment, creating the thread error:<br />setshell: error getting parameter<br /> Client 0 aborted in state 1. Execution of meta-command failed.<br /><br /> I amtrying to find an elegant way to solve this, but I can't figure out yet how to deal with this error as it looks tricky.<br/>
Please find attached the latest version of the patch,
with the threading bug corrected and the documentation updated as well.
The origin of the bug was the alarm signal. Once the duration is over, all the threads have to finish and timer_exceeded is set at true.
A control on this variable in setshell process is enough so as not to show out the thread error.
Regards,
with the threading bug corrected and the documentation updated as well.
The origin of the bug was the alarm signal. Once the duration is over, all the threads have to finish and timer_exceeded is set at true.
A control on this variable in setshell process is enough so as not to show out the thread error.
Regards,
Вложения
Michael Paquier <michael.paquier@gmail.com> wrote: > Please find attached the latest version of the patch, > with the threading bug corrected and the documentation updated as well. Why do you use BUFFER_SIZE-1 for snprintf? snprintf(commandShell, SHELL_COMMAND_SIZE-1, ...) Trailing nulls are also included in the length, so snprintf(commandShell, SHELL_COMMAND_SIZE, ...) would be ok. (removed -1) Other parts look fine, except an empty tag <replaceable></> in the documentation. Is it a typo? Regards, --- Takahiro Itagaki NTT Open Source Software Center
> Michael Paquier <michael.paquier@gmail.com> wrote: > > Please find attached the latest version of the patch, > > with the threading bug corrected and the documentation updated as well. Please don't describe your-specific usage of shell command in the documentation > Why do you use BUFFER_SIZE-1 for snprintf? > snprintf(commandShell, SHELL_COMMAND_SIZE-1, ...) > Trailing nulls are also included in the length, so > snprintf(commandShell, SHELL_COMMAND_SIZE, ...) > would be ok. (removed -1) I found several bugs and matters. * The calcuration of the shell command length is wrong. * You cannot assign 0 to variables with \setshell meta command. * Comparison with "== true" is a bad manner. * You called \shell "shell command" and \setsell "shell script", but we don't need the difference. I think "shell command" is enough. Heavily cleaned up patch attached. Please review it. Regards, --- Takahiro Itagaki NTT Open Source Software Center
Вложения
Takahiro Itagaki wrote: > Heavily cleaned up patch attached. Please review it. > This is almost there. The refactoring work you both did is exactly how I was hoping this would end up looking, code-wise, and the feature set is basically feature complete except for one small UI concern I have. Attached is an updated version of my skewed-select.sql test program, adjusted to take advantage of the now supported syntax. Note that in order to run this properly, you need to pass the database scale into pgbench when you run it like this: pgbench -T 10 -j 4 -c 8 -s 10 -f skewed-select.sql pgbench Because when you use a custom "-f" script, :scale isn't set otherwise. In the current version of the patch, you can do this: \setshell aid skewed-acct.pl :naccounts But you can't do this: \setshell :aid skewed-acct.pl :naccounts What's worse is that you don't get an error in that second case--it just doesn't set the variable. I thought this was fixed by one of Michael's versions here, so maybe this is a regression from the other clean-up? Since all the other ways you interact with this type of variable in pgbench require the ":", not supporting it in this context seems like a recipe for weird problems for those trying to use it. If we can get an updated version where you can use either syntax for the variable name passed to setshell, this one will be ready for commit I think. I was tempted to fix the bug myself, but I seem to be doing better as a tester on this patch rather than working on its development. Everything else--docs, code--looks pretty good now. Only thing we might add is a warning that you're going to completely tank your pgbench results by using the high-overhead shell command if your test would otherwise be limited by CPU. This feature is only really useful as far as performance testing goes if you're using it on an I/O bound test. I have some more pgbench hints to apply at some point in the future, so it wouldn't be problem to skip this for now; I can bundle it into that section later. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com \set naccounts 100000 * :scale -- This works \setshell aid skewed-acct.pl :naccounts -- But this doesn't and should: -- \setshell :aid skewed-acct.pl :naccounts SELECT abalance FROM pgbench_accounts WHERE aid = :aid; \shell echo ::aid :aid selected from :naccounts accounts
Hi,
Yeah the grammar ":variable" is used to set a parameter, the user will feel disorientated if there is no support.
The attached patch supports this new case, based on Itagaki-san's last version. I also added a warning to tell the user that pgbench is slowing down when using this feature.
If nobody is against it, I will mark it as ready to commit.
--
Regards,
Michael Paquier
NIPPON TELEGRAPH AND
TELEPHONE CORPORATION
NTT Open Source Software Center
Yeah the grammar ":variable" is used to set a parameter, the user will feel disorientated if there is no support.
The attached patch supports this new case, based on Itagaki-san's last version. I also added a warning to tell the user that pgbench is slowing down when using this feature.
If nobody is against it, I will mark it as ready to commit.
--
Regards,
Michael Paquier
NIPPON TELEGRAPH AND
TELEPHONE CORPORATION
NTT Open Source Software Center
Вложения
Michael Paquier <michael.paquier@gmail.com> wrote: > Yeah the grammar ":variable" is used to set a parameter, the user will feel > disorientated if there is no support. > The attached patch supports this new case, based on Itagaki-san's last > version. What is the spec of "\setshell :variable" ? Literally interpreted, it declares a variable with name ":variable". But pgbench cannot use such variables because only variables named with alphabets, numbers, and _ can be accepted. Should we forbid to assign variables that name contain invalid characters instead? > I also added a warning to tell the user that pgbench is slowing > down when using this feature. This change seems to be nonsense. Do you want to fill your terminal with such messages? Proposed patch attached. It checks for variable names whether they consist of isalnum or _. The check is only performed when a new variable is assigned to avoid overhead. In normal workload, variables with the same names are re-used repeatedly. I don't think the additinal check would decrease performance of pgbench. Regards, --- Takahiro Itagaki NTT Open Source Software Center
Вложения
Takahiro Itagaki wrote: > Michael Paquier <michael.paquier@gmail.com> wrote: > > >> Yeah the grammar ":variable" is used to set a parameter, the user will feel >> disorientated if there is no support. >> The attached patch supports this new case, based on Itagaki-san's last >> version. >> > What is the spec of "\setshell :variable" ? > Literally interpreted, it declares a variable with name ":variable". > But pgbench cannot use such variables because only variables named > with alphabets, numbers, and _ can be accepted. Should we forbid to > assign variables that name contain invalid characters instead? > After reviewing this a bit more I've realized my idea wasn't very good here. If this is what we're already accepting: \set nbranches :scale It's completely reasonable to say that *only* this works: \setshell aid expr 1 + :naccounts While this does not: \setshell :aid expr 1 + :naccounts And I was wrong to ask that it should. Sorry to have lead you both around over this, my bad. > Proposed patch attached. It checks for variable names whether they > consist of isalnum or _. The check is only performed when a new variable > is assigned to avoid overhead. In normal workload, variables with the > same names are re-used repeatedly. I don't think the additinal check would > decrease performance of pgbench. > This is interesting, but we're already past where this should have wrapped up and I'm not sure if it's worth fiddling with this code path right now. I think your idea is good, just outside of what we should fool with right now and I'm out of time to work on testing it further too. How about this: the version you updated at http://archives.postgresql.org/pgsql-hackers/2009-12/msg00847.php , your pgbenchshell_20091210.patch, worked perfectly for me except for one complaint I've since dropped. How about we move toward committing that one, and maybe we look at this whole variable name handling improvement as a separate patch later if you think it's worth pursuing? At this point I'd just like to have a version of the shell/setshell feature go into the pgbench code without dragging things out further into new territory. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
Greg Smith <greg@2ndquadrant.com> wrote: > How about this: the version you updated at > http://archives.postgresql.org/pgsql-hackers/2009-12/msg00847.php , your > pgbenchshell_20091210.patch, worked perfectly for me except for one > complaint I've since dropped. How about we move toward committing that > one, and maybe we look at this whole variable name handling improvement > as a separate patch later if you think it's worth pursuing? It's fine idea. I'll commit the previous lite version, and discuss whether we need the difference patch for fool proof. Regards, --- Takahiro Itagaki NTT Open Source Software Center