Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> > Nikolay Shaplov wrote:
> >> Story start from the point that I found out that a.m. can not forbid 
> >> changing 
> >> some of it's reloptions with ALTER INDEX command.
> 
> > Hmm, this sounds like a bug to me.  In BRIN, if you change the
> > pages_per_range option for an existing index, the current index
> > continues to work because the value used during the last index build is
> > stored in the metapage.  Only when you reindex after changing the option
> > the new value takes effect.
> 
> > I think Bloom should do likewise.
> 
> AFAICT, Bloom *does* do that.  The reloptions are only consulted directly
> while initializing the metapage.

Ah, clearly I misunderstood what he was saying.

> I think Nikolay's complaint is essentially that there should be a way
> for an AM to forbid ALTER INDEX if it's not going to support on-the-fly
> changes of a given reloption.  That might be a helpful usability
> improvement, but it's only a usability improvement not a bug fix.

I can get behind that idea, but this also says I should get away from
this thread until 10dev opens.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Tom Lane]

Alvaro Herrera  writes:
> Nikolay Shaplov wrote:
>> Story start from the point that I found out that a.m. can not forbid 
>> changing 
>> some of it's reloptions with ALTER INDEX command.

> Hmm, this sounds like a bug to me.  In BRIN, if you change the
> pages_per_range option for an existing index, the current index
> continues to work because the value used during the last index build is
> stored in the metapage.  Only when you reindex after changing the option
> the new value takes effect.

> I think Bloom should do likewise.

AFAICT, Bloom *does* do that.  The reloptions are only consulted directly
while initializing the metapage.

I think Nikolay's complaint is essentially that there should be a way
for an AM to forbid ALTER INDEX if it's not going to support on-the-fly
changes of a given reloption.  That might be a helpful usability
improvement, but it's only a usability improvement not a bug fix.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Alvaro Herrera]

Nikolay Shaplov wrote:
> В письме от 27 мая 2016 15:05:58 Вы написали:
> > Nikolay Shaplov wrote:
> > > Story start from the point that I found out that a.m. can not forbid
> > > changing some of it's reloptions with ALTER INDEX command. That was not
> > > necessary before, because all reloptions at that existed at that time can
> > > be changed on fly. But now for bloom index it is unacceptable, because
> > > for changing bloom's reloptions for existing index will lead to index
> > > malfunction.
> > 
> > Hmm, this sounds like a bug to me.  In BRIN, if you change the
> > pages_per_range option for an existing index, the current index
> > continues to work because the value used during the last index build is
> > stored in the metapage.  Only when you reindex after changing the option
> > the new value takes effect.
> > 
> > I think Bloom should do likewise.
> 
> I do not think that it is the best behavior. Because if we came to this 
> situation, the current value of pages_per_range that index actually using is 
> not available for user, because he is not able to look into meta page.

You're right in that, but this is a much less serious behavior than
causing the index to fail to work altogether just because the option was
changed.  Since we're certainly not going to rework reloptions in 9.6,
IMO the bloom behavior should be changed to match BRIN's.

> In this case it would be better either forbid changing the options, so it 
> would be consistent, or force index rebuild, then it would be consistent too. 
> I would vote for first behavior as this is less work to do for me, and can be 
> changed later, if it is really needed for some case.

I think forcing index rebuild is not a great idea.  That turns a simple
ALTER INDEX command which doesn't block the index heavily nor for a long
time, into a much more serious deal.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Nikolay Shaplov]

В письме от 27 мая 2016 15:05:58 Вы написали:
> Nikolay Shaplov wrote:
> > Story start from the point that I found out that a.m. can not forbid
> > changing some of it's reloptions with ALTER INDEX command. That was not
> > necessary before, because all reloptions at that existed at that time can
> > be changed on fly. But now for bloom index it is unacceptable, because
> > for changing bloom's reloptions for existing index will lead to index
> > malfunction.
> 
> Hmm, this sounds like a bug to me.  In BRIN, if you change the
> pages_per_range option for an existing index, the current index
> continues to work because the value used during the last index build is
> stored in the metapage.  Only when you reindex after changing the option
> the new value takes effect.
> 
> I think Bloom should do likewise.

