Re: TODO : Allow parallel cores to be used by vacuumdb [ WIP ]

On Sat, Oct 25, 2014 at 5:52 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
>
> ***************
> *** 358,363 **** handle_sigint(SIGNAL_ARGS)
> --- 358,364 ----
>
> /* Send QueryCancel if we are processing a database query */
> if (cancelConn != NULL)
> {
> + inAbort = true;
> if (PQcancel(cancelConn, errbuf, sizeof(errbuf)))
> fprintf(stderr, _("Cancel request sent\n"));
> else
>
> Do we need to set inAbort flag incase PQcancel is successful?
> Basically if PQCancel fails due to any reason, I think behaviour
> can be undefined as the executing thread can assume that cancel is
> done.
>
> *** 391,396 **** consoleHandler(DWORD dwCtrlType)
> --- 392,399 ----
> EnterCriticalSection
> (&cancelConnLock);
> if (cancelConn != NULL)
> {
> + inAbort =
> true;
> +
>
> You have set this flag in case of windows handler, however the same
> is never used incase of windows, are you expecting any use of this
> flag for windows?

Going further with verification of this patch, I found below issue:
Run the testcase.sql file at below link:
http://www.postgresql.org/message-id/4205E661176A124FAF891E0A6BA9135266347F25@szxeml509-mbs.china.huawei.com
./vacuumdb --analyze-in-stages -j 8 -d postgres
Generating minimal optimizer statistics (1 target)
Segmentation fault

Server Log:
ERROR: syntax error at or near "minimal" at character 12
STATEMENT: ANALYZE ng minimal optimizer statistics (1 target)
LOG: could not receive data from client: Connection reset by peer

Fixed below issues and attached an updated patch with mail:

1.
make check for docs gives below errors:
{ \
echo "<!ENTITY version \"9.5devel\">"; \
echo "<!ENTITY majorversion \"9.5\">"; \
} > version.sgml
'/usr/bin/perl' ./mk_feature_tables.pl YES
../../../src/backend/catalog/sql_feature_packages.txt
../../../src/backend/catalog/sql_features.txt > features-supported.sgml
'/usr/bin/perl' ./mk_feature_tables.pl NO
../../../src/backend/catalog/sql_feature_packages.txt
../../../src/backend/catalog/sql_features.txt > features-unsupported.sgml
'/usr/bin/perl' ./generate-errcodes-table.pl
../../../src/backend/utils/errcodes.txt > errcodes-table.sgml
onsgmls -wall -wno-unused-param -wno-empty -wfully-tagged -D . -D . -s
postgres.sgml
onsgmls:ref/vacuumdb.sgml:224:15:E: end tag for "LISTITEM" omitted, but
OMITTAG NO was specified
onsgmls:ref/vacuumdb.sgml:209:8: start tag was here
onsgmls:ref/vacuumdb.sgml:224:15:E: end tag for "VARLISTENTRY" omitted, but
OMITTAG NO was specified
onsgmls:ref/vacuumdb.sgml:206:5: start tag was here
onsgmls:ref/vacuumdb.sgml:224:15:E: end tag for "VARIABLELIST" omitted, but
OMITTAG NO was specified
onsgmls:ref/vacuumdb.sgml:79:4: start tag was here
onsgmls:ref/vacuumdb.sgml:225:18:E: end tag for element "LISTITEM" which is
not open
onsgmls:ref/vacuumdb.sgml:226:21:E: end tag for element "VARLISTENTRY"
which is not open
onsgmls:ref/vacuumdb.sgml:228:18:E: document type does not allow element
"VARLISTENTRY" here; assuming
missing "VARIABLELIST" start-tag
onsgmls:ref/vacuumdb.sgml:260:9:E: end tag for element "PARA" which is not
open
make: *** [check] Error 1

Fixed.

2.
Below multi-line comment is wrong:
/* Otherwise, we got a stage from vacuum_all_databases(), so run
* only that one. */

Fixed.

3.
fprintf(stderr, _("%s: vacuuming of database \"%s\" failed: %s"),
progname, dbname, PQerrorMessage(conn));
indentation of fprintf is not proper.

Fixed.

4.
/* This can only happen if user has sent the cacle request using
* Ctrl+C, Cancle is
handled by 0th slot, so fetch the error result
*/
spelling of cancel is wrong and multi-line comment is not proper.

Fixed

5.
/* This can only happen if user has sent the cacle request using
* Ctrl+C, Cancle is handled by 0th slot, so fetch the error result
*/

GetQueryResult(pSlot[0].connection, dbname, progname,
completedb);

indentation of completedb parameter is wrong.

Fixed.

6.
/*
* vacuum_parallel
* This function will open the multiple concurrent connections as
* suggested by used, it will derive the table list using server call
* if table list is not given by user and perform the vacuum call
*/

s/used/user

Fixed.

In general, I think you can once go through all the comments
and code to see if similar issues exist at other places as well.

I have done some performance test with the patch, data for which is
as below:
Performance Data
------------------------------
IBM POWER-7 16 cores, 64 hardware threads
RAM = 64GB
max_connections = 128
checkpoint_segments=256
checkpoint_timeout =15min
shared_buffers = 1GB

Before each test, run the testcase.sql file at below link:
http://www.postgresql.org/message-id/4205E661176A124FAF891E0A6BA9135266347F25@szxeml509-mbs.china.huawei.com

Un-patched -
time ./vacuumdb -t t1 -t t2 -t t3 -t t4 -t t5 -t t6 -t t7 -t t8 -d postgres
real 0m2.454s
user 0m0.002s
sys 0m0.006s

Patched -
time ./vacuumdb -t t1 -t t2 -t t3 -t t4 -t t5 -t t6 -t t7 -t t8 -j 4 -d
postgres

real 0m1.691s
user 0m0.001s
sys 0m0.004s

time ./vacuumdb -t t1 -t t2 -t t3 -t t4 -t t5 -t t6 -t t7 -t t8 -j 8 -d
postgres

real 0m1.496s
user 0m0.002s
sys 0m0.004s

Above data indicates that the patch improves performance when used
with more number of concurrent connections. I have done similar tests
for whole database as well and the results are quite similar to above.

I think you can once run the performance test for --analyze-in-stages
option as well after fixing the issue reported above in this mail.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com