Re: Add Index-level REINDEX with multiple jobs

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