pgsql: reindexdb: Add the index-level REINDEX with multiple jobs
reindexdb: Add the index-level REINDEX with multiple jobs
Straight-forward index-level REINDEX is not supported with multiple jobs as
we cannot control the concurrent processing of multiple indexes depending on
the same relation. Instead, we dedicate the whole table to certain reindex
job. Thus, if indexes in the lists belong to different tables, that gives us
a fair level of parallelism.
This commit teaches get_parallel_object_list() to fetch table names for
indexes in the case of index-level REINDEX. The same tables are grouped
together in the output order, and the list of indexes is also rebuilt to
match that order. Later during processingof that list, we push indexes
belonging to the same table into the same job.
Discussion: /messages/by-id/CACG=ezZU_VwDi-1PN8RUSE6mcYG+Yx1NH_rJO4+Ke-mKqLp=Nw@mail.gmail.com
Author: Maxim Orlov, Svetlana Derevyanko, Alexander Korotkov
Reviewed-by: Michael Paquier
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/47f99a407de224df6f9c43697d0a9c0a5598b250
Modified Files
--------------
doc/src/sgml/ref/reindexdb.sgml | 3 +-
src/bin/scripts/reindexdb.c | 146 +++++++++++++++++++++++++++++++------
src/bin/scripts/t/090_reindexdb.pl | 8 +-
3 files changed, 130 insertions(+), 27 deletions(-)
On Mon, 25 Mar 2024 at 13:07, Alexander Korotkov
<akorotkov@postgresql.org> wrote:
reindexdb: Add the index-level REINDEX with multiple jobs
This seems to cause a new compiler warning:
reindexdb.c: In function ‘reindex_one_database’:
reindexdb.c:437:45: warning: ‘indices_tables_cell’ may be used
uninitialized [-Wmaybe-uninitialized]
437 | indices_tables_cell = indices_tables_cell->next;
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
reindexdb.c:280:31: note: ‘indices_tables_cell’ was declared here
280 | SimpleStringListCell *indices_tables_cell;
| ^~~~~~~~~~~~~~~~~~~
which is causing problems in -Werror buildfarm members like mamba [1]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba&dt=2024-03-25%2001%3A03%3A35
David
[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba&dt=2024-03-25%2001%3A03%3A35
On 2024-Mar-25, Alexander Korotkov wrote:
reindexdb: Add the index-level REINDEX with multiple jobs
Straight-forward index-level REINDEX is not supported with multiple jobs as
we cannot control the concurrent processing of multiple indexes depending on
the same relation. Instead, we dedicate the whole table to certain reindex
job. Thus, if indexes in the lists belong to different tables, that gives us
a fair level of parallelism.
I tested this, because of a refactoring suggestion [1]/messages/by-id/CAEudQApP=u5-9PR_fs1DpZToQNrtTFSP+_fjrOgfi73UkrBXKQ@mail.gmail.com and I find that
it's rather completely broken. I ran this to setup a bunch of tables
that I'd want reindexed in parallel:
create table foo (a int);
insert into foo select * from generate_series(1, (10^7)::numeric);
create index foo1 on foo (a);
create index foo2 on foo (a);
create table bar (a int);
insert into bar select * from generate_series(1, (2 * (10^7))::numeric);
create index bar1 on bar (a);
create index bar2 on bar (a);
create table baz (a int);
insert into baz select * from generate_series(1, (5 * (10^6))::numeric);
create index baz1 on baz (a);
create index baz2 on baz (a);
create index baz3 on baz (a);
create index baz4 on baz (a);
I then run this:
reindexdb -j4 --echo -i foo1 -i foo2 -i bar1 -i bar2 -i baz1 -i baz2 -i baz3 -i baz4 | grep REINDEX
Looking at active processes with psql's \watch during the run, I learn
that what happens is that we process the indexes on baz first, without
any other process in parallel, until we get to the last one of baz
table, and we start processing one from baz and one from foo in
parallel. But when the one in baz is done, we only continue with one
process until the list reaches indexes of 'bar', and we process two in
parallel, and then we ran out of indexes in foo so we complete without
any more paralellism.
This is a waste and surely not what was intended: surely what we want is
that given that we have more parallel jobs available than there are
tables, we would start processing the first index of each table at
roughly the same time, namely right at program start. That would keep
three processes occupied until we ran out of indexes on one table, so
we'd keep two processes occupied, and so on. But this is not what
happens.
Am I misunderstanding something?
[1]: /messages/by-id/CAEudQApP=u5-9PR_fs1DpZToQNrtTFSP+_fjrOgfi73UkrBXKQ@mail.gmail.com
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On 2025-Mar-07, Álvaro Herrera wrote:
I tested this, because of a refactoring suggestion [1] and I find that
it's rather completely broken.
I think we need significantly more complex scheduling code if we want
this to actually work, possibly even having to hack the ParallelSlot
API some, so that we can inspect which tables have a running reindex and
know not to schedule the next one on it. What we're doing now makes no
sense.
We should strike this out from the list of features of 17 and revert
this commit.
If we want this feature in 19, we need another go through the drawing
board. (There's clearly not enough time to do it for 18.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On Sat, Mar 8, 2025 at 11:57 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2025-Mar-07, Álvaro Herrera wrote:
I tested this, because of a refactoring suggestion [1] and I find that
it's rather completely broken.I think we need significantly more complex scheduling code if we want
this to actually work, possibly even having to hack the ParallelSlot
API some, so that we can inspect which tables have a running reindex and
know not to schedule the next one on it. What we're doing now makes no
sense.We should strike this out from the list of features of 17 and revert
this commit.If we want this feature in 19, we need another go through the drawing
board. (There's clearly not enough time to do it for 18.)
Yes, I also think we need to revert this from 17. One thing to care
about: it might be already used in some user scripts. Should we
replace pg_fatal() with some notice and then run in a single job? So,
user scripts wouldn't error out.
------
Regards,
Alexander Korotkov
Supabase
On Fri, Mar 7, 2025 at 8:20 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2024-Mar-25, Alexander Korotkov wrote:
reindexdb: Add the index-level REINDEX with multiple jobs
Straight-forward index-level REINDEX is not supported with multiple jobs as
we cannot control the concurrent processing of multiple indexes depending on
the same relation. Instead, we dedicate the whole table to certain reindex
job. Thus, if indexes in the lists belong to different tables, that gives us
a fair level of parallelism.I tested this, because of a refactoring suggestion [1] and I find that
it's rather completely broken.
The code was written with assumption that running
run_reindex_command() with async == true can schedule a number of
queries for a connection. But actually that's not true and everything
is broken.
------
Regards,
Alexander Korotkov
Supabase
On Sat, Mar 8, 2025 at 12:49 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
On Fri, Mar 7, 2025 at 8:20 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2024-Mar-25, Alexander Korotkov wrote:
reindexdb: Add the index-level REINDEX with multiple jobs
Straight-forward index-level REINDEX is not supported with multiple jobs as
we cannot control the concurrent processing of multiple indexes depending on
the same relation. Instead, we dedicate the whole table to certain reindex
job. Thus, if indexes in the lists belong to different tables, that gives us
a fair level of parallelism.I tested this, because of a refactoring suggestion [1] and I find that
it's rather completely broken.The code was written with assumption that running
run_reindex_command() with async == true can schedule a number of
queries for a connection. But actually that's not true and everything
is broken.
The draft patch for revert is attached. Could you, please, check.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
v1-0001-revert-reindexdb-Add-the-index-level-REINDEX-with.patchapplication/octet-stream; name=v1-0001-revert-reindexdb-Add-the-index-level-REINDEX-with.patchDownload
From 09a261ef837420aaceef9d352a3626cf5b126797 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Sun, 9 Mar 2025 03:06:51 +0200
Subject: [PATCH v1] revert: reindexdb: Add the index-level REINDEX with
multiple jobs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This commit reverts 47f99a407d. The code was written assuming that running
run_reindex_command() with async == true can schedule a number of queries for
a connection. That's not true, and the second query sent using
run_reindex_command() will wait for the completion of the previous one.
That makes this feature almost useless. However, there is still an ability
to call index-level reindex in parallel mode. It would issue a warning and
fallback to serial mode, as it might already be in some user scripts.
Reported-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/202503071820.j25zn3lo4hvn%40alvherre.pgsql
---
doc/src/sgml/ref/reindexdb.sgml | 3 +-
src/bin/scripts/reindexdb.c | 162 +++++------------------------
src/bin/scripts/t/090_reindexdb.pl | 8 +-
3 files changed, 31 insertions(+), 142 deletions(-)
diff --git a/doc/src/sgml/ref/reindexdb.sgml b/doc/src/sgml/ref/reindexdb.sgml
index 98c3333228f..a877439dc5b 100644
--- a/doc/src/sgml/ref/reindexdb.sgml
+++ b/doc/src/sgml/ref/reindexdb.sgml
@@ -179,7 +179,8 @@ PostgreSQL documentation
setting is high enough to accommodate all connections.
</para>
<para>
- Note that this option is incompatible with the <option>--system</option> option.
+ Note that this option is incompatible with the <option>--index</option>
+ and <option>--system</option> options.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index b00c8112869..5ddd83f3828 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -205,8 +205,23 @@ main(int argc, char *argv[])
setup_cancel_handler(NULL);
- if (concurrentCons > 1 && syscatalog)
- pg_fatal("cannot use multiple jobs to reindex system catalogs");
+ if (concurrentCons > 1)
+ {
+ /*
+ * Index-level REINDEX is not supported with multiple jobs as we
+ * cannot control the concurrent processing of multiple indexes
+ * depending on the same relation.
+ */
+ if (indexes.head != NULL)
+ {
+ pg_log_warning("cannot use multiple jobs to reindex indexes, "
+ "fallback to single job");
+ concurrentCons = 1;
+ }
+
+ if (syscatalog)
+ pg_fatal("cannot use multiple jobs to reindex system catalogs");
+ }
if (alldb)
{
@@ -246,7 +261,7 @@ main(int argc, char *argv[])
if (indexes.head != NULL)
reindex_one_database(&cparams, REINDEX_INDEX, &indexes,
progname, echo, verbose,
- concurrently, concurrentCons, tablespace);
+ concurrently, 1, tablespace);
if (tables.head != NULL)
reindex_one_database(&cparams, REINDEX_TABLE, &tables,
@@ -276,16 +291,12 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
{
PGconn *conn;
SimpleStringListCell *cell;
- SimpleStringListCell *indices_tables_cell = NULL;
bool parallel = concurrentCons > 1;
SimpleStringList *process_list = user_list;
- SimpleStringList *indices_tables_list = NULL;
ReindexType process_type = type;
ParallelSlotArray *sa;
bool failed = false;
int items_count = 0;
- char *prev_index_table_name = NULL;
- ParallelSlot *free_slot = NULL;
conn = connectDatabase(cparams, progname, echo, false, true);
@@ -361,36 +372,8 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
}
break;
- case REINDEX_INDEX:
- Assert(user_list != NULL);
-
- /*
- * Build a list of relations from the indices. This will
- * accordingly reorder the list of indices too.
- */
- indices_tables_list = get_parallel_object_list(conn, process_type,
- user_list, echo);
-
- /*
- * Bail out if nothing to process. 'user_list' was modified
- * in-place, so check if it has at least one cell.
- */
- if (user_list->head == NULL)
- {
- PQfinish(conn);
- return;
- }
-
- /*
- * Assuming 'user_list' is not empty, 'indices_tables_list'
- * shouldn't be empty as well.
- */
- Assert(indices_tables_list != NULL);
- indices_tables_cell = indices_tables_list->head;
-
- break;
-
case REINDEX_SYSTEM:
+ case REINDEX_INDEX:
/* not supported */
Assert(false);
break;
@@ -431,7 +414,7 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
do
{
const char *objname = cell->val;
- bool need_new_slot = true;
+ ParallelSlot *free_slot = NULL;
if (CancelRequested)
{
@@ -439,33 +422,14 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
goto finish;
}
- /*
- * For parallel index-level REINDEX, the indices of the same table are
- * ordered together and they are to be processed by the same job. So,
- * we don't switch the job as soon as the index belongs to the same
- * table as the previous one.
- */
- if (parallel && process_type == REINDEX_INDEX)
+ free_slot = ParallelSlotsGetIdle(sa, NULL);
+ if (!free_slot)
{
- if (prev_index_table_name != NULL &&
- strcmp(prev_index_table_name, indices_tables_cell->val) == 0)
- need_new_slot = false;
- prev_index_table_name = indices_tables_cell->val;
- indices_tables_cell = indices_tables_cell->next;
- }
-
- if (need_new_slot)
- {
- free_slot = ParallelSlotsGetIdle(sa, NULL);
- if (!free_slot)
- {
- failed = true;
- goto finish;
- }
-
- ParallelSlotSetHandler(free_slot, TableCommandResultHandler, NULL);
+ failed = true;
+ goto finish;
}
+ ParallelSlotSetHandler(free_slot, TableCommandResultHandler, NULL);
run_reindex_command(free_slot->connection, process_type, objname,
echo, verbose, concurrently, true, tablespace);
@@ -482,12 +446,6 @@ finish:
pg_free(process_list);
}
- if (indices_tables_list)
- {
- simple_string_list_destroy(indices_tables_list);
- pg_free(indices_tables_list);
- }
-
ParallelSlotsTerminate(sa);
pfree(sa);
@@ -705,61 +663,8 @@ get_parallel_object_list(PGconn *conn, ReindexType type,
}
break;
- case REINDEX_INDEX:
- {
- SimpleStringListCell *cell;
-
- Assert(user_list != NULL);
-
- /*
- * Straight-forward index-level REINDEX is not supported with
- * multiple jobs as we cannot control the concurrent
- * processing of multiple indexes depending on the same
- * relation. But we can extract the appropriate table name
- * for the index and put REINDEX INDEX commands into different
- * jobs, according to the parent tables.
- *
- * We will order the results to group the same tables
- * together. We fetch index names as well to build a new list
- * of them with matching order.
- */
- appendPQExpBufferStr(&catalog_query,
- "SELECT t.relname, n.nspname, i.relname\n"
- "FROM pg_catalog.pg_index x\n"
- "JOIN pg_catalog.pg_class t ON t.oid = x.indrelid\n"
- "JOIN pg_catalog.pg_class i ON i.oid = x.indexrelid\n"
- "LEFT JOIN pg_catalog.pg_namespace n ON n.oid = t.relnamespace\n"
- "WHERE x.indexrelid OPERATOR(pg_catalog.=) ANY(ARRAY['");
-
- for (cell = user_list->head; cell; cell = cell->next)
- {
- if (cell != user_list->head)
- appendPQExpBufferStr(&catalog_query, "', '");
-
- appendQualifiedRelation(&catalog_query, cell->val, conn, echo);
- }
-
- /*
- * Order tables by the size of its greatest index. Within the
- * table, order indexes by their sizes.
- */
- appendPQExpBufferStr(&catalog_query,
- "']::pg_catalog.regclass[])\n"
- "ORDER BY max(i.relpages) OVER \n"
- " (PARTITION BY n.nspname, t.relname),\n"
- " n.nspname, t.relname, i.relpages;\n");
-
- /*
- * We're going to re-order the user_list to match the order of
- * tables. So, empty the user_list to fill it from the query
- * result.
- */
- simple_string_list_destroy(user_list);
- user_list->head = user_list->tail = NULL;
- }
- break;
-
case REINDEX_SYSTEM:
+ case REINDEX_INDEX:
case REINDEX_TABLE:
Assert(false);
break;
@@ -791,21 +696,6 @@ get_parallel_object_list(PGconn *conn, ReindexType type,
simple_string_list_append(tables, buf.data);
resetPQExpBuffer(&buf);
-
- if (type == REINDEX_INDEX)
- {
- /*
- * For index-level REINDEX, rebuild the list of indexes to match
- * the order of tables list.
- */
- appendPQExpBufferStr(&buf,
- fmtQualifiedIdEnc(PQgetvalue(res, i, 1),
- PQgetvalue(res, i, 2),
- PQclientEncoding(conn)));
-
- simple_string_list_append(user_list, buf.data);
- resetPQExpBuffer(&buf);
- }
}
termPQExpBuffer(&buf);
PQclear(res);
diff --git a/src/bin/scripts/t/090_reindexdb.pl b/src/bin/scripts/t/090_reindexdb.pl
index 378f8ad7a58..2c4b641c9b2 100644
--- a/src/bin/scripts/t/090_reindexdb.pl
+++ b/src/bin/scripts/t/090_reindexdb.pl
@@ -263,11 +263,9 @@ $node->safe_psql(
CREATE SCHEMA s1;
CREATE TABLE s1.t1(id integer);
CREATE INDEX ON s1.t1(id);
- CREATE INDEX i1 ON s1.t1(id);
CREATE SCHEMA s2;
CREATE TABLE s2.t2(id integer);
CREATE INDEX ON s2.t2(id);
- CREATE INDEX i2 ON s2.t2(id);
-- empty schema
CREATE SCHEMA s3;
|);
@@ -279,11 +277,11 @@ $node->command_ok(
[
'reindexdb',
'--jobs' => '2',
- '--index' => 's1.i1',
- '--index' => 's2.i2',
+ '--index' => 's1.t1_id_idx',
+ '--index' => 's2.t2_id_idx',
'postgres',
],
- 'parallel reindexdb for indices');
+ 'failover for parallel reindexdb for indices');
# Note that the ordering of the commands is not stable, so the second
# command for s2.t2 is not checked after.
$node->issues_sql_like(
--
2.39.5 (Apple Git-154)
On Sun, Mar 9, 2025 at 4:53 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
On Sat, Mar 8, 2025 at 12:49 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
On Fri, Mar 7, 2025 at 8:20 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2024-Mar-25, Alexander Korotkov wrote:
reindexdb: Add the index-level REINDEX with multiple jobs
Straight-forward index-level REINDEX is not supported with multiple jobs as
we cannot control the concurrent processing of multiple indexes depending on
the same relation. Instead, we dedicate the whole table to certain reindex
job. Thus, if indexes in the lists belong to different tables, that gives us
a fair level of parallelism.I tested this, because of a refactoring suggestion [1] and I find that
it's rather completely broken.The code was written with assumption that running
run_reindex_command() with async == true can schedule a number of
queries for a connection. But actually that's not true and everything
is broken.The draft patch for revert is attached. Could you, please, check.
After second thought it's not so hard to fix. The attached patch does
it by putting REINDEX commands related to the same table into a single
SQL statement. Possibly, that could be better than revert given we
need to handle compatibility. What do you think?
------
Regards,
Alexander Korotkov
Supabase
Attachments:
v1-0001-reindexdb-Fix-the-index-level-REINDEX-with-multip.patchapplication/octet-stream; name=v1-0001-reindexdb-Fix-the-index-level-REINDEX-with-multip.patchDownload
From ccb3ed7e3115ebceb09163344e2939cffb115137 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Sun, 9 Mar 2025 23:47:21 +0200
Subject: [PATCH v1] reindexdb: Fix the index-level REINDEX with multiple jobs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
47f99a407d introduced a parallel index-level REINDEX. The code was written
assuming that running run_reindex_command() with async == true can schedule
a number of queries for a connection. That's not true, and the second query
sent using run_reindex_command() will wait for the completion of the previous
one.
This commit fixes that by putting REINDEX commands for the same table into a
single query.
Reported-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/202503071820.j25zn3lo4hvn%40alvherre.pgsql
---
src/bin/scripts/reindexdb.c | 117 ++++++++++++++++++++----------------
1 file changed, 65 insertions(+), 52 deletions(-)
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index b00c8112869..d2778eb5735 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -49,10 +49,13 @@ static void reindex_all_databases(ConnParams *cparams,
bool syscatalog, SimpleStringList *schemas,
SimpleStringList *tables,
SimpleStringList *indexes);
-static void run_reindex_command(PGconn *conn, ReindexType type,
+static void gen_reindex_command(PGconn *conn, ReindexType type,
const char *name, bool echo, bool verbose,
- bool concurrently, bool async,
- const char *tablespace);
+ bool concurrently, const char *tablespace,
+ PQExpBufferData *sql);
+static void run_reindex_command(PGconn *conn, ReindexType type,
+ const char *name, bool echo, bool async,
+ PQExpBufferData *sq);
static void help(const char *progname);
@@ -284,7 +287,6 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
ParallelSlotArray *sa;
bool failed = false;
int items_count = 0;
- char *prev_index_table_name = NULL;
ParallelSlot *free_slot = NULL;
conn = connectDatabase(cparams, progname, echo, false, true);
@@ -430,8 +432,8 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
cell = process_list->head;
do
{
+ PQExpBufferData sql;
const char *objname = cell->val;
- bool need_new_slot = true;
if (CancelRequested)
{
@@ -439,35 +441,45 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
goto finish;
}
- /*
- * For parallel index-level REINDEX, the indices of the same table are
- * ordered together and they are to be processed by the same job. So,
- * we don't switch the job as soon as the index belongs to the same
- * table as the previous one.
- */
- if (parallel && process_type == REINDEX_INDEX)
+ free_slot = ParallelSlotsGetIdle(sa, NULL);
+ if (!free_slot)
{
- if (prev_index_table_name != NULL &&
- strcmp(prev_index_table_name, indices_tables_cell->val) == 0)
- need_new_slot = false;
- prev_index_table_name = indices_tables_cell->val;
- indices_tables_cell = indices_tables_cell->next;
+ failed = true;
+ goto finish;
}
- if (need_new_slot)
+ ParallelSlotSetHandler(free_slot, TableCommandResultHandler, NULL);
+ initPQExpBuffer(&sql);
+ if (parallel && process_type == REINDEX_INDEX)
{
- free_slot = ParallelSlotsGetIdle(sa, NULL);
- if (!free_slot)
+ /*
+ * For parallel index-level REINDEX, the indices of the same table
+ * are ordered together and they are to be processed by the same
+ * job. So, we put all the relevant REINDEX commands into the
+ * same SQL query to be processed by this job at once.
+ */
+ gen_reindex_command(free_slot->connection, process_type, objname,
+ echo, verbose, concurrently, tablespace, &sql);
+ while (indices_tables_cell->next &&
+ strcmp(indices_tables_cell->val, indices_tables_cell->next->val) == 0)
{
- failed = true;
- goto finish;
+ indices_tables_cell = indices_tables_cell->next;
+ cell = cell->next;
+ objname = cell->val;
+ appendPQExpBufferChar(&sql, '\n');
+ gen_reindex_command(free_slot->connection, process_type, objname,
+ echo, verbose, concurrently, tablespace, &sql);
}
-
- ParallelSlotSetHandler(free_slot, TableCommandResultHandler, NULL);
+ indices_tables_cell = indices_tables_cell->next;
+ }
+ 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, verbose, concurrently, true, tablespace);
+ echo, true, &sql);
+ termPQExpBuffer(&sql);
cell = cell->next;
} while (cell != NULL);
@@ -496,56 +508,52 @@ finish:
}
static void
-run_reindex_command(PGconn *conn, ReindexType type, const char *name,
- bool echo, bool verbose, bool concurrently, bool async,
- const char *tablespace)
+gen_reindex_command(PGconn *conn, ReindexType type, const char *name,
+ bool echo, bool verbose, bool concurrently,
+ const char *tablespace, PQExpBufferData *sql)
{
const char *paren = "(";
const char *comma = ", ";
const char *sep = paren;
- PQExpBufferData sql;
- bool status;
Assert(name);
/* build the REINDEX query */
- initPQExpBuffer(&sql);
-
- appendPQExpBufferStr(&sql, "REINDEX ");
+ appendPQExpBufferStr(sql, "REINDEX ");
if (verbose)
{
- appendPQExpBuffer(&sql, "%sVERBOSE", sep);
+ appendPQExpBuffer(sql, "%sVERBOSE", sep);
sep = comma;
}
if (tablespace)
{
- appendPQExpBuffer(&sql, "%sTABLESPACE %s", sep,
+ appendPQExpBuffer(sql, "%sTABLESPACE %s", sep,
fmtIdEnc(tablespace, PQclientEncoding(conn)));
sep = comma;
}
if (sep != paren)
- appendPQExpBufferStr(&sql, ") ");
+ appendPQExpBufferStr(sql, ") ");
/* object type */
switch (type)
{
case REINDEX_DATABASE:
- appendPQExpBufferStr(&sql, "DATABASE ");
+ appendPQExpBufferStr(sql, "DATABASE ");
break;
case REINDEX_INDEX:
- appendPQExpBufferStr(&sql, "INDEX ");
+ appendPQExpBufferStr(sql, "INDEX ");
break;
case REINDEX_SCHEMA:
- appendPQExpBufferStr(&sql, "SCHEMA ");
+ appendPQExpBufferStr(sql, "SCHEMA ");
break;
case REINDEX_SYSTEM:
- appendPQExpBufferStr(&sql, "SYSTEM ");
+ appendPQExpBufferStr(sql, "SYSTEM ");
break;
case REINDEX_TABLE:
- appendPQExpBufferStr(&sql, "TABLE ");
+ appendPQExpBufferStr(sql, "TABLE ");
break;
}
@@ -555,37 +563,44 @@ run_reindex_command(PGconn *conn, ReindexType type, const char *name,
* object type.
*/
if (concurrently)
- appendPQExpBufferStr(&sql, "CONCURRENTLY ");
+ appendPQExpBufferStr(sql, "CONCURRENTLY ");
/* object name */
switch (type)
{
case REINDEX_DATABASE:
case REINDEX_SYSTEM:
- appendPQExpBufferStr(&sql,
+ appendPQExpBufferStr(sql,
fmtIdEnc(name, PQclientEncoding(conn)));
break;
case REINDEX_INDEX:
case REINDEX_TABLE:
- appendQualifiedRelation(&sql, name, conn, echo);
+ appendQualifiedRelation(sql, name, conn, echo);
break;
case REINDEX_SCHEMA:
- appendPQExpBufferStr(&sql, name);
+ appendPQExpBufferStr(sql, name);
break;
}
/* finish the query */
- appendPQExpBufferChar(&sql, ';');
+ appendPQExpBufferChar(sql, ';');
+}
+
+static void
+run_reindex_command(PGconn *conn, ReindexType type, const char *name,
+ bool echo, bool async, PQExpBufferData *sql)
+{
+ bool status;
if (async)
{
if (echo)
- printf("%s\n", sql.data);
+ printf("%s\n", sql->data);
- status = PQsendQuery(conn, sql.data) == 1;
+ status = PQsendQuery(conn, sql->data) == 1;
}
else
- status = executeMaintenanceCommand(conn, sql.data, echo);
+ status = executeMaintenanceCommand(conn, sql->data, echo);
if (!status)
{
@@ -618,8 +633,6 @@ run_reindex_command(PGconn *conn, ReindexType type, const char *name,
exit(1);
}
}
-
- termPQExpBuffer(&sql);
}
/*
--
2.39.5 (Apple Git-154)
On 2025-Mar-09, Alexander Korotkov wrote:
After second thought it's not so hard to fix. The attached patch does
it by putting REINDEX commands related to the same table into a single
SQL statement. Possibly, that could be better than revert given we
need to handle compatibility. What do you think?
Oh, this is an egg of Columbus solution, I like it. It seems to work as
intended on a quick test. Please add some commentary to run_reindex_command.
Maybe another, possibly better way to do this would be to use libpq
pipeline mode, sending all the commands for a table in one pipeline
instead of a single command. The advantage of this would be that
server-side log entries for each command would be separate, and they
wouldn't appear clumped together in pg_stat_activity and so on.
However, this would require more invasive changes, so it might be better
to leave that for a future project -- it's certainly a harder sell for
such a change to be backpatched. So I'm +1 on your current patch for 17
and master.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"
On Mon, Mar 10, 2025 at 1:12 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2025-Mar-09, Alexander Korotkov wrote:
After second thought it's not so hard to fix. The attached patch does
it by putting REINDEX commands related to the same table into a single
SQL statement. Possibly, that could be better than revert given we
need to handle compatibility. What do you think?Oh, this is an egg of Columbus solution, I like it. It seems to work as
intended on a quick test. Please add some commentary to run_reindex_command.Maybe another, possibly better way to do this would be to use libpq
pipeline mode, sending all the commands for a table in one pipeline
instead of a single command. The advantage of this would be that
server-side log entries for each command would be separate, and they
wouldn't appear clumped together in pg_stat_activity and so on.
However, this would require more invasive changes, so it might be better
to leave that for a future project -- it's certainly a harder sell for
such a change to be backpatched. So I'm +1 on your current patch for 17
and master.
Thank you for your feedback! I also think that pipelining would be a
better options, but it's too invasive for backpatching. I've written
comments for gen_reindex_command() and run_reindex_command(). I'm
going to push this patch to master and 17 if no objections.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
v2-0001-reindexdb-Fix-the-index-level-REINDEX-with-multip.patchapplication/octet-stream; name=v2-0001-reindexdb-Fix-the-index-level-REINDEX-with-multip.patchDownload
From 8c3188e9713cf0401a14f64fb45389293bbbbd42 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Tue, 11 Mar 2025 23:31:14 +0200
Subject: [PATCH v2] reindexdb: Fix the index-level REINDEX with multiple jobs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
47f99a407d introduced a parallel index-level REINDEX. The code was written
assuming that running run_reindex_command() with async == true can schedule
a number of queries for a connection. That's not true, and the second query
sent using run_reindex_command() will wait for the completion of the previous
one.
This commit fixes that by putting REINDEX commands for the same table into a
single query.
Reported-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/202503071820.j25zn3lo4hvn%40alvherre.pgsql
Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Backpatch-through: 17
---
src/bin/scripts/reindexdb.c | 126 +++++++++++++++++++++---------------
1 file changed, 74 insertions(+), 52 deletions(-)
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index b00c8112869..355f2a52391 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -49,10 +49,13 @@ static void reindex_all_databases(ConnParams *cparams,
bool syscatalog, SimpleStringList *schemas,
SimpleStringList *tables,
SimpleStringList *indexes);
-static void run_reindex_command(PGconn *conn, ReindexType type,
+static void gen_reindex_command(PGconn *conn, ReindexType type,
const char *name, bool echo, bool verbose,
- bool concurrently, bool async,
- const char *tablespace);
+ bool concurrently, const char *tablespace,
+ PQExpBufferData *sql);
+static void run_reindex_command(PGconn *conn, ReindexType type,
+ const char *name, bool echo, bool async,
+ PQExpBufferData *sq);
static void help(const char *progname);
@@ -284,7 +287,6 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
ParallelSlotArray *sa;
bool failed = false;
int items_count = 0;
- char *prev_index_table_name = NULL;
ParallelSlot *free_slot = NULL;
conn = connectDatabase(cparams, progname, echo, false, true);
@@ -430,8 +432,8 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
cell = process_list->head;
do
{
+ PQExpBufferData sql;
const char *objname = cell->val;
- bool need_new_slot = true;
if (CancelRequested)
{
@@ -439,35 +441,45 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
goto finish;
}
- /*
- * For parallel index-level REINDEX, the indices of the same table are
- * ordered together and they are to be processed by the same job. So,
- * we don't switch the job as soon as the index belongs to the same
- * table as the previous one.
- */
- if (parallel && process_type == REINDEX_INDEX)
+ free_slot = ParallelSlotsGetIdle(sa, NULL);
+ if (!free_slot)
{
- if (prev_index_table_name != NULL &&
- strcmp(prev_index_table_name, indices_tables_cell->val) == 0)
- need_new_slot = false;
- prev_index_table_name = indices_tables_cell->val;
- indices_tables_cell = indices_tables_cell->next;
+ failed = true;
+ goto finish;
}
- if (need_new_slot)
+ ParallelSlotSetHandler(free_slot, TableCommandResultHandler, NULL);
+ initPQExpBuffer(&sql);
+ if (parallel && process_type == REINDEX_INDEX)
{
- free_slot = ParallelSlotsGetIdle(sa, NULL);
- if (!free_slot)
+ /*
+ * For parallel index-level REINDEX, the indices of the same table
+ * are ordered together and they are to be processed by the same
+ * job. So, we put all the relevant REINDEX commands into the
+ * same SQL query to be processed by this job at once.
+ */
+ gen_reindex_command(free_slot->connection, process_type, objname,
+ echo, verbose, concurrently, tablespace, &sql);
+ while (indices_tables_cell->next &&
+ strcmp(indices_tables_cell->val, indices_tables_cell->next->val) == 0)
{
- failed = true;
- goto finish;
+ indices_tables_cell = indices_tables_cell->next;
+ cell = cell->next;
+ objname = cell->val;
+ appendPQExpBufferChar(&sql, '\n');
+ gen_reindex_command(free_slot->connection, process_type, objname,
+ echo, verbose, concurrently, tablespace, &sql);
}
-
- ParallelSlotSetHandler(free_slot, TableCommandResultHandler, NULL);
+ indices_tables_cell = indices_tables_cell->next;
+ }
+ 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, verbose, concurrently, true, tablespace);
+ echo, true, &sql);
+ termPQExpBuffer(&sql);
cell = cell->next;
} while (cell != NULL);
@@ -495,57 +507,57 @@ finish:
exit(1);
}
+/*
+ * Append a SQL command required to reindex a given database object to the
+ * '*sql' string.
+ */
static void
-run_reindex_command(PGconn *conn, ReindexType type, const char *name,
- bool echo, bool verbose, bool concurrently, bool async,
- const char *tablespace)
+gen_reindex_command(PGconn *conn, ReindexType type, const char *name,
+ bool echo, bool verbose, bool concurrently,
+ const char *tablespace, PQExpBufferData *sql)
{
const char *paren = "(";
const char *comma = ", ";
const char *sep = paren;
- PQExpBufferData sql;
- bool status;
Assert(name);
/* build the REINDEX query */
- initPQExpBuffer(&sql);
-
- appendPQExpBufferStr(&sql, "REINDEX ");
+ appendPQExpBufferStr(sql, "REINDEX ");
if (verbose)
{
- appendPQExpBuffer(&sql, "%sVERBOSE", sep);
+ appendPQExpBuffer(sql, "%sVERBOSE", sep);
sep = comma;
}
if (tablespace)
{
- appendPQExpBuffer(&sql, "%sTABLESPACE %s", sep,
+ appendPQExpBuffer(sql, "%sTABLESPACE %s", sep,
fmtIdEnc(tablespace, PQclientEncoding(conn)));
sep = comma;
}
if (sep != paren)
- appendPQExpBufferStr(&sql, ") ");
+ appendPQExpBufferStr(sql, ") ");
/* object type */
switch (type)
{
case REINDEX_DATABASE:
- appendPQExpBufferStr(&sql, "DATABASE ");
+ appendPQExpBufferStr(sql, "DATABASE ");
break;
case REINDEX_INDEX:
- appendPQExpBufferStr(&sql, "INDEX ");
+ appendPQExpBufferStr(sql, "INDEX ");
break;
case REINDEX_SCHEMA:
- appendPQExpBufferStr(&sql, "SCHEMA ");
+ appendPQExpBufferStr(sql, "SCHEMA ");
break;
case REINDEX_SYSTEM:
- appendPQExpBufferStr(&sql, "SYSTEM ");
+ appendPQExpBufferStr(sql, "SYSTEM ");
break;
case REINDEX_TABLE:
- appendPQExpBufferStr(&sql, "TABLE ");
+ appendPQExpBufferStr(sql, "TABLE ");
break;
}
@@ -555,37 +567,49 @@ run_reindex_command(PGconn *conn, ReindexType type, const char *name,
* object type.
*/
if (concurrently)
- appendPQExpBufferStr(&sql, "CONCURRENTLY ");
+ appendPQExpBufferStr(sql, "CONCURRENTLY ");
/* object name */
switch (type)
{
case REINDEX_DATABASE:
case REINDEX_SYSTEM:
- appendPQExpBufferStr(&sql,
+ appendPQExpBufferStr(sql,
fmtIdEnc(name, PQclientEncoding(conn)));
break;
case REINDEX_INDEX:
case REINDEX_TABLE:
- appendQualifiedRelation(&sql, name, conn, echo);
+ appendQualifiedRelation(sql, name, conn, echo);
break;
case REINDEX_SCHEMA:
- appendPQExpBufferStr(&sql, name);
+ appendPQExpBufferStr(sql, name);
break;
}
/* finish the query */
- appendPQExpBufferChar(&sql, ';');
+ appendPQExpBufferChar(sql, ';');
+}
+
+/*
+ * Run one or more reindex commands accumulated in the '*sql' string against
+ * a given database connection. Note that the 'async' parameter skips the
+ * PQfinish() but does not support pipelining.
+ */
+static void
+run_reindex_command(PGconn *conn, ReindexType type, const char *name,
+ bool echo, bool async, PQExpBufferData *sql)
+{
+ bool status;
if (async)
{
if (echo)
- printf("%s\n", sql.data);
+ printf("%s\n", sql->data);
- status = PQsendQuery(conn, sql.data) == 1;
+ status = PQsendQuery(conn, sql->data) == 1;
}
else
- status = executeMaintenanceCommand(conn, sql.data, echo);
+ status = executeMaintenanceCommand(conn, sql->data, echo);
if (!status)
{
@@ -618,8 +642,6 @@ run_reindex_command(PGconn *conn, ReindexType type, const char *name,
exit(1);
}
}
-
- termPQExpBuffer(&sql);
}
/*
--
2.39.5 (Apple Git-154)
On 2025-Mar-11, Alexander Korotkov wrote:
Thank you for your feedback! I also think that pipelining would be a
better options, but it's too invasive for backpatching. I've written
comments for gen_reindex_command() and run_reindex_command(). I'm
going to push this patch to master and 17 if no objections.
Looks good.
Actually, I don't understand why is run_reindex_command() calling
PQfinish(). Hmm, also, why is the 'async' parameter always given as
true? Is that PQfinish() actually dead code?
[... Looks at git history ...]
Ah! this is actually a problem older than your patch -- it appears that
the last (only?) caller with async=false was removed by commit
2cbc3c17a5c1. I think it's worth removing that while you're at it. We
no longer need executeMaintenanceCommand() in reindexdb.c anymore
either, I think.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/