Обсуждение: 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.