Add Index-level REINDEX with multiple jobs

On Fri, Dec 29, 2023 at 04:15:35PM +0300, Maxim Orlov wrote:

Recently, one of our customers came to us with the question: why do reindex
utility does not support multiple jobs for indices (-i opt)?
And, of course, it is because we cannot control the concurrent processing
of multiple indexes on the same relation. This was
discussed somewhere in [0], I believe. So, customer have to make a shell
script to do his business and so on.

Yep, that should be the correct thread. As far as I recall, one major
reason was code simplicity because dealing with parallel jobs at table
level is a no-brainer on the client side (see 0003): we know that
relations with physical storage will never interact with each other.

But. This seems to be not that complicated to split indices by parent
tables and do reindex in multiple jobs? Or I miss something?
PFA patch implementing this.

+   appendPQExpBufferStr(&catalog_query,
+                       "WITH idx as (\n"
+                       "  SELECT c.relname, ns.nspname\n"
+                       "  FROM pg_catalog.pg_class c,\n"
+                       "       pg_catalog.pg_namespace ns\n"
+                       "  WHERE c.relnamespace OPERATOR(pg_catalog.=) ns.oid AND\n"
+                       "        c.oid OPERATOR(pg_catalog.=) ANY(ARRAY['\n");

The problem may be actually trickier than that, no? Could there be
other factors to take into account for their classification, like
their sizes (typically, we'd want to process the biggest one first, I
guess)?
--
Michael

On 6 Feb 2024, at 11:21, Michael Paquier <michael@paquier.xyz> wrote:

The problem may be actually trickier than that, no? Could there be
other factors to take into account for their classification, like
their sizes (typically, we'd want to process the biggest one first, I
guess)?

Maxim, what do you think about it?

Best regards, Andrey Borodin.

Andrey M. Borodin писал(а) 2024-03-04 09:26:

On 6 Feb 2024, at 11:21, Michael Paquier <michael@paquier.xyz> wrote:

The problem may be actually trickier than that, no? Could there be
other factors to take into account for their classification, like
their sizes (typically, we'd want to process the biggest one first, I
guess)?

Maxim, what do you think about it?

Best regards, Andrey Borodin.

I agree that, as is the case with tables in REINDEX SCHEME, indices
should be sorted accordingly to their size.

Attaching the second version of patch, with indices being sorted by
size.

Best regards,
Svetlana Derevyanko

On Tue, 6 Feb 2024 at 09:22, Michael Paquier <michael@paquier.xyz> wrote:

The problem may be actually trickier than that, no? Could there be
other factors to take into account for their classification, like
their sizes (typically, we'd want to process the biggest one first, I
guess)?

Sorry for a late reply. Thanks for an explanation. This is sounds
reasonable to me.
Svetlana had addressed this in the patch v2.

--
Best regards,
Maxim Orlov.

On Mon, Mar 11, 2024 at 3:44 PM Maxim Orlov <orlovmg@gmail.com> wrote:

On Tue, 6 Feb 2024 at 09:22, Michael Paquier <michael@paquier.xyz> wrote:

The problem may be actually trickier than that, no? Could there be
other factors to take into account for their classification, like
their sizes (typically, we'd want to process the biggest one first, I
guess)?

Sorry for a late reply. Thanks for an explanation. This is sounds reasonable to me.
Svetlana had addressed this in the patch v2.

I think this patch is a nice improvement. But it doesn't seem to be
implemented in the right way. There is no guarantee that
get_parallel_object_list() will return tables in the same order as
indexes. Especially when there is "ORDER BY idx.relpages". Also,
sort_indices_by_tables() has quadratic complexity (probably OK since
input list shouldn't be too lengthy) and a bit awkward.

I've revised the patchset. Now appropriate ordering is made in SQL
query. The original list of indexes is modified to match the list of
tables. The tables are ordered by the size of its greatest index,
within table indexes are ordered by size.

I'm going to further revise this patch, mostly comments and the commit message.

------
Regards,
Alexander Korotkov

On Wed, Mar 20, 2024 at 7:19 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:

On Mon, Mar 11, 2024 at 3:44 PM Maxim Orlov <orlovmg@gmail.com> wrote:

On Tue, 6 Feb 2024 at 09:22, Michael Paquier <michael@paquier.xyz> wrote:

The problem may be actually trickier than that, no? Could there be
other factors to take into account for their classification, like
their sizes (typically, we'd want to process the biggest one first, I
guess)?

Sorry for a late reply. Thanks for an explanation. This is sounds reasonable to me.
Svetlana had addressed this in the patch v2.

I think this patch is a nice improvement. But it doesn't seem to be
implemented in the right way. There is no guarantee that
get_parallel_object_list() will return tables in the same order as
indexes. Especially when there is "ORDER BY idx.relpages". Also,
sort_indices_by_tables() has quadratic complexity (probably OK since
input list shouldn't be too lengthy) and a bit awkward.

I've revised the patchset. Now appropriate ordering is made in SQL
query. The original list of indexes is modified to match the list of
tables. The tables are ordered by the size of its greatest index,
within table indexes are ordered by size.

I'm going to further revise this patch, mostly comments and the commit message.

Here goes the revised patch. I'm going to push this if there are no objections.

------
Regards,
Alexander Korotkov

Alexander Korotkov <aekorotkov@gmail.com> writes:

Here goes the revised patch. I'm going to push this if there are no objections.

Quite a lot of the buildfarm is complaining about this:

reindexdb.c: In function 'reindex_one_database':
reindexdb.c:434:54: error: 'indices_tables_cell' may be used uninitialized in this function [-Werror=maybe-uninitialized]
434 | strcmp(prev_index_table_name, indices_tables_cell->val) == 0)
| ~~~~~~~~~~~~~~~~~~~^~~~~

I noticed it first on mamba, which is set up with -Werror, but a
scrape of the buildfarm logs shows many other animals reporting this
as a warning. I think they are right. Even granting that the
compiler realizes that "parallel && process_type == REINDEX_INDEX" is
enough to reach the one place where indices_tables_cell is
initialized, that's not really enough, because that place is

if (indices_tables_list)
indices_tables_cell = indices_tables_list->head;

So I believe this code will crash if get_parallel_object_list returns
an empty list. Initializing indices_tables_cell to NULL in its
declaration would stop the compiler warning, but if I'm right it
will do nothing to prevent that crash. This needs a bit more effort.

regards, tom lane

On Mon, Mar 25, 2024 at 10:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alexander Korotkov <aekorotkov@gmail.com> writes:

Here goes the revised patch. I'm going to push this if there are no

objections.

Quite a lot of the buildfarm is complaining about this:

reindexdb.c: In function 'reindex_one_database':
reindexdb.c:434:54: error: 'indices_tables_cell' may be used uninitialized
in this function [-Werror=maybe-uninitialized]
434 | strcmp(prev_index_table_name, indices_tables_cell->val) == 0)
| ~~~~~~~~~~~~~~~~~~~^~~~~

