Horton

Thread

Commits

  1. reindexdb: Fix the index-level REINDEX with multiple jobs

  2. reindexdb: Fix warning about uninitialized indices_tables_cell

  3. reindexdb: Add the index-level REINDEX with multiple jobs

  4. Rework logic and simplify syntax of REINDEX DATABASE/SYSTEM

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

    Alexander Korotkov <akorotkov@postgresql.org> — 2024-03-25T00:07:24Z

    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(-)

  2. Re: pgsql: reindexdb: Add the index-level REINDEX with multiple jobs

    David Rowley <dgrowleyml@gmail.com> — 2024-03-25T01:58:15Z

    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]

David

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

  3. Re: pgsql: reindexdb: Add the index-level REINDEX with multiple jobs

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-07T18:20:34Z

    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.  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=u5-9PR_fs1DpZToQNrtTFSP+_fjrOgfi73UkrBXKQ@mail.gmail.com
 
-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  4. Re: pgsql: reindexdb: Add the index-level REINDEX with multiple jobs

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-08T09:56:55Z

    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/

  5. Re: pgsql: reindexdb: Add the index-level REINDEX with multiple jobs

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-03-08T10:42:41Z

    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

  6. Re: pgsql: reindexdb: Add the index-level REINDEX with multiple jobs

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-03-08T10:49:35Z

    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

  7. Re: pgsql: reindexdb: Add the index-level REINDEX with multiple jobs

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-03-09T02:53:29Z

    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

  8. Re: pgsql: reindexdb: Add the index-level REINDEX with multiple jobs

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-03-09T21:53:00Z

    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

  9. Re: pgsql: reindexdb: Add the index-level REINDEX with multiple jobs

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-10T11:12:33Z

    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"

  10. Re: pgsql: reindexdb: Add the index-level REINDEX with multiple jobs

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-03-11T21:33:25Z

    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

  11. Re: pgsql: reindexdb: Add the index-level REINDEX with multiple jobs

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-12T09:41:43Z

    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/