Обсуждение: [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind

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 Company

On Tue, May 24, 2016 at 10:12 AM, Nikolay Shaplov
<n.shaplov@postgrespro.ru> 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

Robert Haas wrote:
> On Tue, May 24, 2016 at 10:12 AM, Nikolay Shaplov
> <n.shaplov@postgrespro.ru> 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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> 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: teodor@sigaev.ru
  WWW: http://www.sigaev.ru/
 

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

В письме от 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

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=OdmAkK5tiOOw@mail.gmail.com
for previous discussion.

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

В письме от 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
> =OdmAkK5tiOOw@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

В письме от 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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

В письме от 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...

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> 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

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> 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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services