Re: Add Index-level REINDEX with multiple jobs

On Mon, Mar 25, 2024 at 4:47 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> On Mon, Mar 25, 2024 at 10:07 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>> Alexander Korotkov <aekorotkov(at)gmail(dot)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