Обсуждение: Remove IndexInfo.ii_OpclassOptions field

During some refactoring I noticed that the field 
IndexInfo.ii_OpclassOptions is kind of useless.  The IndexInfo struct is 
notionally an executor support node, but this field is not used in the 
executor or by the index AM code.  It is really just used in DDL code in 
index.c and indexcmds.c to pass information around locally.  For that, 
it would be clearer to just use local variables, like for other similar 
cases.  With that change, we can also remove 
RelationGetIndexRawAttOptions(), which only had one caller left, for 
which it was overkill.

On Thu, Aug 24, 2023 at 08:57:58AM +0200, Peter Eisentraut wrote:
> During some refactoring I noticed that the field IndexInfo.ii_OpclassOptions
> is kind of useless.  The IndexInfo struct is notionally an executor support
> node, but this field is not used in the executor or by the index AM code.
> It is really just used in DDL code in index.c and indexcmds.c to pass
> information around locally.  For that, it would be clearer to just use local
> variables, like for other similar cases.  With that change, we can also
> remove RelationGetIndexRawAttOptions(), which only had one caller left, for
> which it was overkill.

I am not so sure.  There is a very recent thread where it has been
pointed out that we have zero support for relcache invalidation with
index options, causing various problems:
https://www.postgresql.org/message-id/CAGem3qAM7M7B3DdccpgepRxuoKPd2Y74qJ5NSNRjLiN21dPhgg%40mail.gmail.com

Perhaps we'd better settle on the other one before deciding if the
change you are proposing here is adapted or not.
--
Michael

On 25.08.23 03:31, Michael Paquier wrote:
> On Thu, Aug 24, 2023 at 08:57:58AM +0200, Peter Eisentraut wrote:
>> During some refactoring I noticed that the field IndexInfo.ii_OpclassOptions
>> is kind of useless.  The IndexInfo struct is notionally an executor support
>> node, but this field is not used in the executor or by the index AM code.
>> It is really just used in DDL code in index.c and indexcmds.c to pass
>> information around locally.  For that, it would be clearer to just use local
>> variables, like for other similar cases.  With that change, we can also
>> remove RelationGetIndexRawAttOptions(), which only had one caller left, for
>> which it was overkill.
> 
> I am not so sure.  There is a very recent thread where it has been
> pointed out that we have zero support for relcache invalidation with
> index options, causing various problems:
> https://www.postgresql.org/message-id/CAGem3qAM7M7B3DdccpgepRxuoKPd2Y74qJ5NSNRjLiN21dPhgg%40mail.gmail.com
> 
> Perhaps we'd better settle on the other one before deciding if the
> change you are proposing here is adapted or not.

Ok, I'll wait for the resolution of that.

At a glance, however, I think my patch is (a) not related, and (b) if it 
were, it would probably *help*, because the change is to not allocate 
any long-lived structures that no one needs and that might get out of date.

On Tue, Aug 29, 2023 at 10:51:10AM +0200, Peter Eisentraut wrote:
> At a glance, however, I think my patch is (a) not related, and (b) if it
> were, it would probably *help*, because the change is to not allocate any
> long-lived structures that no one needs and that might get out of date.

Hmm, yeah, perhaps you're right about (b) here.  I have a few other
high-priority items for stable branches on my board before being able
to look at all this in more details, unfortunately, so feel free to
ignore me if you think that this is an improvement anyway even
regarding the other issue discussed.
--
Michael

On 30.08.23 02:51, Michael Paquier wrote:
> On Tue, Aug 29, 2023 at 10:51:10AM +0200, Peter Eisentraut wrote:
>> At a glance, however, I think my patch is (a) not related, and (b) if it
>> were, it would probably *help*, because the change is to not allocate any
>> long-lived structures that no one needs and that might get out of date.
> 
> Hmm, yeah, perhaps you're right about (b) here.  I have a few other
> high-priority items for stable branches on my board before being able
> to look at all this in more details, unfortunately, so feel free to
> ignore me if you think that this is an improvement anyway even
> regarding the other issue discussed.

I have committed this.