I do not think that it is the best behavior. Because if we came to this 
situation, the current value of pages_per_range that index actually using is 
not available for user, because he is not able to look into meta page.

In this case it would be better either forbid changing the options, so it 
would be consistent, or force index rebuild, then it would be consistent too. 
I would vote for first behavior as this is less work to do for me, and can be 
changed later, if it is really needed for some case.

PS sorry I did not add mail list to the CC, so I send it second time...


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Alvaro Herrera]

Nikolay Shaplov wrote:

> Story start from the point that I found out that a.m. can not forbid changing 
> some of it's reloptions with ALTER INDEX command. That was not necessary  
> before, because all reloptions at that existed at that time can be changed on 
> fly. But now for bloom index it is unacceptable, because for changing bloom's 
> reloptions for existing index will lead to index malfunction.

Hmm, this sounds like a bug to me.  In BRIN, if you change the
pages_per_range option for an existing index, the current index
continues to work because the value used during the last index build is
stored in the metapage.  Only when you reindex after changing the option
the new value takes effect.

I think Bloom should do likewise.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Nikolay Shaplov]

В письме от 24 мая 2016 17:12:16 пользователь Nikolay Shaplov написал:

While working on this patch I met some difficulties that makes me to completely 
rewrite a code that is responsible for interacting reloptions.c with access 
methods.

Story start from the point that I found out that a.m. can not forbid changing 
some of it's reloptions with ALTER INDEX command. That was not necessary  
before, because all reloptions at that existed at that time can be changed on 
fly. But now for bloom index it is unacceptable, because for changing bloom's 
reloptions for existing index will lead to index malfunction.

I was thinking about adding for_alter flag argument for parseRelOptions funtion 
and pass it all the way from indexcmds.c & tablecmds.c, and add some flag in 
the definition of reloptions to mark those reloptions that should not be used 
in ALTER INDEX clause. 

This theoretically this will give us a way to throw error when user tries to 
change an reloption that should not be changed.

But unfortunately it turned out that in ATExecSetRelOptions new reloptions is 
merged with existing reloptions values using transformRelOptions, and only 
after that the result is sent for validation. 

So on the step of the validation we can not distinguish old options from a new 
one. 

I've been thinking how to work this around, but found out that there is no 
simple way. I can pass another array of flags through the whole call stack. But 
this make all thing uglier. 

I can create another function that do pre-validation for new reloptions, 
before doing final validation of the whole set. But this will make me to have  
to entities available from the a.m.: the descriptor of  reloptions and 
function for custom validation of the results (i.e. like adjustBloomOptions 
for Bloom).

But it would be not good if we dedicate two entires of a.m. to reoptions 
definition. And there can be even more... 

So I came to a conclusion: gather all the staff that defines the all the 
behavior of reloption parsing and validation into one structure, both array of 
relopt_value, custom validation function, and all the other staff.

And this structure is the only thing that a.m. gives to reloptions.c code for 
parsing and validation purposes. And that should be enough.

I hope this make code more flexible and more easy to understand.



-- 
Nikolay Shaplov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Nikolay Shaplov]

В письме от 25 мая 2016 14:03:17 Вы написали:

> > > > >This all should me moved behind "access method" abstraction...
> > > > 
> > > > +1 relopt_kind should be moved in am, at least. Or removed.
> > > 
> > > Hm, but we have tablespace options too, so I'm not sure that using AM as
> > > abstraction level is correct.
> > 
> > We will use am for all indexes, and keep all the rest in relopotion.c for
> > non-indexes. May be divided options catalog into sections one section for
> > each kind.
> That makes sense.  I can review the patch later.
That would be great! ;-)


> 
> > And as I also would like to use this code for dynamic attoptions later, I
> > would like to remove relopt_kind at all. Because it spoils live in that
> > case.
> As I remember, Fabrízio (in CC) had a patch for dynamic reloptions, but
> there was some problem with it and we dumped it; see
> https://www.postgresql.org/message-id/CAFcNs+rqCq1H5eXW-cvdti6T-xo2STEkhjREx
> =odmakk5ti...@mail.gmail.com for previous discussion.

