Обсуждение: review: pgbench - aggregation of info written into log
Hello I did review of pgbench patch http://archives.postgresql.org/pgsql-hackers/2012-09/msg00737.php * this patch is cleanly patched * I had a problem with doc make[1]: Entering directory `/home/pavel/src/postgresql/doc/src/sgml' openjade -wall -wno-unused-param -wno-empty -wfully-tagged -D . -D . -c /usr/share/sgml/docbook/dsssl-stylesheets/catalog -d stylesheet.dsl -t sgml -i output-html -V html-index postgres.sgml openjade:pgbench.sgml:767:8:E: document type does not allow element "TITLE" here; missing one of "ABSTRACT", "AUTHORBLURB", "MSGSET", "CALLOUTLIST", "ITEMIZEDLIST", "ORDEREDLIST", "SEGMENTEDLIST", "VARIABLELIST", "CAUTION", "IMPORTANT", "NOTE", "TIP", "WARNING", "FORMALPARA", "BLOCKQUOTE", "EQUATION", "EXAMPLE", "FIGURE", "TABLE", "PROCEDURE", "SIDEBAR", "QANDASET", "REFSECT3" start-tag make[1]: *** [HTML.index] Error 1 make[1]: *** Deleting file `HTML.index' make[1]: Leaving directory `/home/pavel/src/postgresql/doc/src/sgml * pgbench is compiled without warnings * there is a few issues in source code + + /* should we aggregate the results or not? */ + if (use_log_agg) + { + + /* are we still in the same interval? if yes, accumulate the + * values (print them otherwise) */ + if (agg->start_time + use_log_agg >= INSTR_TIME_GET_DOUBLE(now)) + { + * a real time range for aggregation is dynamic (pgbench is not realtime application), so I am not sure if following constraint has sense + if ((duration > 0) && (use_log_agg > 0) && (duration % use_log_agg != 0)) { + fprintf(stderr, "duration (%d) must be a multiple of aggregation interval (%d)\n", duration, use_log_agg); + exit(1); + } * it doesn't flush last aggregated data (it is mentioned by Tomas: "Also, I've realized the last interval may not be logged at all - I'll take a look into this in the next version of the patch."). And it can be significant for higher values of "use_log_agg" * a name of variable "use_log_agg" is not good - maybe "log_agg_interval" ? Regards Pavel Stehule
Hi, attached is a v4 of the patch. There are not many changes, mostly some simple tidying up, except for handling the Windows. After a bit more investigation, it seems to me that we can't really get the same behavior as in other systems - basically the timestamp is unavailable so we can't log the interval start. So it seems to me the best we can do is to disable this option on Windows (and this is done in the patch). So when you try to use "--aggregate-interval" on Win it will complain it's an unknown option. Now that I think of it, there might be a better solution that would not mimic the Linux/Unix behaviour exactly - we do have elapsed time since the start of the benchmark, so maybe we should use this instead of the timestamp? I mean on systems with reasonable gettimeofday we'd get 1345828501 5601 1542744 483552416 61 2573 1345828503 7884 1979812 565806736 60 1479 1345828505 7208 1979422 567277552 59 1391 1345828507 7685 1980268 569784714 60 1398 1345828509 7073 1979779 573489941 236 1411 but on Windows we'd get 0 5601 1542744 483552416 61 2573 2 7884 1979812 565806736 60 1479 4 7208 1979422 567277552 59 1391 6 7685 1980268 569784714 60 1398 8 7073 1979779 573489941 236 1411 i.e. the first column is "seconds since start of the test". Hmmmm, and maybe we could call 'gettimeofday' at the beginning, to get the timestamp of the test start (AFAIK the issue on Windows is the resolution, but it should be enough). Then we could add it up with the elapsed time and we'd get the same output as on Linux. And maybe this could be done in regular pgbench execution too? But I really need help from someone who knows Windows and can test this. Comments regarding Pavel's reviews are below: On 2.10.2012 20:08, Pavel Stehule wrote: > Hello > > I did review of pgbench patch > http://archives.postgresql.org/pgsql-hackers/2012-09/msg00737.php > > * this patch is cleanly patched > > * I had a problem with doc > > make[1]: Entering directory `/home/pavel/src/postgresql/doc/src/sgml' > openjade -wall -wno-unused-param -wno-empty -wfully-tagged -D . -D . > -c /usr/share/sgml/docbook/dsssl-stylesheets/catalog -d stylesheet.dsl > -t sgml -i output-html -V html-index postgres.sgml > openjade:pgbench.sgml:767:8:E: document type does not allow element > "TITLE" here; missing one of "ABSTRACT", "AUTHORBLURB", "MSGSET", > "CALLOUTLIST", "ITEMIZEDLIST", "ORDEREDLIST", "SEGMENTEDLIST", > "VARIABLELIST", "CAUTION", "IMPORTANT", "NOTE", "TIP", "WARNING", > "FORMALPARA", "BLOCKQUOTE", "EQUATION", "EXAMPLE", "FIGURE", "TABLE", > "PROCEDURE", "SIDEBAR", "QANDASET", "REFSECT3" start-tag > make[1]: *** [HTML.index] Error 1 > make[1]: *** Deleting file `HTML.index' > make[1]: Leaving directory `/home/pavel/src/postgresql/doc/src/sgml Hmmm, I've never got the docs to build at all, all I do get is a heap of some SGML/DocBook related issues. Can you investigate a bit more where's the issue? I don't remember messing with the docs in a way that might cause this ... mostly copy'n'paste edits. > * pgbench is compiled without warnings > > * there is a few issues in source code > > + > + /* should we aggregate the results or not? */ > + if (use_log_agg) > + { > + > + /* are we still in the same interval? if yes, accumulate the > + * values (print them otherwise) */ > + if (agg->start_time + use_log_agg >= INSTR_TIME_GET_DOUBLE(now)) > + { > + Errr, so what are the issues here? > > * a real time range for aggregation is dynamic (pgbench is not > realtime application), so I am not sure if following constraint has > sense > > + if ((duration > 0) && (use_log_agg > 0) && (duration % use_log_agg != 0)) { > + fprintf(stderr, "duration (%d) must be a multiple of aggregation > interval (%d)\n", duration, use_log_agg); > + exit(1); > + } I'm not sure what might be the issue here either. If the test duration is set (using "-T" option), then this kicks in and says that the duration should be a multiple of aggregation interval. Seems like a sensible assumption to me. Or do you think this is check should be removed? > * it doesn't flush last aggregated data (it is mentioned by Tomas: > "Also, I've realized the last interval may not be logged at all - I'll > take a look into this in the next version of the patch."). And it can > be significant for higher values of "use_log_agg" Yes, and I'm still not sure how to fix this in practice. But I do have this on my TODO. > > * a name of variable "use_log_agg" is not good - maybe "log_agg_interval" ? I've changed it to agg_interval. regards Tomas
Вложения
Hello 2012/11/12 Tomas Vondra <tv@fuzzy.cz>: > Hi, > > attached is a v4 of the patch. There are not many changes, mostly some > simple tidying up, except for handling the Windows. > > After a bit more investigation, it seems to me that we can't really get > the same behavior as in other systems - basically the timestamp is > unavailable so we can't log the interval start. So it seems to me the > best we can do is to disable this option on Windows (and this is done in > the patch). So when you try to use "--aggregate-interval" on Win it will > complain it's an unknown option. > > Now that I think of it, there might be a better solution that would not > mimic the Linux/Unix behaviour exactly - we do have elapsed time since > the start of the benchmark, so maybe we should use this instead of the > timestamp? I mean on systems with reasonable gettimeofday we'd get > > 1345828501 5601 1542744 483552416 61 2573 > 1345828503 7884 1979812 565806736 60 1479 > 1345828505 7208 1979422 567277552 59 1391 > 1345828507 7685 1980268 569784714 60 1398 > 1345828509 7073 1979779 573489941 236 1411 > > but on Windows we'd get > > 0 5601 1542744 483552416 61 2573 > 2 7884 1979812 565806736 60 1479 > 4 7208 1979422 567277552 59 1391 > 6 7685 1980268 569784714 60 1398 > 8 7073 1979779 573489941 236 1411 > > i.e. the first column is "seconds since start of the test". Hmmmm, and > maybe we could call 'gettimeofday' at the beginning, to get the > timestamp of the test start (AFAIK the issue on Windows is the > resolution, but it should be enough). Then we could add it up with the > elapsed time and we'd get the same output as on Linux. > > And maybe this could be done in regular pgbench execution too? But I > really need help from someone who knows Windows and can test this. > > Comments regarding Pavel's reviews are below: > > On 2.10.2012 20:08, Pavel Stehule wrote: >> Hello >> >> I did review of pgbench patch >> http://archives.postgresql.org/pgsql-hackers/2012-09/msg00737.php >> >> * this patch is cleanly patched >> >> * I had a problem with doc >> >> make[1]: Entering directory `/home/pavel/src/postgresql/doc/src/sgml' >> openjade -wall -wno-unused-param -wno-empty -wfully-tagged -D . -D . >> -c /usr/share/sgml/docbook/dsssl-stylesheets/catalog -d stylesheet.dsl >> -t sgml -i output-html -V html-index postgres.sgml >> openjade:pgbench.sgml:767:8:E: document type does not allow element >> "TITLE" here; missing one of "ABSTRACT", "AUTHORBLURB", "MSGSET", >> "CALLOUTLIST", "ITEMIZEDLIST", "ORDEREDLIST", "SEGMENTEDLIST", >> "VARIABLELIST", "CAUTION", "IMPORTANT", "NOTE", "TIP", "WARNING", >> "FORMALPARA", "BLOCKQUOTE", "EQUATION", "EXAMPLE", "FIGURE", "TABLE", >> "PROCEDURE", "SIDEBAR", "QANDASET", "REFSECT3" start-tag >> make[1]: *** [HTML.index] Error 1 >> make[1]: *** Deleting file `HTML.index' >> make[1]: Leaving directory `/home/pavel/src/postgresql/doc/src/sgml > > Hmmm, I've never got the docs to build at all, all I do get is a heap of > some SGML/DocBook related issues. Can you investigate a bit more where's > the issue? I don't remember messing with the docs in a way that might > cause this ... mostly copy'n'paste edits. > >> * pgbench is compiled without warnings >> >> * there is a few issues in source code >> >> + >> + /* should we aggregate the results or not? */ >> + if (use_log_agg) >> + { >> + >> + /* are we still in the same interval? if yes, accumulate the >> + * values (print them otherwise) */ >> + if (agg->start_time + use_log_agg >= INSTR_TIME_GET_DOUBLE(now)) >> + { >> + > > Errr, so what are the issues here? using a use_log_agg variable - bad name for variable - it is fixed > >> >> * a real time range for aggregation is dynamic (pgbench is not >> realtime application), so I am not sure if following constraint has >> sense >> >> + if ((duration > 0) && (use_log_agg > 0) && (duration % use_log_agg != 0)) { >> + fprintf(stderr, "duration (%d) must be a multiple of aggregation >> interval (%d)\n", duration, use_log_agg); >> + exit(1); >> + } > > I'm not sure what might be the issue here either. If the test duration > is set (using "-T" option), then this kicks in and says that the > duration should be a multiple of aggregation interval. Seems like a > sensible assumption to me. Or do you think this is check should be removed? > >> * it doesn't flush last aggregated data (it is mentioned by Tomas: >> "Also, I've realized the last interval may not be logged at all - I'll >> take a look into this in the next version of the patch."). And it can >> be significant for higher values of "use_log_agg" > > Yes, and I'm still not sure how to fix this in practice. But I do have > this on my TODO. > >> >> * a name of variable "use_log_agg" is not good - maybe "log_agg_interval" ? > > I've changed it to agg_interval. ok issues: * empty lines with invisible chars (tabs) + and sometimes empty lines after and before {} * adjustment of start_time + * the desired interval */ + while (agg->start_time + agg_interval < INSTR_TIME_GET_DOUBLE(now)) + agg->start_time = agg->start_time + agg_interval; can "skip" one interval - so when transaction time will be larger or similar to agg_interval - then results can be strange. We have to know real length of interval Regards Pavel > > regards > Tomas > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Hi Tomas, On 2012-11-27 14:55:59 +0100, Pavel Stehule wrote: > > attached is a v4 of the patch. There are not many changes, mostly some > > simple tidying up, except for handling the Windows. After a quick look I am not sure what all the talk about windows is about? instr_time.h seems to provide all you need, even for windows? The only issue of gettimeofday() for windows seems to be that it is that its not all that fast an not too high precision, but that shouldn't be a problem in this case? Could you expand a bit on the problems? > >> * I had a problem with doc The current patch has conflict markers in the sgml source, there seems to have been some unresolved merge. Maybe that's all that causes the errors? Whats your problem with setting up the doc toolchain? > issues: > > * empty lines with invisible chars (tabs) + and sometimes empty lines > after and before {} > > * adjustment of start_time > > + * the desired interval */ > + while (agg->start_time > + agg_interval < INSTR_TIME_GET_DOUBLE(now)) > + > agg->start_time = agg->start_time + agg_interval; > > can "skip" one interval - so when transaction time will be larger or > similar to agg_interval - then results can be strange. We have to know > real length of interval Could you post a patch that adresses these issues? Greetings, Andres Freund --Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 8.12.2012 16:33, Andres Freund wrote: > Hi Tomas, > > On 2012-11-27 14:55:59 +0100, Pavel Stehule wrote: >>> attached is a v4 of the patch. There are not many changes, mostly some >>> simple tidying up, except for handling the Windows. > > After a quick look I am not sure what all the talk about windows is > about? instr_time.h seems to provide all you need, even for windows? The > only issue of gettimeofday() for windows seems to be that it is that its > not all that fast an not too high precision, but that shouldn't be a > problem in this case? > > Could you expand a bit on the problems? Well, in the current pgbench.c, there's this piece of code #ifndef WIN32 /* This is more than we really ought to know about instr_time */ fprintf(logfile, "%d %d %.0f %d %ld %ld\n", st->id, st->cnt, usec, st->use_file, (long) now.tv_sec, (long) now.tv_usec); #else /* On Windows, instr_time doesn't provide a timestamp anyway */ fprintf(logfile, "%d %d %.0f %d 0 0\n", st->id,st->cnt, usec, st->use_file); #endif and the Windows-related discussion in this patch is about the same issue. As I understand it the problem seems to be that INSTR_TIME_SET_CURRENT does not provide the timestamp at all. I was thinking about improving this by combining gettimeofday and INSTR_TIME, but I have no Windows box to test it on :-( > >>>> * I had a problem with doc > > The current patch has conflict markers in the sgml source, there seems > to have been some unresolved merge. Maybe that's all that causes the > errors? Ah, I see. Probably my fault, I'll fix that. > Whats your problem with setting up the doc toolchain? > >> issues: >> >> * empty lines with invisible chars (tabs) + and sometimes empty lines >> after and before {} >> >> * adjustment of start_time >> >> + * the desired interval */ >> + while (agg->start_time >> + agg_interval < INSTR_TIME_GET_DOUBLE(now)) >> + >> agg->start_time = agg->start_time + agg_interval; >> >> can "skip" one interval - so when transaction time will be larger or >> similar to agg_interval - then results can be strange. We have to know >> real length of interval > > Could you post a patch that adresses these issues? Yes, I'll work on it today/tomorrow and I'll send an improved patch. Tomas
Hi, attached is a v5 of this patch. Details below: On 8.12.2012 16:33, Andres Freund wrote: > Hi Tomas, > > On 2012-11-27 14:55:59 +0100, Pavel Stehule wrote: >>> attached is a v4 of the patch. There are not many changes, mostly some >>> simple tidying up, except for handling the Windows. > > After a quick look I am not sure what all the talk about windows is > about? instr_time.h seems to provide all you need, even for windows? The > only issue of gettimeofday() for windows seems to be that it is that its > not all that fast an not too high precision, but that shouldn't be a > problem in this case? > > Could you expand a bit on the problems? As explained in the previous message, this is an existing problem with unavailable timestamp. I'm not very fond of adding Linux-only features, but fixing that was not the goal of this patch. >>>> * I had a problem with doc > > The current patch has conflict markers in the sgml source, there seems > to have been some unresolved merge. Maybe that's all that causes the > errors? > > Whats your problem with setting up the doc toolchain? Yeah, my fault because of incomplete merge. But the real culprit was a missing refsect2. Fixed. > >> issues: >> >> * empty lines with invisible chars (tabs) + and sometimes empty lines >> after and before {} Fixed (I've removed the lines etc.) >> >> * adjustment of start_time >> >> + * the desired interval */ >> + while (agg->start_time >> + agg_interval < INSTR_TIME_GET_DOUBLE(now)) >> + >> agg->start_time = agg->start_time + agg_interval; >> >> can "skip" one interval - so when transaction time will be larger or >> similar to agg_interval - then results can be strange. We have to know >> real length of interval > > Could you post a patch that adresses these issues? So, in the end I've rewritten the section advancing the start_time. Now it works so that when skipping an interval (because of a very long transaction), it will print lines even for those "empty" intervals. So for example with a transaction file containing a single query SELECT pg_sleep(1.5); and an interval length of 1 second, you'll get something like this: 1355009677 0 0 0 0 0 1355009678 1 1501028 2253085056784 1501028 1501028 1355009679 0 0 0 0 0 1355009680 1 1500790 2252370624100 1500790 1500790 1355009681 1 1500723 2252169522729 1500723 1500723 1355009682 0 0 0 0 0 1355009683 1 1500719 2252157516961 1500719 1500719 1355009684 1 1500703 2252109494209 1500703 1500703 1355009685 0 0 0 0 0 which is IMHO the best way to deal with this. I've fixed several minor issues, added a few comments. regards Tomas
Вложения
Hi, I'm looking into this as a committer. It seems that this is the newest patch and the reviewer(Pavel) stated that it is ready for commit. However, I noticed that this patch adds a Linux/UNIX only feature(not available on Windows). So I would like to ask cores if this is ok or not. -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp > Hi, > > attached is a v5 of this patch. Details below: > > On 8.12.2012 16:33, Andres Freund wrote: >> Hi Tomas, >> >> On 2012-11-27 14:55:59 +0100, Pavel Stehule wrote: >>>> attached is a v4 of the patch. There are not many changes, mostly some >>>> simple tidying up, except for handling the Windows. >> >> After a quick look I am not sure what all the talk about windows is >> about? instr_time.h seems to provide all you need, even for windows? The >> only issue of gettimeofday() for windows seems to be that it is that its >> not all that fast an not too high precision, but that shouldn't be a >> problem in this case? >> >> Could you expand a bit on the problems? > > As explained in the previous message, this is an existing problem with > unavailable timestamp. I'm not very fond of adding Linux-only features, > but fixing that was not the goal of this patch. > >>>>> * I had a problem with doc >> >> The current patch has conflict markers in the sgml source, there seems >> to have been some unresolved merge. Maybe that's all that causes the >> errors? >> >> Whats your problem with setting up the doc toolchain? > > Yeah, my fault because of incomplete merge. But the real culprit was a > missing refsect2. Fixed. > >> >>> issues: >>> >>> * empty lines with invisible chars (tabs) + and sometimes empty lines >>> after and before {} > > Fixed (I've removed the lines etc.) > >>> >>> * adjustment of start_time >>> >>> + * the desired interval */ >>> + while (agg->start_time >>> + agg_interval < INSTR_TIME_GET_DOUBLE(now)) >>> + >>> agg->start_time = agg->start_time + agg_interval; >>> >>> can "skip" one interval - so when transaction time will be larger or >>> similar to agg_interval - then results can be strange. We have to know >>> real length of interval >> >> Could you post a patch that adresses these issues? > > So, in the end I've rewritten the section advancing the start_time. Now > it works so that when skipping an interval (because of a very long > transaction), it will print lines even for those "empty" intervals. > > So for example with a transaction file containing a single query > > SELECT pg_sleep(1.5); > > and an interval length of 1 second, you'll get something like this: > > 1355009677 0 0 0 0 0 > 1355009678 1 1501028 2253085056784 1501028 1501028 > 1355009679 0 0 0 0 0 > 1355009680 1 1500790 2252370624100 1500790 1500790 > 1355009681 1 1500723 2252169522729 1500723 1500723 > 1355009682 0 0 0 0 0 > 1355009683 1 1500719 2252157516961 1500719 1500719 > 1355009684 1 1500703 2252109494209 1500703 1500703 > 1355009685 0 0 0 0 0 > > which is IMHO the best way to deal with this. > > I've fixed several minor issues, added a few comments. > > regards > Tomas
On 01/16/2013 05:59 PM, Tatsuo Ishii wrote: > Hi, > > I'm looking into this as a committer. It seems that this is the > newest patch and the reviewer(Pavel) stated that it is ready for > commit. However, I noticed that this patch adds a Linux/UNIX only > feature(not available on Windows). So I would like to ask cores if > this is ok or not. I haven't been following the thread, but if the complaint is that Windows doesn't have accurate high-resolution timers, which is what it kinda looks like from the rest of your message, then it's not true. Every version since Windows2000 has had QueryPerformanceCounter()/QueryPerformanceFrequency(). And we use it: see src/include/portability/instr_time.h If that's not the problem, then can someone please point me at the message that sets the problem out clearly, or else just recap it? cheers andrew
>> I'm looking into this as a committer. It seems that this is the >> newest patch and the reviewer(Pavel) stated that it is ready for >> commit. However, I noticed that this patch adds a Linux/UNIX only >> feature(not available on Windows). So I would like to ask cores if >> this is ok or not. > > I haven't been following the thread, but if the complaint is that > Windows doesn't have accurate high-resolution timers, which is what it > kinda looks like from the rest of your message, then it's not > true. Every version since Windows2000 has had > QueryPerformanceCounter()/QueryPerformanceFrequency(). And we use it: > see src/include/portability/instr_time.h In my understanding the problem is not related to resolution. > If that's not the problem, then can someone please point me at the > message that sets the problem out clearly, or else just recap it? It seems instr_time.h on Windows simply does not provide current timestamp. From pgbench.c: /* * if transaction finished, record the time it took in the log */ if (logfile && commands[st->state + 1]== NULL) { instr_time now; instr_time diff; double usec; INSTR_TIME_SET_CURRENT(now); diff = now; INSTR_TIME_SUBTRACT(diff, st->txn_begin); usec = (double)INSTR_TIME_GET_MICROSEC(diff); #ifndef WIN32 /* This is more than we really ought to know about instr_time */ fprintf(logfile, "%d %d %.0f%d %ld %ld\n", st->id, st->cnt, usec, st->use_file, (long) now.tv_sec, (long) now.tv_usec); #else /* On Windows, instr_time doesn't provide a timestamp anyway */ fprintf(logfile, "%d %d %.0f %d 0 0\n", st->id, st->cnt, usec, st->use_file); #endif } -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp
On 01/16/2013 06:48 PM, Tatsuo Ishii wrote: >>> I'm looking into this as a committer. It seems that this is the >>> newest patch and the reviewer(Pavel) stated that it is ready for >>> commit. However, I noticed that this patch adds a Linux/UNIX only >>> feature(not available on Windows). So I would like to ask cores if >>> this is ok or not. >> I haven't been following the thread, but if the complaint is that >> Windows doesn't have accurate high-resolution timers, which is what it >> kinda looks like from the rest of your message, then it's not >> true. Every version since Windows2000 has had >> QueryPerformanceCounter()/QueryPerformanceFrequency(). And we use it: >> see src/include/portability/instr_time.h > In my understanding the problem is not related to resolution. > >> If that's not the problem, then can someone please point me at the >> message that sets the problem out clearly, or else just recap it? > It seems instr_time.h on Windows simply does not provide current > timestamp. From pgbench.c: > > /* > * if transaction finished, record the time it took in the log > */ > if (logfile && commands[st->state + 1] == NULL) > { > instr_time now; > instr_time diff; > double usec; > > INSTR_TIME_SET_CURRENT(now); > diff = now; > INSTR_TIME_SUBTRACT(diff, st->txn_begin); > usec = (double) INSTR_TIME_GET_MICROSEC(diff); > > #ifndef WIN32 > /* This is more than we really ought to know about instr_time */ > fprintf(logfile, "%d %d %.0f %d %ld %ld\n", > st->id, st->cnt, usec, st->use_file, > (long) now.tv_sec, (long) now.tv_usec); > #else > /* On Windows, instr_time doesn't provide a timestamp anyway */ > fprintf(logfile, "%d %d %.0f %d 0 0\n", > st->id, st->cnt, usec, st->use_file); > #endif > } This might be way more than we want to do, but there is an article that describes some techniques for doing what seems to be missing (AIUI): <http://msdn.microsoft.com/en-us/magazine/cc163996.aspx> cheers andrew
>> It seems instr_time.h on Windows simply does not provide current >> timestamp. From pgbench.c: >> >> /* >> * if transaction finished, record the time it took in the log >> */ >> if (logfile && commands[st->state + 1] == NULL) >> { >> instr_time now; >> instr_time diff; >> double usec; >> >> INSTR_TIME_SET_CURRENT(now); >> diff = now; >> INSTR_TIME_SUBTRACT(diff, st->txn_begin); >> usec = (double) INSTR_TIME_GET_MICROSEC(diff); >> >> #ifndef WIN32 >> /* This is more than we really ought to know about instr_time */ >> fprintf(logfile, "%d %d %.0f %d %ld %ld\n", >> st->id, st->cnt, usec, st->use_file, >> (long) now.tv_sec, (long) now.tv_usec); >> #else >> /* On Windows, instr_time doesn't provide a timestamp anyway */ >> fprintf(logfile, "%d %d %.0f %d 0 0\n", >> st->id, st->cnt, usec, st->use_file); >> #endif >> } > > > This might be way more than we want to do, but there is an article > that describes some techniques for doing what seems to be missing > (AIUI): > > <http://msdn.microsoft.com/en-us/magazine/cc163996.aspx> Even this would be doable, I'm afraid it may not fit in 9.3 if we think about the current status of CF. So our choice would be: 1) Postpone the patch to 9.4 2) Commit the patch in 9.3 without Windows support I personally am ok with #2. We traditionally avoid particular paltform specific features on PostgreSQL. However I think the policiy could be losen for contrib staffs. Also pgbench is just a client program. We could always use pgbench on UNIX/Linux if we truely need the feature. What do you think? -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp
On 01/16/2013 08:05 PM, Tatsuo Ishii wrote: >>> It seems instr_time.h on Windows simply does not provide current >>> timestamp. From pgbench.c: >>> >>> /* >>> * if transaction finished, record the time it took in the log >>> */ >>> if (logfile && commands[st->state + 1] == NULL) >>> { >>> instr_time now; >>> instr_time diff; >>> double usec; >>> >>> INSTR_TIME_SET_CURRENT(now); >>> diff = now; >>> INSTR_TIME_SUBTRACT(diff, st->txn_begin); >>> usec = (double) INSTR_TIME_GET_MICROSEC(diff); >>> >>> #ifndef WIN32 >>> /* This is more than we really ought to know about instr_time */ >>> fprintf(logfile, "%d %d %.0f %d %ld %ld\n", >>> st->id, st->cnt, usec, st->use_file, >>> (long) now.tv_sec, (long) now.tv_usec); >>> #else >>> /* On Windows, instr_time doesn't provide a timestamp anyway */ >>> fprintf(logfile, "%d %d %.0f %d 0 0\n", >>> st->id, st->cnt, usec, st->use_file); >>> #endif >>> } >> >> This might be way more than we want to do, but there is an article >> that describes some techniques for doing what seems to be missing >> (AIUI): >> >> <http://msdn.microsoft.com/en-us/magazine/cc163996.aspx> > Even this would be doable, I'm afraid it may not fit in 9.3 if we > think about the current status of CF. So our choice would be: > > 1) Postpone the patch to 9.4 > > 2) Commit the patch in 9.3 without Windows support > > I personally am ok with #2. We traditionally avoid particular paltform > specific features on PostgreSQL. However I think the policiy could be > losen for contrib staffs. Also pgbench is just a client program. We > could always use pgbench on UNIX/Linux if we truely need the feature. > > What do you think? Fair enough, I was just trying to point out alternatives. We have committed platform-specific features before now. I hope it doesn't just get left like this, though. cheers andrew
>>> This might be way more than we want to do, but there is an article >>> that describes some techniques for doing what seems to be missing >>> (AIUI): >>> >>> <http://msdn.microsoft.com/en-us/magazine/cc163996.aspx> >> Even this would be doable, I'm afraid it may not fit in 9.3 if we >> think about the current status of CF. So our choice would be: >> >> 1) Postpone the patch to 9.4 >> >> 2) Commit the patch in 9.3 without Windows support >> >> I personally am ok with #2. We traditionally avoid particular paltform >> specific features on PostgreSQL. However I think the policiy could be >> losen for contrib staffs. Also pgbench is just a client program. We >> could always use pgbench on UNIX/Linux if we truely need the feature. >> >> What do you think? > > Fair enough, I was just trying to point out alternatives. We have > committed platform-specific features before now. I hope it doesn't > just get left like this, though. Yeah, I hope someone pick this up and propose as a TODO item. In the mean time, I'm going to commit the patch without Windows support unless there's objection. -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp
On Thu, Jan 17, 2013 at 2:35 AM, Tatsuo Ishii <ishii@postgresql.org> wrote: >>>> This might be way more than we want to do, but there is an article >>>> that describes some techniques for doing what seems to be missing >>>> (AIUI): >>>> >>>> <http://msdn.microsoft.com/en-us/magazine/cc163996.aspx> >>> Even this would be doable, I'm afraid it may not fit in 9.3 if we >>> think about the current status of CF. So our choice would be: >>> >>> 1) Postpone the patch to 9.4 >>> >>> 2) Commit the patch in 9.3 without Windows support >>> >>> I personally am ok with #2. We traditionally avoid particular paltform >>> specific features on PostgreSQL. However I think the policiy could be >>> losen for contrib staffs. Also pgbench is just a client program. We >>> could always use pgbench on UNIX/Linux if we truely need the feature. >>> >>> What do you think? >> >> Fair enough, I was just trying to point out alternatives. We have >> committed platform-specific features before now. I hope it doesn't >> just get left like this, though. We have committed platform-specific features before, but generally only when it's not *possible* to do them for all platforms. For example the posix_fadvise stuff isn't available on Windows at all, so there isn't much we can do there. > Yeah, I hope someone pick this up and propose as a TODO item. In the > mean time, I'm going to commit the patch without Windows support > unless there's objection. Perhaps we should actually hold off until someone committs to actually getting it fixed in the next version? If we do have that, then we can commit it as a partial feature, but if we just "hope someone picks it up", that's leaving it very loose.. --Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On Thu, Jan 17, 2013 at 9:36 AM, Magnus Hagander <magnus@hagander.net> wrote: > On Thu, Jan 17, 2013 at 2:35 AM, Tatsuo Ishii <ishii@postgresql.org> wrote: >>>>> This might be way more than we want to do, but there is an article >>>>> that describes some techniques for doing what seems to be missing >>>>> (AIUI): >>>>> >>>>> <http://msdn.microsoft.com/en-us/magazine/cc163996.aspx> >>>> Even this would be doable, I'm afraid it may not fit in 9.3 if we >>>> think about the current status of CF. So our choice would be: >>>> >>>> 1) Postpone the patch to 9.4 >>>> >>>> 2) Commit the patch in 9.3 without Windows support >>>> >>>> I personally am ok with #2. We traditionally avoid particular paltform >>>> specific features on PostgreSQL. However I think the policiy could be >>>> losen for contrib staffs. Also pgbench is just a client program. We >>>> could always use pgbench on UNIX/Linux if we truely need the feature. >>>> >>>> What do you think? >>> >>> Fair enough, I was just trying to point out alternatives. We have >>> committed platform-specific features before now. I hope it doesn't >>> just get left like this, though. > > We have committed platform-specific features before, but generally > only when it's not *possible* to do them for all platforms. For > example the posix_fadvise stuff isn't available on Windows at all, so > there isn't much we can do there. Right - having platform specific features for other reasons like lack of time is a slippery slope in my opinion. We should not get into such a habit or Windows will quickly become a second class platform as far as PostgreSQL features are concerned. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Jan 17, 2013 at 11:09 AM, Dave Page <dpage@pgadmin.org> wrote: > On Thu, Jan 17, 2013 at 9:36 AM, Magnus Hagander <magnus@hagander.net> wrote: >> On Thu, Jan 17, 2013 at 2:35 AM, Tatsuo Ishii <ishii@postgresql.org> wrote: >>>>>> This might be way more than we want to do, but there is an article >>>>>> that describes some techniques for doing what seems to be missing >>>>>> (AIUI): >>>>>> >>>>>> <http://msdn.microsoft.com/en-us/magazine/cc163996.aspx> >>>>> Even this would be doable, I'm afraid it may not fit in 9.3 if we >>>>> think about the current status of CF. So our choice would be: >>>>> >>>>> 1) Postpone the patch to 9.4 >>>>> >>>>> 2) Commit the patch in 9.3 without Windows support >>>>> >>>>> I personally am ok with #2. We traditionally avoid particular paltform >>>>> specific features on PostgreSQL. However I think the policiy could be >>>>> losen for contrib staffs. Also pgbench is just a client program. We >>>>> could always use pgbench on UNIX/Linux if we truely need the feature. >>>>> >>>>> What do you think? >>>> >>>> Fair enough, I was just trying to point out alternatives. We have >>>> committed platform-specific features before now. I hope it doesn't >>>> just get left like this, though. >> >> We have committed platform-specific features before, but generally >> only when it's not *possible* to do them for all platforms. For >> example the posix_fadvise stuff isn't available on Windows at all, so >> there isn't much we can do there. > > Right - having platform specific features for other reasons like lack > of time is a slippery slope in my opinion. We should not get into such > a habit or Windows will quickly become a second class platform as far > as PostgreSQL features are concerned. Especially since there is no lack of time - the functionality is there, it just looks (significantly) different. --Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Dne 17.01.2013 10:36, Magnus Hagander napsal: > On Thu, Jan 17, 2013 at 2:35 AM, Tatsuo Ishii <ishii@postgresql.org> > wrote: >>>>> This might be way more than we want to do, but there is an >>>>> article >>>>> that describes some techniques for doing what seems to be missing >>>>> (AIUI): >>>>> >>>>> <http://msdn.microsoft.com/en-us/magazine/cc163996.aspx> >>>> Even this would be doable, I'm afraid it may not fit in 9.3 if we >>>> think about the current status of CF. So our choice would be: >>>> >>>> 1) Postpone the patch to 9.4 >>>> >>>> 2) Commit the patch in 9.3 without Windows support >>>> >>>> I personally am ok with #2. We traditionally avoid particular >>>> paltform >>>> specific features on PostgreSQL. However I think the policiy >>>> could be >>>> losen for contrib staffs. Also pgbench is just a client program. >>>> We >>>> could always use pgbench on UNIX/Linux if we truely need the >>>> feature. >>>> >>>> What do you think? >>> >>> Fair enough, I was just trying to point out alternatives. We have >>> committed platform-specific features before now. I hope it doesn't >>> just get left like this, though. > > We have committed platform-specific features before, but generally > only when it's not *possible* to do them for all platforms. For > example the posix_fadvise stuff isn't available on Windows at all, so > there isn't much we can do there. Maybe, although this platform-dependence already exists in pgbench to some extent (the Windows branch is unable to log the timestamps of transactions). It would certainly be better if pgbench was able to handle Windows and Linux equally, but that was not the aim of this patch. >> Yeah, I hope someone pick this up and propose as a TODO item. In the >> mean time, I'm going to commit the patch without Windows support >> unless there's objection. > > Perhaps we should actually hold off until someone committs to > actually > getting it fixed in the next version? If we do have that, then we can > commit it as a partial feature, but if we just "hope someone picks it > up", that's leaving it very loose.. Well, given that I'm an author of that patch, that 'someone' would have to be me. The problem is I have access to absolutely no Windows machines, not mentioning the development tools (and that I have no clue about it). I vaguely remember there were people on this list doing Windows development on a virtual machine or something. Any interest in working on this / giving me some help? Tomas
Dne 17.01.2013 11:16, Magnus Hagander napsal: > On Thu, Jan 17, 2013 at 11:09 AM, Dave Page <dpage@pgadmin.org> > wrote: >> On Thu, Jan 17, 2013 at 9:36 AM, Magnus Hagander >> <magnus@hagander.net> wrote: >>> We have committed platform-specific features before, but generally >>> only when it's not *possible* to do them for all platforms. For >>> example the posix_fadvise stuff isn't available on Windows at all, >>> so >>> there isn't much we can do there. >> >> Right - having platform specific features for other reasons like >> lack >> of time is a slippery slope in my opinion. We should not get into >> such >> a habit or Windows will quickly become a second class platform as >> far >> as PostgreSQL features are concerned. > > Especially since there is no lack of time - the functionality is > there, it just looks (significantly) different. Really? Any link to relevant docs or something? When doing some research in this field, the only option I was able to come up with was combining gettimeofday() with the timing functionality, and do something like this: 1) call gettimeofday() at thread start, giving a common unix timestamp 2) measure the time from the thread start using the conters (for each transaction) 3) combine those values This might of course give up to a second difference compared to the actual time (because of the gettimeofday precision), but IMHO that's fine. An even simpler option would omit the (1), so the timestamps would start at 0. Or is there a better way? Tomas
On 01/17/2013 06:11 AM, Tomas Vondra wrote: > Dne 17.01.2013 11:16, Magnus Hagander napsal: >> On Thu, Jan 17, 2013 at 11:09 AM, Dave Page <dpage@pgadmin.org> wrote: >>> On Thu, Jan 17, 2013 at 9:36 AM, Magnus Hagander >>> <magnus@hagander.net> wrote: >>>> We have committed platform-specific features before, but generally >>>> only when it's not *possible* to do them for all platforms. For >>>> example the posix_fadvise stuff isn't available on Windows at all, so >>>> there isn't much we can do there. >>> >>> Right - having platform specific features for other reasons like lack >>> of time is a slippery slope in my opinion. We should not get into such >>> a habit or Windows will quickly become a second class platform as far >>> as PostgreSQL features are concerned. >> >> Especially since there is no lack of time - the functionality is >> there, it just looks (significantly) different. > > Really? Any link to relevant docs or something? > > When doing some research in this field, the only option I was able to > come up > with was combining gettimeofday() with the timing functionality, and > do something > like this: > > 1) call gettimeofday() at thread start, giving a common unix timestamp > 2) measure the time from the thread start using the conters (for > each transaction) > 3) combine those values > > This might of course give up to a second difference compared to the > actual time > (because of the gettimeofday precision), but IMHO that's fine. The link I posted showed a technique which essentially uses edge detection on gettimeofday() to get an accurate correspondence between the time of day and the performance counter, following which you can supposedly calculate the time of day with a high degree of accuracy just from the performance counter. You should be able to do this just once, at program start. If you don't care that much about the initial precision then your procedure should work fine, I think. cheers andrew
On 01/17/2013 06:04 AM, Tomas Vondra wrote: > The problem is I have access to absolutely no Windows machines, > not mentioning the development tools (and that I have no clue about it). > > I vaguely remember there were people on this list doing Windows > development > on a virtual machine or something. Any interest in working on this / > giving > me some help? > > One of the items on my very long TODO list (see stuff elsewhere about committers not doing enough reviewing and committing) is to create a package that can easily be run to set Windows Postgres development environments for MSVC, Mingw and Cygwin on Amazon, GoGrid etc. Once I get that done I'll be a lot less sympathetic to "I don't have access to Windows" pleas. Windows does run in a VM very well, but if you're going to set it up on your own VM environment, (e.h. VirtualBox or KVM/qemu) you need your own legal copy to install there. If you don't already have one, that will set you back about $140.00 (for w7 Pro) in the USA. Note that that's significantly better than the situation with OSX, which you can't run at all except on Apple hardware. cheers andrew
On Thu, Jan 17, 2013 at 1:29 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > > On 01/17/2013 06:04 AM, Tomas Vondra wrote: >> >> The problem is I have access to absolutely no Windows machines, >> not mentioning the development tools (and that I have no clue about it). >> >> I vaguely remember there were people on this list doing Windows >> development >> on a virtual machine or something. Any interest in working on this / >> giving >> me some help? >> >> > > > One of the items on my very long TODO list (see stuff elsewhere about > committers not doing enough reviewing and committing) is to create a package > that can easily be run to set Windows Postgres development environments for > MSVC, Mingw and Cygwin on Amazon, GoGrid etc. FYI, I have a similar item on my TODO list, though I was looking primarily at VC++ on AWS. It did get close to the top last week, but then I got busy with other things :-/. Anyway, I'd suggest we ping each other if either actually get started, to avoid duplication of effort. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Jan 17, 2013 at 2:29 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > > On 01/17/2013 06:04 AM, Tomas Vondra wrote: >> >> The problem is I have access to absolutely no Windows machines, >> not mentioning the development tools (and that I have no clue about it). >> >> I vaguely remember there were people on this list doing Windows >> development >> on a virtual machine or something. Any interest in working on this / >> giving >> me some help? >> >> > > > One of the items on my very long TODO list (see stuff elsewhere about > committers not doing enough reviewing and committing) is to create a package > that can easily be run to set Windows Postgres development environments for > MSVC, Mingw and Cygwin on Amazon, GoGrid etc. > > Once I get that done I'll be a lot less sympathetic to "I don't have access > to Windows" pleas. > > Windows does run in a VM very well, but if you're going to set it up on your > own VM environment, (e.h. VirtualBox or KVM/qemu) you need your own legal > copy to install there. If you don't already have one, that will set you back > about $140.00 (for w7 Pro) in the USA. Note that that's significantly better > than the situation with OSX, which you can't run at all except on Apple > hardware. Yeah. I used to have an AMI with the VS environment preinstalled on Amazon, but I managed to fat finger things and delete it at some point and haven't really had time to rebuild it. Having a script that would download and install all the pre-requisites on such a box would be *great*. Then you could get up and going pretty quickly, and getting a Windows box up running for a few hours there is almost free, and you don't have to deal with licensing hassles. (Of course, the AMI method doesn't work all the way since you'd be distributing Visual Studio, but if we can have a script that auto-downloads-and-installs it as necessary we can get around that) --Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
> On Thu, Jan 17, 2013 at 2:29 PM, Andrew Dunstan <andrew@dunslane.net> wrote: >> >> On 01/17/2013 06:04 AM, Tomas Vondra wrote: >>> >>> The problem is I have access to absolutely no Windows machines, >>> not mentioning the development tools (and that I have no clue about it). >>> >>> I vaguely remember there were people on this list doing Windows >>> development >>> on a virtual machine or something. Any interest in working on this / >>> giving >>> me some help? >>> >>> >> >> >> One of the items on my very long TODO list (see stuff elsewhere about >> committers not doing enough reviewing and committing) is to create a package >> that can easily be run to set Windows Postgres development environments for >> MSVC, Mingw and Cygwin on Amazon, GoGrid etc. >> >> Once I get that done I'll be a lot less sympathetic to "I don't have access >> to Windows" pleas. >> >> Windows does run in a VM very well, but if you're going to set it up on your >> own VM environment, (e.h. VirtualBox or KVM/qemu) you need your own legal >> copy to install there. If you don't already have one, that will set you back >> about $140.00 (for w7 Pro) in the USA. Note that that's significantly better >> than the situation with OSX, which you can't run at all except on Apple >> hardware. > > Yeah. I used to have an AMI with the VS environment preinstalled on > Amazon, but I managed to fat finger things and delete it at some point > and haven't really had time to rebuild it. > > Having a script that would download and install all the pre-requisites > on such a box would be *great*. Then you could get up and going pretty > quickly, and getting a Windows box up running for a few hours there is > almost free, and you don't have to deal with licensing hassles. > > (Of course, the AMI method doesn't work all the way since you'd be > distributing Visual Studio, but if we can have a script that > auto-downloads-and-installs it as necessary we can get around that) So if my understating is correct, 1)Tomas Vondra commits to work on Windows support for 9.4, 2)on the assumption that one of Andrew Dunstan, Dave Page or Magnus Hagander will help him in Windows development. Ok? If so, I can commit the patch for 9.3 without Windows support. If not, I will move the patch to next CF (for 9.4). Please correct me if I am wrong. -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp
On Thu, Jan 17, 2013 at 7:43 PM, Tatsuo Ishii <ishii@postgresql.org> wrote: > So if my understating is correct, 1)Tomas Vondra commits to work on > Windows support for 9.4, 2)on the assumption that one of Andrew > Dunstan, Dave Page or Magnus Hagander will help him in Windows > development. > > Ok? If so, I can commit the patch for 9.3 without Windows support. If > not, I will move the patch to next CF (for 9.4). > > Please correct me if I am wrong. +1 for this approach. I agree with Dave and Magnus that we don't want Windows to become a second-class platform, but this patch isn't making it so. The #ifdef that peeks inside of an instr_time is already there, and it's not Tomas's fault that nobody has gotten around to fixing it before now. OTOH, I think that this sort of thing is quite wrong: +#ifndef WIN32 + " --aggregate-interval NUM\n" + " aggregate data over NUM seconds\n" +#endif The right approach if this can't be supported on Windows is to still display the option in the --help output, and to display an error message if the user tries to use it, saying that it is not currently supported on Windows. That fact should also be mentioned in the documentation. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> On Thu, Jan 17, 2013 at 7:43 PM, Tatsuo Ishii <ishii@postgresql.org> wrote: >> So if my understating is correct, 1)Tomas Vondra commits to work on >> Windows support for 9.4, 2)on the assumption that one of Andrew >> Dunstan, Dave Page or Magnus Hagander will help him in Windows >> development. >> >> Ok? If so, I can commit the patch for 9.3 without Windows support. If >> not, I will move the patch to next CF (for 9.4). >> >> Please correct me if I am wrong. > > +1 for this approach. I agree with Dave and Magnus that we don't want > Windows to become a second-class platform, but this patch isn't making > it so. The #ifdef that peeks inside of an instr_time is already > there, and it's not Tomas's fault that nobody has gotten around to > fixing it before now. Right. > OTOH, I think that this sort of thing is quite wrong: > > +#ifndef WIN32 > + " --aggregate-interval NUM\n" > + " aggregate data over NUM seconds\n" > +#endif > > The right approach if this can't be supported on Windows is to still > display the option in the --help output, and to display an error > message if the user tries to use it, saying that it is not currently > supported on Windows. That fact should also be mentioned in the > documentation. Agreed. This seems to be much better approach. -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp
<div class="moz-cite-prefix">On 01/17/2013 09:36 PM, Magnus Hagander wrote:<br /></div><blockquote cite="mid:CABUevEyeiSFk=428ThV5tOR=Xsz9rpOno=uhAznVzgNG3woo7w@mail.gmail.com"type="cite"><br /><pre wrap=""> Yeah. I used to have an AMI with the VS environment preinstalled on Amazon, but I managed to fat finger things and delete it at some point and haven't really had time to rebuild it. Having a script that would download and install all the pre-requisites on such a box would be *great*.</pre></blockquote> I'm working on it:<br /><br /><a href="https://github.com/2ndQuadrant/pg_build_win">https://github.com/2ndQuadrant/pg_build_win</a><br/><br /><a href="http://blog.2ndquadrant.com/easier-postgresql-builds-for-windows/">http://blog.2ndquadrant.com/easier-postgresql-builds-for-windows/</a><br /><br/> I've identified the silent install procedures for most of the required tools (see the docs) and got build recipeswritten for some of the library dependencies. The next planned step is to have the scripts automatically downloadand silent-install Visual Studio, etc, rather than have the user run the command lines given in the README manually.<br/><br /> It's usable as-is, I just need the time to finish it off. The goal is to have a script that turns buildingPostgreSQL on a clean fresh Windows install into:<br /><br /> - Download ActivePerl<br /> - Install ActivePerl<br/> - Run "buildgit.pl check install"<br /><br /> Right now it takes a fair bit more than that, but it's alreadybetter than a fully manual build.<br /><br /><blockquote cite="mid:CABUevEyeiSFk=428ThV5tOR=Xsz9rpOno=uhAznVzgNG3woo7w@mail.gmail.com"type="cite"><pre wrap="">Then you could getup and going pretty quickly, and getting a Windows box up running for a few hours there is almost free, and you don't have to deal with licensing hassles. (Of course, the AMI method doesn't work all the way since you'd be distributing Visual Studio, but if we can have a script that auto-downloads-and-installs it as necessary we can get around that) </pre></blockquote> I've found EC2 to be unusably slow for Windows builds, with a medium instance taking an hour and a halfto do a simple build and "vcregress check". They're also restrictive in disk space terms, so you land up needing to adda second EBS volume.<br /><br /> A local kvm instance works well if a physical host isn't available; so do some of thealternative cloud providers like LunaCloud, which seems to perform significantly better in the quick testing I did. Ihaven't tried GoGrid yet.<br /><br /> Many of us have Windows license stickers on laptops/desktops, even if we don't usethe thing, so for a signficiant proportion of people it's as simple as downloading Windows install media ( <a href="http://blog.ringerc.id.au/2012/05/you-can-download-legal-windows-7-iso.html">http://blog.ringerc.id.au/2012/05/you-can-download-legal-windows-7-iso.html</a>) andinstalling a KVM instance then shapshotting it.<br /><br /><br /> I've also put together a Jenkins server that runs buildson Windows whenever they're pushed to watched git repos. I'd love to make this public, but unfortunately allowing awide group to run arbitrary code on the current build box isn't something I can afford. I'd need a system that launcheda snapshot Windows instance for each build and then destroyed it at the end. This is entirely practical with somethinglike KVM, so if I ever get the chance to work on a Jenkins plugin to do that (or to launch/destroy Windows cloudinstances automatically), it's possible a semi-public Windows build testing service may be possible.<br /><br /> Whilewe're in fantasy land, the next step would be adding git URLs and branch names to the commitfest app, so it could pinga continuous integration server to build-test any new patch added to the CF. Right now I'm doing that manually when Ihave time, but it's slow going.<br /><br /><pre class="moz-signature" cols="72">-- Craig Ringer <a class="moz-txt-link-freetext"href="http://www.2ndQuadrant.com/">http://www.2ndQuadrant.com/</a>PostgreSQL Development, 24x7Support, Training & Services</pre>
On Mon, Jan 21, 2013 at 10:01:29AM +0800, Craig Ringer wrote: > I've found EC2 to be unusably slow for Windows builds, with a medium > instance taking an hour and a half to do a simple build and "vcregress > check". They're also restrictive in disk space terms, so you land up > needing to add a second EBS volume. Yikes. The "build DEBUG" step takes 5-7 minutes for me; I use EBS-optimized m1.xlarge spot instances in US East (lately about US$0.19/hr). Fairly sure I once used a c1.medium, though, and it still took <10 minutes. I don't know why your experience has been so different.
On 01/21/2013 08:55 PM, Noah Misch wrote: > On Mon, Jan 21, 2013 at 10:01:29AM +0800, Craig Ringer wrote: >> I've found EC2 to be unusably slow for Windows builds, with a medium >> instance taking an hour and a half to do a simple build and "vcregress >> check". They're also restrictive in disk space terms, so you land up >> needing to add a second EBS volume. > Yikes. The "build DEBUG" step takes 5-7 minutes for me; I use EBS-optimized > m1.xlarge spot instances in US East (lately about US$0.19/hr). Fairly sure I > once used a c1.medium, though, and it still took <10 minutes. I don't know > why your experience has been so different. I was using m1.medium instances, but the same instance type gets Linux builds done in 15-20 mins. Slow, but not that slow. Performance was consistently miserable across several instances, including one full clean rebuild from scratch. Weird. I should perhaps give the bigger instances a go. Unfortunately Jenkins can't auto-start and auto-stop Windows instances yet and I don't have time to improve the Jenkins ec2 plugin right now, so using any instance bigger than an m1.medium would pretty much require manually stopping and starting it for builds. Yick. Instead I'm using my home media PC, a Pentium G630 (like a cut-down Core 2 i3) with a laptop hard drive. It completes builds in 20 minutes. My more power-hungry laptop does it in 7. I was never able to determine why the Windows instances were so much slower than the corresponding Linux instance of the same type. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Jan 21, 2013 at 1:00 PM, Craig Ringer <craig@2ndquadrant.com> wrote: > On 01/21/2013 08:55 PM, Noah Misch wrote: >> On Mon, Jan 21, 2013 at 10:01:29AM +0800, Craig Ringer wrote: >>> I've found EC2 to be unusably slow for Windows builds, with a medium >>> instance taking an hour and a half to do a simple build and "vcregress >>> check". They're also restrictive in disk space terms, so you land up >>> needing to add a second EBS volume. >> Yikes. The "build DEBUG" step takes 5-7 minutes for me; I use EBS-optimized >> m1.xlarge spot instances in US East (lately about US$0.19/hr). Fairly sure I >> once used a c1.medium, though, and it still took <10 minutes. I don't know >> why your experience has been so different. > I was using m1.medium instances, but the same instance type gets Linux > builds done in 15-20 mins. Slow, but not that slow. Performance was > consistently miserable across several instances, including one full > clean rebuild from scratch. Weird. FYI, in my experience VC++ is typically much faster than GCC, on comparable hardware - particularly with C++. > I should perhaps give the bigger instances a go. Unfortunately Jenkins > can't auto-start and auto-stop Windows instances yet and I don't have > time to improve the Jenkins ec2 plugin right now, so using any instance > bigger than an m1.medium would pretty much require manually stopping and > starting it for builds. Yick. It should be trivial to script I would think - it's a one-liner to create an instance with ec2 tools. > Instead I'm using my home media PC, a Pentium G630 (like a cut-down Core > 2 i3) with a laptop hard drive. It completes builds in 20 minutes. My > more power-hungry laptop does it in 7. > > I was never able to determine why the Windows instances were so much > slower than the corresponding Linux instance of the same type. Full vs. para-virtualisation perhaps? -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 01/21/2013 08:11 AM, Dave Page wrote: >> >> I was never able to determine why the Windows instances were so much >> slower than the corresponding Linux instance of the same type. > Full vs. para-virtualisation perhaps? > No, Windows builds just are slower. For some time the buildfarm has been reporting run times for various members, so there's plenty of data on this. For example, nightjar and currawong are respectively FreeBSD/gcc and WindowsXP/VC2008 members running in VMs on the same host. currawong takes three times as long for a buildfarm run even though it's doing less work. cheers andrdew
On Mon, Jan 21, 2013 at 1:59 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > > On 01/21/2013 08:11 AM, Dave Page wrote: > >>> >>> I was never able to determine why the Windows instances were so much >>> slower than the corresponding Linux instance of the same type. >> >> Full vs. para-virtualisation perhaps? >> > > No, Windows builds just are slower. For some time the buildfarm has been > reporting run times for various members, so there's plenty of data on this. > For example, nightjar and currawong are respectively FreeBSD/gcc and > WindowsXP/VC2008 members running in VMs on the same host. currawong takes > three times as long for a buildfarm run even though it's doing less work. Hmm, OK. I don't build PostgreSQL interactively enough to notice I guess. For C++ it's definitely the other way round - I can build pgAdmin in a fraction of the time in a Windows VM than I can on the host Mac it runs on. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Jan 21, 2013 at 3:03 PM, Dave Page <dpage@pgadmin.org> wrote: > On Mon, Jan 21, 2013 at 1:59 PM, Andrew Dunstan <andrew@dunslane.net> wrote: >> >> On 01/21/2013 08:11 AM, Dave Page wrote: >> >>>> >>>> I was never able to determine why the Windows instances were so much >>>> slower than the corresponding Linux instance of the same type. >>> >>> Full vs. para-virtualisation perhaps? >>> >> >> No, Windows builds just are slower. For some time the buildfarm has been >> reporting run times for various members, so there's plenty of data on this. >> For example, nightjar and currawong are respectively FreeBSD/gcc and >> WindowsXP/VC2008 members running in VMs on the same host. currawong takes >> three times as long for a buildfarm run even though it's doing less work. > > Hmm, OK. I don't build PostgreSQL interactively enough to notice I > guess. For C++ it's definitely the other way round - I can build > pgAdmin in a fraction of the time in a Windows VM than I can on the > host Mac it runs on. Yes, for C++ it's a difference in at least one order of magnitude. For C it definitely isn't. MSVC tends to be faster than gcc (both on windows), which I think is mostly because it builds multiple files in one run. However, actually starting each build step takes significantly longer. We've also added some things like the DEF file magic that can definitely take quite some time, particularly when building the backend. --Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
>> On Thu, Jan 17, 2013 at 7:43 PM, Tatsuo Ishii <ishii@postgresql.org> wrote: >>> So if my understating is correct, 1)Tomas Vondra commits to work on >>> Windows support for 9.4, 2)on the assumption that one of Andrew >>> Dunstan, Dave Page or Magnus Hagander will help him in Windows >>> development. >>> >>> Ok? If so, I can commit the patch for 9.3 without Windows support. If >>> not, I will move the patch to next CF (for 9.4). >>> >>> Please correct me if I am wrong. >> >> +1 for this approach. I agree with Dave and Magnus that we don't want >> Windows to become a second-class platform, but this patch isn't making >> it so. The #ifdef that peeks inside of an instr_time is already >> there, and it's not Tomas's fault that nobody has gotten around to >> fixing it before now. > > Right. > >> OTOH, I think that this sort of thing is quite wrong: >> >> +#ifndef WIN32 >> + " --aggregate-interval NUM\n" >> + " aggregate data over NUM seconds\n" >> +#endif >> >> The right approach if this can't be supported on Windows is to still >> display the option in the --help output, and to display an error >> message if the user tries to use it, saying that it is not currently >> supported on Windows. That fact should also be mentioned in the >> documentation. > > Agreed. This seems to be much better approach. Here is the new patch. -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c index 3ca120f..e8867a6 100644 --- a/contrib/pgbench/pgbench.c +++ b/contrib/pgbench/pgbench.c @@ -153,6 +153,7 @@ char *index_tablespace = NULL;bool use_log; /* log transaction latencies to afile */bool use_quiet; /* quiet logging onto stderr */ +int agg_interval; /* log aggregates instead of individual transactions */bool is_connect; /* establish connection for each transaction */bool is_latencies; /* report per-command latencies */int main_pid; /* main process id used in log filename */ @@ -248,6 +249,18 @@ typedef struct char *argv[MAX_ARGS]; /* command word list */} Command; +typedef struct +{ + + long start_time; /* when does the interval start */ + int cnt; /* number of transactions */ + double min_duration; /* min/max durations */ + double max_duration; + double sum; /* sum(duration), sum(duration^2) - for estimates */ + double sum2; + +} AggVals; +static Command **sql_files[MAX_FILES]; /* SQL script files */static int num_files; /* number of scriptfiles */static int num_commands = 0; /* total number of Command structs */ @@ -381,6 +394,8 @@ usage(void) " -l write transaction times to log file\n" " --sampling-rateNUM\n" " fraction of transactions to log (e.g. 0.01 for 1%% sample)\n" + " --aggregate-interval NUM\n" + " aggregate data over NUM seconds\n" " -M simple|extended|prepared\n" " protocol for submitting queries to server (default: simple)\n" " -n do not run VACUUM beforetests\n" @@ -834,9 +849,25 @@ clientDone(CState *st, bool ok) return false; /* always false */} +static +void agg_vals_init(AggVals * aggs, instr_time start) +{ + /* basic counters */ + aggs->cnt = 0; /* number of transactions */ + aggs->sum = 0; /* SUM(duration) */ + aggs->sum2 = 0; /* SUM(duration*duration) */ + + /* min and max transaction duration */ + aggs->min_duration = 0; + aggs->max_duration = 0; + + /* start of the current interval */ + aggs->start_time = INSTR_TIME_GET_DOUBLE(start); +} +/* return false iff client should be disconnected */static bool -doCustom(TState *thread, CState *st, instr_time *conn_time, FILE *logfile) +doCustom(TState *thread, CState *st, instr_time *conn_time, FILE *logfile, AggVals * agg){ PGresult *res; Command **commands; @@ -901,22 +932,74 @@ top: if (sample_rate == 0.0 || pg_erand48(thread->random_state) <= sample_rate) { - INSTR_TIME_SET_CURRENT(now); diff = now; INSTR_TIME_SUBTRACT(diff, st->txn_begin); usec = (double) INSTR_TIME_GET_MICROSEC(diff); + /* should we aggregate the results or not? */ + if (agg_interval > 0) + { + /* are we still in the same interval? if yes, accumulate the + * values (print them otherwise) */ + if (agg->start_time + agg_interval >= INSTR_TIME_GET_DOUBLE(now)) + { + agg->cnt += 1; + agg->sum += usec; + agg->sum2 += usec * usec; + + /* first in this aggregation interval */ + if ((agg->cnt == 1) || (usec < agg->min_duration)) + agg->min_duration = usec; + + if ((agg->cnt == 1) || (usec > agg->max_duration)) + agg->max_duration = usec; + } + else + { + /* Loop until we reach the interval of the current transaction (and + * print all the empty intervals in between). */ + while (agg->start_time + agg_interval < INSTR_TIME_GET_DOUBLE(now)) + { + /* This is a non-Windows branch (thanks to the ifdef in usage), so + * we don't need to handle this in a special way (see below). */ + fprintf(logfile, "%ld %d %.0f %.0f %.0f %.0f\n", + agg->start_time, agg->cnt, agg->sum, agg->sum2, + agg->min_duration, agg->max_duration); + + /* move to the next inteval */ + agg->start_time = agg->start_time + agg_interval; + + /* reset for "no transaction" intervals */ + agg->cnt = 0; + agg->min_duration = 0; + agg->max_duration = 0; + agg->sum = 0; + agg->sum2 = 0; + } + + /* and now update the reset values (include the current) */ + agg->cnt = 1; + agg->min_duration = usec; + agg->max_duration = usec; + agg->sum = usec; + agg->sum2 = usec * usec; + } + } + else + { + /* no, print raw transactions */#ifndef WIN32 - /* This is more than we really ought to know about instr_time */ - fprintf(logfile, "%d %d %.0f %d %ld %ld\n", - st->id, st->cnt, usec, st->use_file, - (long) now.tv_sec, (long) now.tv_usec); + /* This is more than we really ought to know about instr_time */ + fprintf(logfile, "%d %d %.0f %d %ld %ld\n", + st->id, st->cnt, usec, st->use_file, + (long) now.tv_sec, (long) now.tv_usec);#else - /* On Windows, instr_time doesn't provide a timestamp anyway */ - fprintf(logfile, "%d %d %.0f %d 0 0\n", - st->id, st->cnt, usec, st->use_file); + /* On Windows, instr_time doesn't provide a timestamp anyway */ + fprintf(logfile, "%d %d %.0f %d 0 0\n", + st->id, st->cnt, usec, st->use_file);#endif + } } } @@ -1964,6 +2047,9 @@ main(int argc, char **argv) {"tablespace", required_argument, NULL, 2}, {"unlogged-tables",no_argument, &unlogged_tables, 1}, {"sampling-rate", required_argument, NULL, 4}, +#ifndef WIN32 + {"aggregate-interval", required_argument, NULL, 5}, +#endif {NULL, 0, NULL, 0} }; @@ -2202,6 +2288,19 @@ main(int argc, char **argv) exit(1); } break; + case 5: +#ifdef WIN32 + fprintf(stderr, "--aggregate-interval is not currently supported on Windows"); + exit(1); +#else + agg_interval = atoi(optarg); + if (agg_interval <= 0) + { + fprintf(stderr, "invalid number of seconds for aggregation: %d\n", agg_interval); + exit(1); + } +#endif + break; default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"),progname); exit(1); @@ -2251,6 +2350,28 @@ main(int argc, char **argv) exit(1); } + /* --sampling-rate may must not be used with --aggregate-interval */ + if (sample_rate > 0.0 && agg_interval > 0) + { + fprintf(stderr, "log sampling (--sampling-rate) and aggregation (--aggregate-interval) can't be used at the sametime\n"); + exit(1); + } + + if (agg_interval > 0 && (! use_log)) { + fprintf(stderr, "log aggregation is allowed only when actually logging transactions\n"); + exit(1); + } + + if ((duration > 0) && (agg_interval > duration)) { + fprintf(stderr, "number of seconds for aggregation (%d) must not be higher that test duration (%d)\n", agg_interval,duration); + exit(1); + } + + if ((duration > 0) && (agg_interval > 0) && (duration % agg_interval != 0)) { + fprintf(stderr, "duration (%d) must be a multiple of aggregation interval (%d)\n", duration, agg_interval); + exit(1); + } + /* * is_latencies only works with multiple threads in thread-based * implementations, not fork-based ones, becauseit supposes that the @@ -2510,7 +2631,10 @@ threadRun(void *arg) int remains = nstate; /* number of remaining clients */ int i; + AggVals aggs; + result = pg_malloc(sizeof(TResult)); + INSTR_TIME_SET_ZERO(result->conn_time); /* open log file if requested */ @@ -2545,6 +2669,8 @@ threadRun(void *arg) INSTR_TIME_SET_CURRENT(result->conn_time); INSTR_TIME_SUBTRACT(result->conn_time,thread->start_time); + agg_vals_init(&aggs, thread->start_time); + /* send start up queries in async manner */ for (i = 0; i < nstate; i++) { @@ -2553,7 +2679,7 @@ threadRun(void *arg) int prev_ecnt = st->ecnt; st->use_file = getrand(thread,0, num_files - 1); - if (!doCustom(thread, st, &result->conn_time, logfile)) + if (!doCustom(thread, st, &result->conn_time, logfile, &aggs)) remains--; /* I've aborted*/ if (st->ecnt > prev_ecnt && commands[st->state]->type == META_COMMAND) @@ -2655,7 +2781,7 @@ threadRun(void *arg) if (st->con && (FD_ISSET(PQsocket(st->con), &input_mask) || commands[st->state]->type == META_COMMAND)) { - if (!doCustom(thread, st, &result->conn_time, logfile)) + if (!doCustom(thread, st, &result->conn_time, logfile, &aggs)) remains--; /* I'veaborted */ } @@ -2682,7 +2808,6 @@ done: return result;} -/* * Support for duration option: set timer_exceeded after so many seconds. */ diff --git a/doc/src/sgml/pgbench.sgml b/doc/src/sgml/pgbench.sgml index 58686b1..fc1c051 100644 --- a/doc/src/sgml/pgbench.sgml +++ b/doc/src/sgml/pgbench.sgml @@ -346,6 +346,21 @@ pgbench <optional> <replaceable>options</> </optional> <replaceable>dbname</> </varlistentry> <varlistentry> + <term><option>--aggregate-interval</option> <replaceable>seconds</></term> + <listitem> + <para> + Length of aggregation interval (in seconds). May be used only together + with <application>-l</application> - with this option, the log contains + per-interval summary (number of transactions, min/max latency and two + additional fields useful for variance estimation). + </para> + <para> + This option is not currently supported on Windows. + </para> + </listitem> + </varlistentry> + + <varlistentry> <term><option>-M</option> <replaceable>querymode</></term> <listitem> <para> @@ -741,8 +756,9 @@ END; <title>Per-Transaction Logging</title> <para> - With the <option>-l</> option, <application>pgbench</> writes the time - taken by each transaction to a log file. The log file will be named + With the <option>-l</> option but without the <option>--aggregate-interval</option>, + <application>pgbench</> writes the time taken by each transaction + to a log file. The log file will be named <filename>pgbench_log.<replaceable>nnn</></filename>, where <replaceable>nnn</>is the PID of the pgbench process. If the <option>-j</> option is 2 or higher, creating multiple worker @@ -788,6 +804,45 @@ END; </refsect2> <refsect2> + <title>Aggregated Logging</title> + + <para> + With the <option>--aggregate-interval</option> option, the logs use a bit different format: + +<synopsis> +<replaceable>interval_start</> <replaceable>num_of_transactions</> <replaceable>latency_sum</> <replaceable>latency_2_sum</><replaceable>min_latency</> <replaceable>max_latency</> +</synopsis> + + where <replaceable>interval_start</> is the start of the interval (UNIX epoch + format timestamp), <replaceable>num_of_transactions</> is the number of transactions + within the interval, <replaceable>latency_sum</replaceable> is a sum of latencies + (so you can compute average latency easily). The following two fields are useful + for variance estimation - <replaceable>latency_sum</> is a sum of latencies and + <replaceable>latency_2_sum</> is a sum of 2nd powers of latencies. The last two + fields are <replaceable>min_latency</> - a minimum latency within the interval, and + <replaceable>max_latency</> - maximum latency within the interval. A transaction is + counted into the interval when it was committed. + </para> + + <para> + Here is example outputs: +<screen> +1345828501 5601 1542744 483552416 61 2573 +1345828503 7884 1979812 565806736 60 1479 +1345828505 7208 1979422 567277552 59 1391 +1345828507 7685 1980268 569784714 60 1398 +1345828509 7073 1979779 573489941 236 1411 +</screen></para> + + <para> + Notice that while the plain (unaggregated) log file contains index + of the custom script files, the aggregated log does not. Therefore if + you need per script data, you need to aggregate the data on your own. + </para> + + </refsect2> + + <refsect2> <title>Per-Statement Latencies</title> <para>
>>> On Thu, Jan 17, 2013 at 7:43 PM, Tatsuo Ishii <ishii@postgresql.org> wrote: >>>> So if my understating is correct, 1)Tomas Vondra commits to work on >>>> Windows support for 9.4, 2)on the assumption that one of Andrew >>>> Dunstan, Dave Page or Magnus Hagander will help him in Windows >>>> development. >>>> >>>> Ok? If so, I can commit the patch for 9.3 without Windows support. If >>>> not, I will move the patch to next CF (for 9.4). >>>> >>>> Please correct me if I am wrong. >>> >>> +1 for this approach. I agree with Dave and Magnus that we don't want >>> Windows to become a second-class platform, but this patch isn't making >>> it so. The #ifdef that peeks inside of an instr_time is already >>> there, and it's not Tomas's fault that nobody has gotten around to >>> fixing it before now. >> >> Right. >> >>> OTOH, I think that this sort of thing is quite wrong: >>> >>> +#ifndef WIN32 >>> + " --aggregate-interval NUM\n" >>> + " aggregate data over NUM seconds\n" >>> +#endif >>> >>> The right approach if this can't be supported on Windows is to still >>> display the option in the --help output, and to display an error >>> message if the user tries to use it, saying that it is not currently >>> supported on Windows. That fact should also be mentioned in the >>> documentation. >> >> Agreed. This seems to be much better approach. > > Here is the new patch. Committed (with minor fix). -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp