Re: Fix reindexdb with parallel index-level conrurrent run

Hi Kirill,

> reindexdb: error: processing of database "reshke" failed: ERROR:
> REINDEX CONCURRENTLY cannot run inside a transaction block

Reproduced on master, REL_18 and REL_17. With the patch, every
--concurrently --jobs case I tried rebuilds all the requested
indexes, and the new test in 090_reindexdb.pl fails without the
reindexdb.c change and passes with it.

I measured it as well, and a few things came up, all in the new
branch.

1. --jobs no longer runs in parallel. The branch waits for every
command with consumeQueryResult(), even for an index that is the only
one of its table, and the main loop hands out nothing while it waits.
Four tables with one index each, 3M rows per table, median of 6
rounds, same server for all clients:

reindexdb --concurrently -j 4 -i t1_b -i t2_b -i t3_b -i t4_b

master: 6.3 s
v1: 22.5 s
v2 (attached): 6.6 s
(-j 1: 22.5 s)

2. A cancel only stops the current index. The inner loop never looks
at CancelRequested, so after Ctrl-C reindexdb goes on with the rest
of the table's indexes. With one table of three 2M-row indexes and
SIGINT after 3 s, -j 1 exits at once with nothing more rebuilt, while
v1 with -j 2 exits 9 s later, having rebuilt the other two. For the
same reason a failed REINDEX does not stop the run, while elsewhere
reindexdb stops at the first failure (with -j 1, and with -j 2
without --concurrently).

3. Back-patching. ParallelSlotSetIdle() only exists from REL_19 on
(750816971b3), so the patch does not compile on REL_18 or REL_17;
"free_slot->inUse = false" works there. On REL_17 there is a quieter
trap: indices_tables_list is a SimpleStringList, and the existing
branch compares it with strcmp(). The new "==" compiles, but it
compares pointers and never matches. With v1 alone that goes
unnoticed, because everything runs one at a time anyway, but once a
single-index table may run asynchronously, as with 0002, the indexes
of one table end up in different jobs: REL_17 then fails with
"deadlock detected" in 5 runs out of 5, and passes 5 out of 5 with
strcmp().

Attached is v2 as two patches:

0001 is your patch, unchanged. It had no commit message, so I wrote
one from your mail; please change it as you like.

0002 addresses 1 and 2. It takes the waiting path only when the next
index belongs to the same table, so a single-index table goes through
the normal asynchronous path, as on master. It also stops at the
first failure or cancel. 090 passes, and the cancel case exits at
once with nothing more rebuilt.

With the two changes from point 3, 0001+0002 and 090 pass on REL_18
and REL_17 (on 17 the test hunk needs the short -i option). I can
post versions for those branches if that helps.

What v2 does not solve: the main loop still waits while a multi-index
table is processed. Tables are ordered by the size of their largest
index, ascending, so a small multi-index table is handed out first
and delays the larger ones. One table with two 1.5M-row indexes plus
three tables with one 3M-row index, -j 4:

v1: 23.6 s
v2: 12.0 s
the two-index table alone: 5.1 s
the other three alone, -j 3: 6.5 s

Fixing that would need the slot to send the table's next command
itself, which is a larger change than I would put in a back-patched
fix.

Regards,
Manu