I've read the start of that thread. In my opinion Fabrízio offering quite 
different thing. I am speaking about adding options to a new object (access 
method or later operator class). You add these options when you create this 
object and you have a monopoly of defining options for it.

Fabrízio offers adding options for relkind that already exists. This seems to 
be a complex thing. You should resolve conflicts between two extensions that 
want to define same option for example. Somehow deal with the situation when 
the extension is dropped. Should we remove reloptions created by that 
extension from pg_class then?

And moreover, as far as I understand reloptions is about how does relation 
operates. And the external extension would not affect this at all. So we are 
speaking not about options of a relation, but about extension that want to 
store some info about some relation. I think in general this extension should 
store it's info into it's own data structures. 

May be there is cases when you will really need such behavior. But it is quite 
different task, have almost nothing in common with things I am going to do :-)

-- 
Nikolay Shaplov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Fabrízio de Royes Mello]

On Wed, May 25, 2016 at 3:03 PM, Alvaro Herrera 
wrote:
>
> Nikolay Shaplov wrote:
> > В письме от 25 мая 2016 13:25:38 Вы написали:
> > > Teodor Sigaev wrote:
> > > > >This all should me moved behind "access method" abstraction...
> > > >
> > > > +1 relopt_kind should be moved in am, at least. Or removed.
> > >
> > > Hm, but we have tablespace options too, so I'm not sure that using AM
as
> > > abstraction level is correct.
> > We will use am for all indexes, and keep all the rest in relopotion.c
for
> > non-indexes. May be divided options catalog into sections one section
for each
> > kind.
>
> That makes sense.  I can review the patch later.
>
> > And as I also would like to use this code for dynamic attoptions later,
I
> > would like to remove relopt_kind at all. Because it spoils live in that
case.
>
> As I remember, Fabrízio (in CC) had a patch for dynamic reloptions, but
> there was some problem with it and we dumped it; see
>
https://www.postgresql.org/message-id/CAFcNs+rqCq1H5eXW-cvdti6T-xo2STEkhjREx=odmakk5ti...@mail.gmail.com
> for previous discussion.
>

Yeah, and it was forked into other long discussion thread [1] that explain
the reasons to patch got rejected.

IMHO we need a way (maybe at SQL level too) to define and manipulate the
reloptions, and this way should cover all database objects. It isn't a
simple patch because we'll need introduce new catalogs, refactor and
rewrite a lot of code... but it should be a better way to do it. Anyway we
can start with your approach and grow it in small pieces. I have a lot of
ideas about it so I'm glad to discuss it if you want.

Regards,

[1]
https://www.postgresql.org/message-id/cafj8prcx_vdcsdbumknhhyr_-n_ctl84_7r+-bj17hckt_m...@mail.gmail.com
[2]
https://www.postgresql.org/message-id/ca+tgmoznfdqt2kotx38yjus3f_avisclxawbmpdwxhyxrg_...@mail.gmail.com

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Alvaro Herrera]

Nikolay Shaplov wrote:
> В письме от 25 мая 2016 13:25:38 Вы написали:
> > Teodor Sigaev wrote:
> > > >This all should me moved behind "access method" abstraction...
> > > 
> > > +1 relopt_kind should be moved in am, at least. Or removed.
> > 
> > Hm, but we have tablespace options too, so I'm not sure that using AM as
> > abstraction level is correct.
> We will use am for all indexes, and keep all the rest in relopotion.c for 
> non-indexes. May be divided options catalog into sections one section for 
> each 
> kind.

That makes sense.  I can review the patch later.

> And as I also would like to use this code for dynamic attoptions later, I 
> would like to remove relopt_kind at all. Because it spoils live in that case.

As I remember, Fabrízio (in CC) had a patch for dynamic reloptions, but
there was some problem with it and we dumped it; see
https://www.postgresql.org/message-id/CAFcNs+rqCq1H5eXW-cvdti6T-xo2STEkhjREx=odmakk5ti...@mail.gmail.com
for previous discussion.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Nikolay Shaplov]