I noticed it first on mamba, which is set up with -Werror, but a
scrape of the buildfarm logs shows many other animals reporting this
as a warning.

I noticed the similar warning on cfbot:
https://cirrus-ci.com/task/6298504306360320?logs=gcc_warning#L448

reindexdb.c: In function ‘reindex_one_database’:
reindexdb.c:437:24: error: ‘indices_tables_cell’ may be used uninitialized
in this function [-Werror=maybe-uninitialized]
437 | indices_tables_cell = indices_tables_cell->next;
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~

Although it's complaining on line 437 not 434, I think they are the same
issue.

I think they are right. Even granting that the
compiler realizes that "parallel && process_type == REINDEX_INDEX" is
enough to reach the one place where indices_tables_cell is
initialized, that's not really enough, because that place is

if (indices_tables_list)
indices_tables_cell = indices_tables_list->head;

So I believe this code will crash if get_parallel_object_list returns
an empty list. Initializing indices_tables_cell to NULL in its
declaration would stop the compiler warning, but if I'm right it
will do nothing to prevent that crash. This needs a bit more effort.

Agreed. And the comment of get_parallel_object_list() says that it may
indeed return NULL.

BTW, on line 373, it checks 'process_list' and bails out if this list is
NULL. But it seems to me that 'process_list' cannot be NULL in this
case, because it's initialized to be 'user_list' and we have asserted
that user_list is not NULL on line 360. I wonder if we should check
indices_tables_list instead of process_list on line 373.

Thanks
Richard

On Mon, Mar 25, 2024 at 4:47 AM Richard Guo <guofenglinux@gmail.com> wrote:

On Mon, Mar 25, 2024 at 10:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alexander Korotkov <aekorotkov@gmail.com> writes:

Here goes the revised patch. I'm going to push this if there are no objections.

Quite a lot of the buildfarm is complaining about this:

reindexdb.c: In function 'reindex_one_database':
reindexdb.c:434:54: error: 'indices_tables_cell' may be used uninitialized in this function [-Werror=maybe-uninitialized]
434 | strcmp(prev_index_table_name, indices_tables_cell->val) == 0)
| ~~~~~~~~~~~~~~~~~~~^~~~~

I noticed it first on mamba, which is set up with -Werror, but a
scrape of the buildfarm logs shows many other animals reporting this
as a warning.

I noticed the similar warning on cfbot:
https://cirrus-ci.com/task/6298504306360320?logs=gcc_warning#L448

reindexdb.c: In function ‘reindex_one_database’:
reindexdb.c:437:24: error: ‘indices_tables_cell’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
437 | indices_tables_cell = indices_tables_cell->next;
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~

Although it's complaining on line 437 not 434, I think they are the same
issue.

I think they are right. Even granting that the
compiler realizes that "parallel && process_type == REINDEX_INDEX" is
enough to reach the one place where indices_tables_cell is
initialized, that's not really enough, because that place is

if (indices_tables_list)
indices_tables_cell = indices_tables_list->head;

So I believe this code will crash if get_parallel_object_list returns
an empty list. Initializing indices_tables_cell to NULL in its
declaration would stop the compiler warning, but if I'm right it
will do nothing to prevent that crash. This needs a bit more effort.

Agreed. And the comment of get_parallel_object_list() says that it may
indeed return NULL.

BTW, on line 373, it checks 'process_list' and bails out if this list is
NULL. But it seems to me that 'process_list' cannot be NULL in this
case, because it's initialized to be 'user_list' and we have asserted
that user_list is not NULL on line 360. I wonder if we should check
indices_tables_list instead of process_list on line 373.

Thank you. I'm going to deal with this in the next few hours.

------
Regards,
Alexander Korotkov