Re: [HACKERS] [PROPOSAL] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind
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
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
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
В письме от 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
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
В письме от 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
В письме от 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
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
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
В письме от 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
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
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
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
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