Обсуждение: 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 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 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