From: Manu <manuelreyesbravo@gmail.com>
To: Kirill Reshke <reshkekirill@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Fix reindexdb with parallel index-level conrurrent run
Date: Fri, 02 Oct 2026 22:48:37 -0300
Message-ID: <179099211731.145666.6625256734261949794@gmail.com> (raw)
In-Reply-To: <CALdSSPiEkAmS+n7Ea9XDMrLjkbQk=CrB7zw1kcq9B47hfKUhXw@mail.gmail.com>
References: <CALdSSPiEkAmS+n7Ea9XDMrLjkbQk=CrB7zw1kcq9B47hfKUhXw@mail.gmail.com>
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
Attachments:
[text/x-patch] v2-0001-Fix-reindexdb-jobs-concurrently-with-indexes-of-t.patch (6.1K, ../179099211731.145666.6625256734261949794@gmail.com/2-v2-0001-Fix-reindexdb-jobs-concurrently-with-indexes-of-t.patch)
download | inline diff:
From c52841347f12acc351a2b896b1b91054e7edae90 Mon Sep 17 00:00:00 2001
From: Kirill Reshke <reshkekirill@gmail.com>
Date: Fri, 2 Oct 2026 22:45:09 -0300
Subject: [PATCH v2 1/2] Fix reindexdb --jobs --concurrently with indexes of
the same table
For parallel index-level REINDEX, reindexdb puts the commands for all
the indexes of one table into a single query. A multi-statement
simple query runs as an implicit transaction block, where REINDEX
CONCURRENTLY is not allowed, so the run failed. With --concurrently,
send each command separately instead.
Oversight in 47f99a407d.
---
src/bin/scripts/reindexdb.c | 43 ++++++++++++++++++++++++++--
src/bin/scripts/t/090_reindexdb.pl | 24 ++++++++++++++++
src/fe_utils/parallel_slot.c | 2 +-
src/include/fe_utils/parallel_slot.h | 9 ++++++
4 files changed, 74 insertions(+), 4 deletions(-)
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.cindex d7fb16d3c85..d7784de3f55 100644--- a/src/bin/scripts/reindexdb.c+++ b/src/bin/scripts/reindexdb.c@@ -434,7 +434,7 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
ParallelSlotSetHandler(free_slot, TableCommandResultHandler, NULL);
initPQExpBuffer(&sql);
- if (parallel && process_type == REINDEX_INDEX)+ if (parallel && process_type == REINDEX_INDEX && !concurrently)
{
/*
* For parallel index-level REINDEX, the indices of the same table
@@ -455,14 +455,51 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
echo, verbose, concurrently, tablespace, &sql);
}
indices_tables_cell = indices_tables_cell->next;
+ run_reindex_command(free_slot->connection, process_type, objname,+ echo, &sql);+ }+ else if (parallel && process_type == REINDEX_INDEX)+ {+ /*+ * REINDEX CONCURRENTLY cannot run in a transaction block, so it+ * cannot be part of a multi-statement simple query, which would+ * be an implicit transaction block. The indices of the same+ * table still have to be processed by the same job, to avoid+ * concurrent REINDEX CONCURRENTLY commands on the same table,+ * which could deadlock. So, each command is sent separately,+ * waiting for it to complete before sending the next one.+ */+ gen_reindex_command(free_slot->connection, process_type, objname,+ echo, verbose, concurrently, tablespace, &sql);+ run_reindex_command(free_slot->connection, process_type, objname,+ echo, &sql);+ if (!consumeQueryResult(free_slot))+ failed = true;+ while (indices_tables_cell->next &&+ indices_tables_cell->val == indices_tables_cell->next->val)+ {+ indices_tables_cell = indices_tables_cell->next;+ cell = cell->next;+ objname = cell->val;+ termPQExpBuffer(&sql);+ initPQExpBuffer(&sql);+ gen_reindex_command(free_slot->connection, process_type, objname,+ echo, verbose, concurrently, tablespace, &sql);+ run_reindex_command(free_slot->connection, process_type, objname,+ echo, &sql);+ if (!consumeQueryResult(free_slot))+ failed = true;+ }+ indices_tables_cell = indices_tables_cell->next;+ ParallelSlotSetIdle(free_slot);
}
else
{
gen_reindex_command(free_slot->connection, process_type, objname,
echo, verbose, concurrently, tablespace, &sql);
+ run_reindex_command(free_slot->connection, process_type, objname,+ echo, &sql);
}
- run_reindex_command(free_slot->connection, process_type, objname,- echo, &sql);
termPQExpBuffer(&sql);
cell = cell->next;
diff --git a/src/bin/scripts/t/090_reindexdb.pl b/src/bin/scripts/t/090_reindexdb.plindex ae7d3724464..1013120f312 100644--- a/src/bin/scripts/t/090_reindexdb.pl+++ b/src/bin/scripts/t/090_reindexdb.pl@@ -168,6 +168,30 @@ $node->issues_sql_like(
[ 'reindexdb', '--concurrently', '--index' => 'test1x', 'postgres' ],
qr/statement: REINDEX INDEX CONCURRENTLY public\.test1x;/,
'reindex specific index concurrently');
++# Multiple indexes of the same table with parallel jobs must not be+# batched into a single multi-statement query when using --concurrently,+# as REINDEX CONCURRENTLY cannot run inside a transaction block.+$node->safe_psql('postgres',+ 'CREATE TABLE test2 (a int); CREATE INDEX test2x ON test2 (a);'+ . ' CREATE INDEX test2y ON test2 (a);');+$node->command_ok(+ [+ 'reindexdb', '--jobs' => '2', '--concurrently',+ '--index' => 'test2x',+ '--index' => 'test2y',+ 'postgres',+ ],+ 'reindex two indexes of the same table concurrently with two jobs');+$node->command_ok(+ [+ 'reindexdb', '--jobs' => '2',+ '--index' => 'test2x',+ '--index' => 'test2y',+ 'postgres',+ ],+ 'reindex two indexes of the same table with two jobs');+
$node->issues_sql_like(
[ 'reindexdb', '--concurrently', '--schema' => 'public', 'postgres' ],
qr/statement: REINDEX SCHEMA CONCURRENTLY public;/,
diff --git a/src/fe_utils/parallel_slot.c b/src/fe_utils/parallel_slot.cindex fb9e6cc4ec1..831e0c27878 100644--- a/src/fe_utils/parallel_slot.c+++ b/src/fe_utils/parallel_slot.c@@ -54,7 +54,7 @@ processQueryResult(ParallelSlot *slot, PGresult *result)
* nothing remains. If at least one error is encountered, return false.
* Note that this will block if the connection is busy.
*/
-static bool+bool
consumeQueryResult(ParallelSlot *slot)
{
bool ok = true;
diff --git a/src/include/fe_utils/parallel_slot.h b/src/include/fe_utils/parallel_slot.hindex a6ebe273ce0..6d5972614bf 100644--- a/src/include/fe_utils/parallel_slot.h+++ b/src/include/fe_utils/parallel_slot.h@@ -78,6 +78,15 @@ extern void ParallelSlotsTerminate(ParallelSlotArray *sa);
extern bool ParallelSlotsWaitCompletion(ParallelSlotArray *sa);
+/*+ * Wait for the results of the query currently being processed by the given+ * slot, and process them with the handler set for this slot. Returns+ * false if the query failed, true otherwise. The slot is not marked as+ * idle by this function. Note that this will block if the connection is+ * busy.+ */+extern bool consumeQueryResult(ParallelSlot *slot);+
extern bool TableCommandResultHandler(PGresult *res, PGconn *conn,
void *context);
--
2.55.0
[text/x-patch] v2-0002-reindexdb-keep-jobs-parallel-and-stop-on-cancel-w.patch (3.3K, ../179099211731.145666.6625256734261949794@gmail.com/3-v2-0002-reindexdb-keep-jobs-parallel-and-stop-on-cancel-w.patch)
download | inline diff:
From 7420f3194939e08219634ff079a72671766698ad Mon Sep 17 00:00:00 2001
From: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Date: Fri, 2 Oct 2026 22:45:09 -0300
Subject: [PATCH v2 2/2] reindexdb: keep --jobs parallel and stop on cancel
with --concurrently
Only wait for each command when the next index belongs to the same
table. An index that is the only one of its table goes through the
usual asynchronous path, so indexes of different tables are again
processed in parallel.
Also stop at the first failure or cancel request, as the other paths
do, instead of going on with the remaining indexes of the table.
---
src/bin/scripts/reindexdb.c | 37 ++++++++++++++++++++++---------------
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.cindex d7784de3f55..4002616cd8f 100644--- a/src/bin/scripts/reindexdb.c+++ b/src/bin/scripts/reindexdb.c@@ -458,7 +458,9 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
run_reindex_command(free_slot->connection, process_type, objname,
echo, &sql);
}
- else if (parallel && process_type == REINDEX_INDEX)+ else if (parallel && process_type == REINDEX_INDEX &&+ indices_tables_cell->next &&+ indices_tables_cell->val == indices_tables_cell->next->val)
{
/*
* REINDEX CONCURRENTLY cannot run in a transaction block, so it
@@ -469,32 +471,37 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
* which could deadlock. So, each command is sent separately,
* waiting for it to complete before sending the next one.
*/
- gen_reindex_command(free_slot->connection, process_type, objname,- echo, verbose, concurrently, tablespace, &sql);- run_reindex_command(free_slot->connection, process_type, objname,- echo, &sql);- if (!consumeQueryResult(free_slot))- failed = true;- while (indices_tables_cell->next &&- indices_tables_cell->val == indices_tables_cell->next->val)+ for (;;)
{
- indices_tables_cell = indices_tables_cell->next;- cell = cell->next;- objname = cell->val;- termPQExpBuffer(&sql);- initPQExpBuffer(&sql);
gen_reindex_command(free_slot->connection, process_type, objname,
echo, verbose, concurrently, tablespace, &sql);
run_reindex_command(free_slot->connection, process_type, objname,
echo, &sql);
- if (!consumeQueryResult(free_slot))++ /* Stop at the first failure or cancel, as the other paths do. */+ if (!consumeQueryResult(free_slot) || CancelRequested)+ {+ termPQExpBuffer(&sql);
failed = true;
+ goto finish;+ }++ if (!(indices_tables_cell->next &&+ indices_tables_cell->val == indices_tables_cell->next->val))+ break;+ indices_tables_cell = indices_tables_cell->next;+ cell = cell->next;+ objname = cell->val;+ resetPQExpBuffer(&sql);
}
indices_tables_cell = indices_tables_cell->next;
ParallelSlotSetIdle(free_slot);
}
else
{
+ /* The only index of its table: nothing to keep in order. */+ if (parallel && process_type == REINDEX_INDEX)+ indices_tables_cell = indices_tables_cell->next;
gen_reindex_command(free_slot->connection, process_type, objname,
echo, verbose, concurrently, tablespace, &sql);
run_reindex_command(free_slot->connection, process_type, objname,
--
2.55.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: manuelreyesbravo@gmail.com, reshkekirill@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Fix reindexdb with parallel index-level conrurrent run
In-Reply-To: <179099211731.145666.6625256734261949794@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox