Re: [HACKERS] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал:
> > I've just made sure that patch is still applyable to the current master.
> >
> > And I am still waiting for reviews :-)
>
> I see that this patch adds a few tests for reloptions, for instance in
> bloom. I think we should split this in at least two commits, one that
> adds those tests before the functionality change, so that they can be
> committed in advance, verify that the buildfarm likes it with the
> current code, and verify the coverage.
This sounds as a good idea. I created such patch and started a new thread for
it https://www.postgresql.org/message-id/2615372.orqtEn8VGB%40x200m (I will
add that thread to commitfest as soon as I understand what test should be left
there)
> I also noticed that your patch adds an error message that didn't exist
> previously,
>
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("col%i should not be grater than length",
> i)));
>
> this seems a bit troublesome, because of the "col%i" thing
What the problem with col%i ?
> and also because of the typo.
Oh... I always had bad marks for all natural languages in school :-( ;-)
> I wonder if this means you've changed the way sanity checks work here.
This should not be like this (I hope). I will attentively look at this part of
code again, and explain what exactly I've done. (I did it a year ago and now
need some time to recall)
> The new checks around toast table creation look like they belong to a
> completely independent patch also ...
This sounds reasonable too. Will do it, it is possible, as far as I remember.
> the error message there goes against message style guidelines also.
Oh... these style guidelines... will pay attention to it too...
--
Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)
--
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] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
Nikolay Shaplov wrote:
> В письме от 25 июня 2017 21:05:49 пользователь Nikolay Shaplov написал:
>
> I've just made sure that patch is still applyable to the current master.
>
> And I am still waiting for reviews :-)
I see that this patch adds a few tests for reloptions, for instance in
bloom. I think we should split this in at least two commits, one that
adds those tests before the functionality change, so that they can be
committed in advance, verify that the buildfarm likes it with the
current code, and verify the coverage.
I also noticed that your patch adds an error message that didn't exist
previously,
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("col%i should not be grater than length", i)));
this seems a bit troublesome, because of the "col%i" thing and also
because of the typo. I wonder if this means you've changed the way
sanity checks work here.
The new checks around toast table creation look like they belong to a
completely independent patch also ... the error message there goes
against message style guidelines also.
--
Álvaro Herrerahttps://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] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
В письме от 26 марта 2017 15:02:12 пользователь Alvaro Herrera написал: > Nikolay Shaplov wrote: > > If I would think about it now: we always know how many options we will > > have. So we can just pass this number to palloc and assert if somebody > > adds more options then expected... What do yo think about it. > > I think we need to preserve the ability to add custom options, because > extensions may want to do that. I've been thinking about it for a while... I think this might lead to reducing the quality of the code... For example: There was 10 options for some relation type. I decided to add one more option, but i did not ++ the number of options for allocateOptionsCatalog. So space for 10 options were allocated, and then when 11th option is added, optionCatalogAddItem would allocate space for ten more options, and nine of them would not be used. And nobody will notice it. So, I see here four solutions: 0. Leave it as it was. (We both do not like it) 1. Forbid dynamic number of options (you do not like it) 2. Allow dynamic number of options only for special cases, and in all other cases make them strict, and asserting if option number is wrong. This can be done in two ways: 2a. Add strict boolean flag, that tells if allow to add more options or not 2b. Do not add any flags, but if number of items is specified, then process number of items in strict mode. If it is set to -1, then do as it is done now, totally dynamically. I would prefer 2b, if you sure that somebody will need dynamic number of options. PS. I hardly believe that there can be dynamic number of options, as this options later are mapped into C-structure that is quite static. No use case comes into my mind, where I would like to have dynamic number of options, not knowing at build time, how many of them there would be. -- Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire. -- 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] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
Nikolay Shaplov wrote: > В письме от 17 марта 2017 14:21:26 пользователь Alvaro Herrera написал: > > > Please make sure to mark functions as static (e.g. bringetreloptcatalog). > I am a bit confused here: > > For brin and nbtree this is a good idea: brin.c and nbtree.c has AM-handler > inside it, and there are other static functions there. Adding one more > static > function here seems to be quite logical. I am just saying that if there is a function used only in one compilation unit (.c file) then let's make sure its prototype says "static". > For gin, gist and spgist, authors seems to use [index_name]_private.h files > to > hide internal functions from outside code. In ginutil.c and spgutils.c, where > AM-handler is located, there is no static functions at all... gist.c has, > but > I think I should write similar code for all *_private.h indexes. Sure. > hash.c is quite a mess... > There is no hash_private.h, AM-handles is located in hash.c, that has "This > file contains only the public interface routines." comment at the beginning, > and there is no static functions inside. I do not know what is the right way > to hide hashgetreloptcatalog function here... > > What would you advise? Leave it where it is (hashutil.c), no static marker. We may want to move things around later, but that's not your patch's responsibility. -- Álvaro Herrerahttps://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] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
В письме от 17 марта 2017 14:21:26 пользователь Alvaro Herrera написал: > Please make sure to mark functions as static (e.g. bringetreloptcatalog). I am a bit confused here: For brin and nbtree this is a good idea: brin.c and nbtree.c has AM-handler inside it, and there are other static functions there. Adding one more static function here seems to be quite logical. For gin, gist and spgist, authors seems to use [index_name]_private.h files to hide internal functions from outside code. In ginutil.c and spgutils.c, where AM-handler is located, there is no static functions at all... gist.c has, but I think I should write similar code for all *_private.h indexes. So I think it wold be good to hide catalog function via *_pricate.h include file for these three indexes. hash.c is quite a mess... There is no hash_private.h, AM-handles is located in hash.c, that has "This file contains only the public interface routines." comment at the beginning, and there is no static functions inside. I do not know what is the right way to hide hashgetreloptcatalog function here... What would you advise? -- Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire. -- 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] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
В письме от 26 марта 2017 15:02:12 Вы написали: > Nikolay Shaplov wrote: > > If I would think about it now: we always know how many options we will > > have. So we can just pass this number to palloc and assert if somebody > > adds more options then expected... What do yo think about it. > > I think we need to preserve the ability to add custom options, because > extensions may want to do that. Ok. At least this will not do any harm :-) -- Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire. -- 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] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
В письме от 23 марта 2017 16:14:58 пользователь Fabrízio de Royes Mello написал: > On Thu, Mar 23, 2017 at 3:58 PM, Alvaro Herrera > > wrote: > > Copying Fabrízio Mello here, who spent some time trying to work on > > reloptions too. He may have something to say about the new > > functionality that this patch provides. > > Thanks Álvaro, I'll look the patch and try to help in some way. Thank you, that would be great! -- Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire. -- 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] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
Nikolay Shaplov wrote: > If I would think about it now: we always know how many options we will have. > So we can just pass this number to palloc and assert if somebody adds more > options then expected... What do yo think about it. I think we need to preserve the ability to add custom options, because extensions may want to do that. -- Álvaro Herrerahttps://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] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
В письме от 17 марта 2017 14:21:26 пользователь Alvaro Herrera написал: > I gave this patch a quick skim. Thanks! > At first I was confused by the term > "catalog"; I thought it meant we stored options in a system table. But > that's not what is meant at all; instead, what we do is build these > "catalogs" in memory. Maybe a different term could have been used, but > I'm not sure it's stricly necessary. I wouldn't really bother. Yes "catalog" is quite confusing. I did not find better name while I was writing the code, so I take first one, that came into my mind. If you, or somebody else, have better idea how to call this sets of options definitions I will gladly rename it, as catalog is a bad name. May be OptionsDefSet instead of OptionsCatalog? > I'm confused by the "no ALTER, no lock" rule. Does it mean that if > "ALTER..SET" is forbidden? Because I noticed that brin's > pages_per_range is marked as such, but we do allow that option to change > with ALTER..SET, so there's at least one bug there, and I would be > surprised if there aren't any others. If you grep, for example, gist index code for "buffering_mode" option, you will see, that this option is used only in gistbuild.c. There it is written into the meta page, and afterwards, value from meta page is used, and one from options, is just ignored. Nowdays you can successfully alter this value, but this will not affect anything until index is recreated... I thought it is very confusing behavior and decided that we should just forbid such alters. > Please make sure to mark functions as static (e.g. bringetreloptcatalog). Ok. Will do. > Why not pass ->struct_size and ->postprocess_fun (which I'd rename it as > ->postprocess_fn, more in line with our naming style) as a parameter to > allocateOptionsCatalog? struct_size -- very good idea! postprocess_fn -- now it is rarely used. In most cases it is NULL. May be it would be ok to set it afterwards in that rare cases when it is needed. > Also, to avoid repalloc() in most cases (and to > avoid pallocing more options that you're going to need in a bunch of > cases, perhaps that function should the number of times you expect to > call AddItems for that catalog (since you do it immediately afterwards > in all cases I can see), and allocate that number. If later another > item arrives, then repalloc using the same code you already have in > AddItems(). I've copied this code from reloptions code for custom options. Without much thinking about it. If I would think about it now: we always know how many options we will have. So we can just pass this number to palloc and assert if somebody adds more options then expected... What do yo think about it. > Something is wrong with leading whitespace in many places; either you > added too many tabs, or the wrong number spaces; not sure which but > visually it's clearly wrong. ... Actually there are whitespace-style > violations in several places; please fix using pgindent (after adding > any new typedefs your defining to typedefs.list). I will run pgindent on my code. -- Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire. -- 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] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
On Thu, Mar 23, 2017 at 3:58 PM, Alvaro Herrera wrote: > > Copying Fabrízio Mello here, who spent some time trying to work on > reloptions too. He may have something to say about the new > functionality that this patch provides. > Thanks Álvaro, I'll look the patch and try to help in some way. Regards, -- Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Re: [HACKERS] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
Copying Fabrízio Mello here, who spent some time trying to work on reloptions too. He may have something to say about the new functionality that this patch provides. -- Álvaro Herrerahttps://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] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
By the way, it would be very good if you could review some patches, too. -- Álvaro Herrerahttps://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] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
I gave this patch a quick skim. At first I was confused by the term "catalog"; I thought it meant we stored options in a system table. But that's not what is meant at all; instead, what we do is build these "catalogs" in memory. Maybe a different term could have been used, but I'm not sure it's stricly necessary. I wouldn't really bother. I'm confused by the "no ALTER, no lock" rule. Does it mean that if "ALTER..SET" is forbidden? Because I noticed that brin's pages_per_range is marked as such, but we do allow that option to change with ALTER..SET, so there's at least one bug there, and I would be surprised if there aren't any others. Please make sure to mark functions as static (e.g. bringetreloptcatalog). Why not pass ->struct_size and ->postprocess_fun (which I'd rename it as ->postprocess_fn, more in line with our naming style) as a parameter to allocateOptionsCatalog? Also, to avoid repalloc() in most cases (and to avoid pallocing more options that you're going to need in a bunch of cases, perhaps that function should the number of times you expect to call AddItems for that catalog (since you do it immediately afterwards in all cases I can see), and allocate that number. If later another item arrives, then repalloc using the same code you already have in AddItems(). Something is wrong with leading whitespace in many places; either you added too many tabs, or the wrong number spaces; not sure which but visually it's clearly wrong. ... Actually there are whitespace-style violations in several places; please fix using pgindent (after adding any new typedefs your defining to typedefs.list). -- Álvaro Herrerahttps://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