В письме от 25 мая 2016 13:25:38 Вы написали:
> Teodor Sigaev wrote:
> > >This all should me moved behind "access method" abstraction...
> > 
> > +1 relopt_kind should be moved in am, at least. Or removed.
> 
> Hm, but we have tablespace options too, so I'm not sure that using AM as
> abstraction level is correct.
We will use am for all indexes, and keep all the rest in relopotion.c for 
non-indexes. May be divided options catalog into sections one section for each 
kind.

And as I also would like to use this code for dynamic attoptions later, I 
would like to remove relopt_kind at all. Because it spoils live in that case.

-- 
Nikolay Shaplov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Alvaro Herrera]

Teodor Sigaev wrote:
> >This all should me moved behind "access method" abstraction...
> 
> +1 relopt_kind should be moved in am, at least. Or removed.

Hm, but we have tablespace options too, so I'm not sure that using AM as
abstraction level is correct.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Teodor Sigaev]

This all should me moved behind "access method" abstraction...


+1 relopt_kind should be moved in am, at least. Or removed.

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Alvaro Herrera]

Robert Haas wrote:
> On Tue, May 24, 2016 at 10:12 AM, Nikolay Shaplov
>  wrote:
> > While working on patch for attribute options for indexes (see
> > http://www.postgresql.org/message-id/5213596.TqFRiqmCTe@nataraj-amd64 )
> > I found out that code for reloptions is not flexible at all.
> >
> > All definitions of attoptons are stored in central catalog in
> > src/backend/access/common/reloptions.c
> > It is done this way for both heap and tuple relations, and also for all 
> > index
> > access methods that goes with source code.
> >
> > Most of the code of the indexes is now hidden behind
> > "access method" abstraction, but not the reloption code. If you grep through
> > src/backend/access/common/reloptions.c, you will find RELOPT_KIND_GIN,
> > RELOPT_KIND_BTREE, RELOPT_KIND_GIST, RELOPT_KIND_SPGIST, RELOPT_KIND_BRIN...
> >
> > This all should me moved behind "access method" abstraction...
> 
> +1 for that concept.  I'm not sure whether your implementation is good
> or bad because I haven't really looked at it, but I agree that the way
> the reloption stuff is structured is a nuisance, and injecting more
> abstraction there seems like a good thing.

As the author of the old stuff, I agree.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Robert Haas]

On Tue, May 24, 2016 at 10:12 AM, Nikolay Shaplov
 wrote:
