Subject: pgsql: reindexdb: Add the index-level REINDEX with multiple jobs

From: Alexander Korotkov <[email-masked]>
Date: Mon, 25 Mar 2024 00:07:24 +0000

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: https://postgr.es/m/CACG%3DezZU_VwDi-1PN8RUSE6mcYG%2BYx1NH_rJO4%2BKe-mKqLp%3DNw%40mail.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(-)

---

From: David Rowley <[email-masked]>
Date: Mon, 25 Mar 2024 14:58:15 +1300

> 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]

David

 [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba&dt=2024-03-25%2001%3A03%3A35

---

From: Álvaro Herrera <[email-masked]>
Date: Fri, 7 Mar 2025 19:20:34 +0100

> a fair level of parallelism.

I tested this, because of a refactoring suggestion [1] 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] https://postgr.es/m/CAEudQApP=[email-masked]
 
-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

---

From: Álvaro Herrera <[email-masked]>
Date: Sat, 8 Mar 2025 10:56:55 +0100

On 2025-Mar-07, Álvaro Herrera wrote:

> 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/

---

From: Alexander Korotkov <[email-masked]>
Date: Sat, 8 Mar 2025 12:42:41 +0200

> 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

---

From: Alexander Korotkov <[email-masked]>
Date: Sat, 8 Mar 2025 12:49:35 +0200

> 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

---

From: Alexander Korotkov <[email-masked]>
Date: Sun, 9 Mar 2025 04:53:29 +0200

> queries for a connection.  But actually that's not true and everything

The draft patch for revert is attached.  Could you, please, check.

------
Regards,
Alexander Korotkov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Sun, 9 Mar 2025 23:53:00 +0200

> 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

---

From: Álvaro Herrera <[email-masked]>
Date: Mon, 10 Mar 2025 12:12:33 +0100

> 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"

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 11 Mar 2025 23:33:25 +0200

> such a change to be backpatched.  So I'm +1 on your current patch for 17

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

---

From: Álvaro Herrera <[email-masked]>
Date: Wed, 12 Mar 2025 10:41:43 +0100

> 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.