Skip site navigation (1) Skip section navigation (2)

review: pgbench - aggregation of info written into log

From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Cc: Tomáš Vondra <tv(at)fuzzy(dot)cz>
Subject: review: pgbench - aggregation of info written into log
Date: 2012-10-02 18:08:26
Message-ID: CAFj8pRAxRxnixx=UUHsnOZSt6q-cjN4MMK6fNSjxJesFW8wZGw@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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


Responses

pgsql-hackers by date

Next:From: Simon RiggsDate: 2012-10-02 18:11:03
Subject: Re: Prevent restored WAL files from being archived again Re: Unnecessary WAL archiving after failover
Previous:From: Fujii MasaoDate: 2012-10-02 18:06:14
Subject: Re: Prevent restored WAL files from being archived again Re: Unnecessary WAL archiving after failover

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group