> While working on patch for attribute options for indexes (see
> http://www.postgresql.org/message-id/5213596.TqFRiqmCTe@nataraj-amd64 )
> I found out that code for reloptions is not flexible at all.
>
> All definitions of attoptons are stored in central catalog in
> src/backend/access/common/reloptions.c
> It is done this way for both heap and tuple relations, and also for all index
> access methods that goes with source code.
>
> Most of the code of the indexes is now hidden behind
> "access method" abstraction, but not the reloption code. If you grep through
> src/backend/access/common/reloptions.c, you will find RELOPT_KIND_GIN,
> RELOPT_KIND_BTREE, RELOPT_KIND_GIST, RELOPT_KIND_SPGIST, RELOPT_KIND_BRIN...
>
> This all should me moved behind "access method" abstraction...

+1 for that concept.  I'm not sure whether your implementation is good
or bad because I haven't really looked at it, but I agree that the way
the reloption stuff is structured is a nuisance, and injecting more
abstraction there seems like a good thing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

[Nikolay Shaplov]

While working on patch for attribute options for indexes (see  
http://www.postgresql.org/message-id/5213596.TqFRiqmCTe@nataraj-amd64 )
I found out that code for reloptions is not flexible at all.

All definitions of attoptons are stored in central catalog in 
src/backend/access/common/reloptions.c
It is done this way for both heap and tuple relations, and also for all index 
access methods that goes with source code. 

Most of the code of the indexes is now hidden behind 
"access method" abstraction, but not the reloption code. If you grep through 
src/backend/access/common/reloptions.c, you will find RELOPT_KIND_GIN, 
RELOPT_KIND_BTREE, RELOPT_KIND_GIST, RELOPT_KIND_SPGIST, RELOPT_KIND_BRIN...

This all should me moved behind "access method" abstraction...

Custom indexes, like postgresql/contrib/bloom can add own reloptions, and 
relopt_kind. But the number of relopt_kinfd available is short (it would be 
enough for reloptions, but if we add attoptions, we will meet shortage soon).

So my proposial is the following: Each access method has it's own catalog of 
options (reloptions, and later attoptions) and when it want to call any 
function from src/backend/access/common/reloptions.c it uses a reference to 
that catalog instead of relopt_kind.

In the patch that is attached to this message, there is an idea of how it can 
be done.

In that patch I've left relopt_kind, and added refence to options catalog, 
and functions from reloptions.c uses that one that is defined. I've implemented 
"catalog reference" version for bloom index, and all the rest still work as 
they were.

In final patch there should be no relopt_kind at all, all descriptions of 
reloptions for indexes should go to src/backend/access/[am-name], reloptions 
for heap, toast, views and so on, should stay in 
src/backend/access/common/reloptions.c but should be stored as separate option 
catalog for each type. I still not sure what to do  with 
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST options. But one or another solutions can 
be found...

-- 
Nikolay Shaplov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Companydiff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index 05dbe87..a71f7d0 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -36,8 +36,8 @@
 
 PG_FUNCTION_INFO_V1(blhandler);
 
-/* Kind of relation optioms for bloom index */
-static relopt_kind bl_relopt_kind;
+/* Catalog of relation options for bloom index */
+static options_catalog bl_relopt_catalog;
 
 static int32 myRand(void);
 static void mySrand(uint32 seed);
@@ -51,15 +51,18 @@ _PG_init(void)
 	int			i;
 	char		buf[16];
 
-	bl_relopt_kind = add_reloption_kind();
+	bl_relopt_catalog.definitions = NULL;
+	bl_relopt_catalog.num = 0;
+	bl_relopt_catalog.max = 0;
+	bl_relopt_catalog.need_initialization = 1;
 
-	add_int_reloption(bl_relopt_kind, "length",
+	add_int_reloption(0, _relopt_catalog, "length",
 	  "Length of signature in uint16 type", 5, 1, 256);
 
 	for (i = 0; i < INDEX_MAX_KEYS; i++)
 	{
 		snprintf(buf, 16, "col%d", i + 1);
-		add_int_reloption(bl_relopt_kind, buf,
+		add_int_reloption(0, _relopt_catalog, buf,
 	  "Number of bits for corresponding column", 2, 1, 2048);
 	}
 }
@@ -457,7 +460,7 @@ bloptions(Datum reloptions, bool validate)
 		tab[i + 1].offset = offsetof(BloomOptions, bitSize[i]);
 	}
 
-	options = parseRelOptions(reloptions, validate, bl_relopt_kind, );
+	options = parseRelOptions(reloptions, validate, _relopt_catalog, 0, );
 	rdopts = allocateReloptStruct(sizeof(BloomOptions), options, numoptions);
 	fillRelOptions((void *) rdopts, sizeof(BloomOptions), options, numoptions,
    validate, tab, INDEX_MAX_KEYS + 1);
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 89bad05..c804212 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -761,7 +761,7 @@ brinoptions(Datum reloptions, bool validate)
 		{"pages_per_range", RELOPT_TYPE_INT, offsetof(BrinOptions, pagesPerRange)}
 	};
 
-	options = parseRelOptions(reloptions, validate, RELOPT_KIND_BRIN,
+	options = parseRelOptions(reloptions, validate, NULL, RELOPT_KIND_BRIN,
 			  );
 
 	/* if none set, we're done */
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 7448c7f..38fe2c8 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -378,6 +378,7 @@ static bits32 last_assigned_kind = RELOPT_KIND_LAST_DEFAULT;
 static int	num_custom_options = 0;
 static relopt_gen **custom_options = NULL;
 static bool need_initialization = true;
+static int	max_custom_options = 0;
 
 static void initialize_reloptions(void);
 static void parse_one_reloption(relopt_value *option, char *text_str,
@@ -496,32 +497,31 @@ add_reloption_kind(void)
  *		main parser table.
  */
 static void
-add_reloption(relopt_gen *newoption)
+add_reloption(relopt_gen *newoption, options_catalog * catalog)
 {
-	static int