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.
Attachments:
0001-Remove-IndexInfo.ii_OpclassOptions-field.patchtext/plain; charset=UTF-8; name=0001-Remove-IndexInfo.ii_OpclassOptions-field.patchDownload+33-37
0002-Remove-unused-include.patchtext/plain; charset=UTF-8; name=0002-Remove-unused-include.patchDownload+0-2
0003-Remove-RelationGetIndexRawAttOptions.patchtext/plain; charset=UTF-8; name=0003-Remove-RelationGetIndexRawAttOptions.patchDownload+5-34
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:
/messages/by-id/CAGem3qAM7M7B3DdccpgepRxuoKPd2Y74qJ5NSNRjLiN21dPhgg@mail.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:
/messages/by-id/CAGem3qAM7M7B3DdccpgepRxuoKPd2Y74qJ5NSNRjLiN21dPhgg@mail.gmail.comPerhaps 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.