Hi,
I gone through the discussion for this patch and here is my review:
The main aim of this patch is to reduce the number of log lines. It is also
suggested to use an options to provide the interval but few of us are not
much agree on it. So final discussion ended at keeping 5 sec interval
between each log line.
However, I see, there are two types of users here:
1. Who likes these log lines, so that they can troubleshoot some slowness
and all
2. Who do not like these log lines.
So keeping these in mind, I rather go for an option which will control
this. People falling in category one can set this option to very low where
as users falling under second category can keep it high.
However, assuming we settled on 5 sec delay, here are few comments on that
patch attached:
Comments:
=========
Patch gets applied cleanly with some whitespace errors. make and make
install too went smooth.
make check was smooth. Rather it should be smooth since there are NO
changes in other part of the code rather than just pgbench.c and we do not
have any test-case as well.
However, here are few comments on changes in pgbench.c
1.
Since the final discussion ended at keeping a 5 seconds interval will be
good enough, Author used a global int variable for that.
Given that it's just a constant, #define would be a better choice.
2.
+ /* let's not call the timing for each row, but only each 100 rows
*/
Why only 100 rows ? Have you done any testing to come up with number 100 ?
To me it seems very low. It will be good to test with 1K or even 10K.
On my machine (2.4 GHz Intel core 2 duo Macbook PRO, running Ubuntu in VM
with 4GB RAM, 1067 DDR3), in 5 Sec, approx 1M rows were inserted. So
checking every 100 rows looks overkill.
3.
Please indent following block as per the indentation just above that
/* used to track elapsed time and estimate of the remaining time */
instr_time start, diff;
double elapsed_sec, remaining_sec;
int log_interval = 1;
4.
+ /* have ve reached the next interval? */
Do you mean "have WE reached..."
5.
While applying a patch, I got few white-space errors. But I think every
patch goes through pgindent which might take care of this.
Thanks
On Sun, Nov 11, 2012 at 11:02 PM, Tomas Vondra <t...@fuzzy.cz> wrote:
> On 23.10.2012 18:21, Robert Haas wrote:
> > On Tue, Oct 23, 2012 at 12:02 PM, Alvaro Herrera
> > <alvhe...@2ndquadrant.com> wrote:
> >> Tomas Vondra wrote:
> >>
> >>> I've been thinking about this a bit more, and do propose to use an
> >>> option that determines "logging step" i.e. number of items (either
> >>> directly or as a percentage) between log lines.
> >>>
> >>> The attached patch defines a new option "--logging-step" that accepts
> >>> either integers or percents. For example if you want to print a line
> >>> each 1000 lines, you can to this
> >>>
> >>> $ pgbench -i -s 1000 --logging-step 1000 testdb
> >>
> >> I find it hard to get excited about having to specify a command line
> >> argument to tweak this. Would it work to have it emit messages
> >> depending on elapsed time and log scale of tuples emitted? So for
> >> example emit the first message after 5 seconds or 100k tuples, then back
> >> off until (say) 15 seconds have lapsed and 1M tuples, etc? The idea is
> >> to make it verbose enough to keep a human satisfied with what he sees,
> >> but not flood the terminal with pointless updates. (I think printing
> >> the ETA might be nice as well, not sure).
> >
> > I like this idea. One of the times when the more verbose output is
> > really useful is when you expect it to run fast but then it turns out
> > that for some reason it runs really slow. If you make the output too
> > terse, then you end up not really knowing what's going on. Having it
> > give an update at least every 5 seconds would be a nice way to give
> > the user a heads-up if things aren't going as planned, without
> > cluttering the normal case.
>
> I've prepared a patch along these lines. The attached version used only
> elapsed time to print the log messages each 5 seconds, so now it prints
> a meessage each 5 seconds no matter what, along with an estimate of
> remaining time.
>
> I've removed the config option, although it might be useful to specify
> the interval?
>
> I'm not entirely sure how the 'log scale of tuples' should work - for
> example when the time 15 seconds limit is reached, should it be reset
> back to the previous step (5 seconds) to give a more detailed info, or
> should it be kept at 15 seconds?
>
> Tomas
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>
--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Phone: +91 20 30589500
Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb
This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.
