On Thu, Jul 25, 2019 at 12:12:40PM +0300, Sergei Kornilov wrote:
> Thank you, v9 code and behavior looks good for me. Builds cleanly,
> works with older releases. I'll mark Ready to Commiter.
The restriction with --jobs and --system is not documented, and that's
good to have from the start. This actually makes the parallel job
handling with --index inconsistent as we mask parallelism in this case
by enforcing the use of one connection. I think that we should
revisit the interactions with --index and --jobs actually, because,
assuming that users never read the documentation, they may actually be
surprised to see that something like --index idx1 .. --index idxN
--jobs=N does not lead to any improvements at all, until they find out
the reason why. It is also much easier to have an error as starting
point because it can be lifted later one. There is an argument that
we may actually not have this restriction at all on --index as long as
the user knows what it is doing and does not define indexes from the
same relation, still I would keep an error.
Thinking deeper about it, there is also a point of gathering first all
the relations if one associates --schemas and --tables in the same
call of reindexdb and then pass down a list of decomposed relations
which are processed in parallel. The code as currently presented is
rather straight-forward, and I don't think that this is worth the
extra complication, but this was not mentioned until now on this
thread :)
For the non-parallel case in reindex_one_database(), I would add an
Assert on REINDEX_DATABASE and REINDEX_SYSTEM with a comment to
mention that a NULL list of objects can just be passed down only in
those two cases when the single-object list with the database name is
built.
>> I don't have a strong opinion here. If we change for >=, it'd be
>> better to also adapt vacuumdb for consistency. I didn't change it for
>> now, to stay consistent with vacuumdb.
>
> Yep, no strong opinion from me too.
My opinion tends towards consistency. Consistency sounds good.
--
Michael
