Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY[
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 signature.asc Description: PGP signature
Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY
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 signature.asc Description: PGP signature
Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY
On Sat, Oct 30, 2021 at 3:59 AM Zhihong Yu wrote: > > > On Sat, Oct 30, 2021 at 1:28 AM Michael Paquier > 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
Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY
On Sat, Oct 30, 2021 at 1:28 AM Michael Paquier 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
