Horton

Thread

Commits

  1. Preserve opclass parameters across REINDEX CONCURRENTLY

  1. Opclass parameters of indexes lost after REINDEX CONCURRENTLY

    Michael Paquier <michael@paquier.xyz> — 2021-10-30T08:28:11Z

    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

  2. Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY

    Zhihong Yu <zyu@yugabyte.com> — 2021-10-30T10:59:05Z

    On Sat, Oct 30, 2021 at 1:28 AM Michael Paquier <michael@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

  3. Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY

    Zhihong Yu <zyu@yugabyte.com> — 2021-10-30T11:11:06Z

    On Sat, Oct 30, 2021 at 3:59 AM Zhihong Yu <zyu@yugabyte.com> wrote:

>
>
> On Sat, Oct 30, 2021 at 1:28 AM Michael Paquier <michael@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

  4. Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY

    Michael Paquier <michael@paquier.xyz> — 2021-10-30T12:26:35Z

    On Sat, Oct 30, 2021 at 04:11:06AM -0700, Zhihong Yu wrote:
> Should datumCopy() be used inside the loop ? I saw the following
> in get_attoptions(Oid relid, int16 attnum):

Yeah, you are right that it would be better here to use
get_attoptions() to grab a copy of each attribute's option directly
from the catalogs.  We also do that for predicates and expressions.
--
Michael

  5. Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY[

    Michael Paquier <michael@paquier.xyz> — 2021-11-01T02:55:40Z

    On Sat, Oct 30, 2021 at 09:26:35PM +0900, Michael Paquier wrote:
> Yeah, you are right that it would be better here to use
> get_attoptions() to grab a copy of each attribute's option directly
> from the catalogs.  We also do that for predicates and expressions.

While looking again at this one this morning, I have extended the
tests with more columns and some default values, then applied the
patch by using get_attoptions() to grab each attribute's options as of
add5cf2.  Predicates and expressions are grabbed through the syscache,
so that's just more consistent.
--
Michael