On Sat, Oct 30, 2021 at 3:59 AM Zhihong Yu <z...@yugabyte.com> wrote:
>
>
> On Sat, Oct 30, 2021 at 1:28 AM Michael Paquier <mich...@paquier.xyz>
> wrote:
>
>> Hi all,
>>
>> While reviewing the code for opclass parameters with indexes, I have
>> noticed that opclass parameters are lost after a concurrent reindex.
>> As we use a IndexInfo to hold the information of the new index when
>> creating a copy of the old one, it is just a matter of making sure
>> that ii_OpclassOptions is filled appropriately, but that was missed by
>> 911e702.
>>
>> Attached is a patch to fix the issue. After a concurrent reindex, we
>> would not finish with a corrupted index, just with one rebuilt with
>> default opclass parameter values.
>>
>> Any objections or comments?
>> --
>> Michael
>>
> Hi,
>
> + newInfo->ii_OpclassOptions = palloc0(sizeof(Datum) *
> + newInfo->ii_NumIndexAttrs);
>
> It seems we may not need to allocate these many entries (as shown by the
> concur_appclass_ind_2 example in the test).
> In the previous loop (starting line 1359), we can increment a counter
> which would finally tell us how many oldInfo->ii_OpclassOptions[i] is not
> NULL.
>
> Then that many entries can be allocated in the above code.
>
> Cheers
>
Hi,
Upon further look, my previous comment was premature. Please ignore that.
+ newInfo->ii_OpclassOptions[i] = oldInfo->ii_OpclassOptions[i];
Should datumCopy() be used inside the loop ? I saw the following
in get_attoptions(Oid relid, int16 attnum):
result = datumCopy(attopts, false, -1); /* text[] */
Cheers
