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

On Tue, Aug 19, 2014 at 4:27 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> Few more comments:
>
Some more comments:

1. I could see one shortcoming in the way the patch has currently
parallelize the
work for --analyze-in-stages. Basically patch is performing the work for
each stage
for multiple tables in concurrent connections that seems okay for the
cases when
number of parallel connections is less than equal to number of tables,
but for
the case when user has asked for more number of connections than number
of tables,
then I think this strategy will not be able to use the extra
connections.

2. Similarly for the case of multiple databases, currently it will not be
able
to use connections more than number of tables in each database because
the
parallelizing strategy is to just use the conncurrent connections for
tables inside single database.

I am not completely sure whether current strategy is good enough or
we should try to address the above problems. What do you think?

3.
+ do
+ {
+ i = select_loop(maxFd, &slotset);
+ Assert(i != 0);

Could you explain the reason of using this loop, I think you
want to wait for data on socket descriptor, but why for maxFd?
Also it is better if you explain this logic in comments.

4.
+ for (i = 0; i < max_slot; i++)
+ {
+ if (!FD_ISSET(pSlot[i].sock, &slotset))
+ continue;
+
+ PQconsumeInput(pSlot[i].connection);
+ if (PQisBusy(pSlot[i].connection))
+ continue;

I think it is better to call PQconsumeInput() only if you find
connection is busy.

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