Thread: Re: [PATCH] Do not use StdRdOptions in Access Methods
Hi! I am starting a new thread as commitfest wants new thread for new patch. This new thread is a follow up to an https://www.postgresql.org/message-id/ 2620882.s52SJui4ql%40x200m thread, where I've been trying to get rid of StdRdOpions structure, and now I've splitted the patch into smaller parts. Read the quote below, to get what this patch is about > I've been thinking about this patch and came to a conclusion that it > would be better to split it to even smaller parts, so they can be > easily reviewed, one by one. May be even leaving most complex > Heap/Toast part for later. > > This is first part of this patch. Here we give each Access Method it's > own option binary representation instead of StdRdOptions. > > I think this change is obvious. Because, first, Access Methods do not > use most of the values defined in StdRdOptions. > > Second this patch make Options structure code unified for all core > Access Methods. Before some AM used StdRdOptions, some AM used it's own > structure, now all AM uses own structures and code is written in the > same style, so it would be more easy to update it in future. > > John Dent, would you join reviewing this part of the patch? I hope it > will be more easy now... And now I have a newer version of the patch, as I forgot to remove vacuum_cleanup_index_scale_factor from StdRdOptions as it was used only in Btree index and now do not used at all. New version fixes it. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Attachment
Hi Nikolay, I like the new approach. And I agree with the ambition — to split out the representation from StdRdOptions. However, with that change, in the AM’s *options() function, it looks as if you could simply add new fields to the relopt_parse_eltlist. That’s still not true, because parseRelOptions() will fail to find a matching entry, causing numoptionsto be left zero, and an early exit made. (At least, that’s if I correctly understand how things work.) I think that is fine as an interim limitation, because your change has not yet fully broken the connection to the boolRelOpts,intRelOpts, realRelOpts and stringRelOpts strutures. But perhaps a comment would help to make it clear. Perhapsadd something like this above the tab[]: "When adding or changing a relopt in the relopt_parse_elt tab[], ensure thecorresponding change is made to boolRelOpts, intRelOpts, realRelOpts or stringRelOpts." denty. > On 6 Oct 2019, at 14:45, Nikolay Shaplov <dhyan@nataraj.su> wrote: > > Hi! I am starting a new thread as commitfest wants new thread for new patch. > > This new thread is a follow up to an https://www.postgresql.org/message-id/ > 2620882.s52SJui4ql%40x200m thread, where I've been trying to get rid of > StdRdOpions structure, and now I've splitted the patch into smaller parts. > > Read the quote below, to get what this patch is about > >> I've been thinking about this patch and came to a conclusion that it >> would be better to split it to even smaller parts, so they can be >> easily reviewed, one by one. May be even leaving most complex >> Heap/Toast part for later. >> >> This is first part of this patch. Here we give each Access Method it's >> own option binary representation instead of StdRdOptions. >> >> I think this change is obvious. Because, first, Access Methods do not >> use most of the values defined in StdRdOptions. >> >> Second this patch make Options structure code unified for all core >> Access Methods. Before some AM used StdRdOptions, some AM used it's own >> structure, now all AM uses own structures and code is written in the >> same style, so it would be more easy to update it in future. >> >> John Dent, would you join reviewing this part of the patch? I hope it >> will be more easy now... > > And now I have a newer version of the patch, as I forgot to remove > vacuum_cleanup_index_scale_factor from StdRdOptions as it was used only in > Btree index and now do not used at all. New version fixes it. > > -- > Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da > Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)<do-not-use-StdRdOptions-in-AM_2.diff>
В письме от понедельник, 7 октября 2019 г. 18:55:20 MSK пользователь Dent John написал: > I like the new approach. And I agree with the ambition — to split out the > representation from StdRdOptions. Thanks. > However, with that change, in the AM’s *options() function, it looks as if > you could simply add new fields to the relopt_parse_elt list. That’s still > not true, because parseRelOptions() will fail to find a matching entry, > causing numoptions to be left zero, and an early exit made. (At least, > that’s if I correctly understand how things work.) > > I think that is fine as an interim limitation, because your change has not > yet fully broken the connection to the boolRelOpts, intRelOpts, realRelOpts > and stringRelOpts strutures. But perhaps a comment would help to make it > clear. Perhaps add something like this above the tab[]: "When adding or > changing a relopt in the relopt_parse_elt tab[], ensure the corresponding > change is made to boolRelOpts, intRelOpts, realRelOpts or stringRelOpts." Whoa-whoa! I am not inventing something new here. Same code is already used in brin (brin.c:820), gin (ginutils.c:602) and gist (gistutils.c:909) indexes. I've just copied the idea, to do all index code uniform. This does not mean that these code can't be improved, but as far as I can guess it is better to do it in small steps, first make option code uniform, and then improve all of it this way or another... So I here I would suggest to discuss it I copied this code correctly, without going very deeply into discussion how we can improve code I've used as a source for cloning. And then I have ideas how to do it better. But this will come later... -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
> On 8 Oct 2019, at 11:33, Nikolay Shaplov <dhyan@nataraj.su> wrote: > > Whoa-whoa! > > I am not inventing something new here. Same code is already used in brin > (brin.c:820), gin (ginutils.c:602) and gist (gistutils.c:909) indexes. I've > just copied the idea, to do all index code uniform. > > This does not mean that these code can't be improved, but as far as I can > guess it is better to do it in small steps, first make option code uniform, and > then improve all of it this way or another... I didn’t spot it was an existing pattern. And I agree — making the code uniform will make it easier to evolve in future. Gets my vote. denty.
В письме от среда, 9 октября 2019 г. 20:26:14 MSK пользователь Dent John написал: > I didn’t spot it was an existing pattern. Sorry, this might be my mistake I should explicitly mention it in the first place. > And I agree — making the code uniform will make it easier to evolve in > future. > > Gets my vote. Thanks! Can you please check, I did not do any mistake while copying and adapting the code. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Hello Nikolay, I read comments that Tomas left at: https://www.postgresql.org/message-id/20190727173841.7ypzo4xuzizvijge%40development I'd like to join Michael in reiterating one point from Tomas' review. I think the patch can go further in trying to make the code in this area more maintainable. For example, even without this patch, the following stanza is repeated in many places: options = parseRelOptions(reloptions, validate, foo_relopt_kind, &numoptions); rdopts = allocateReloptStruct(sizeof(FooOptions), options, numoptions); fillRelOptions((void *) rdopts, sizeof(FooOptions), options, numoptions, validate, foo_relopt_tab, lengthof(foo_relopt_tab)); return (bytea *) rdopts; and this patch adds few more instances as it's adding more Options structs. I think it wouldn't be hard to encapsulate the above stanza in a new public function in reloptions.c and call it from the various places that now have the above code. Thanks, Amit
В письме от четверг, 10 октября 2019 г. 17:17:30 MSK пользователь Amit Langote написал: > I read comments that Tomas left at: > https://www.postgresql.org/message-id/20190727173841.7ypzo4xuzizvijge%40deve > lopment > > I'd like to join Michael in reiterating one point from Tomas' review. > I think the patch can go further in trying to make the code in this > area more maintainable. > > For example, even without this patch, the following stanza is repeated > in many places: > > options = parseRelOptions(reloptions, validate, foo_relopt_kind, > &numoptions); > rdopts = allocateReloptStruct(sizeof(FooOptions), options, numoptions); > fillRelOptions((void *) rdopts, sizeof(FooOptions), options, numoptions, > validate, foo_relopt_tab, lengthof(foo_relopt_tab)); return (bytea *) > rdopts; > > and this patch adds few more instances as it's adding more Options structs. > > I think it wouldn't be hard to encapsulate the above stanza in a new > public function in reloptions.c and call it from the various places > that now have the above code. The code of reloptions is very historical and illogical. I also noticed that these lines are repeated several time. And may be it would be better to put them into reloptions.c. But could anybody clearly explain what are they doing? Just to give function a proper name. I understand what they are doing, but I am unable to give short and clear explanation. I am planning to rewrite this part completely. So we have none of this lines repeated. I had a proposal you can see it here https:// commitfest.postgresql.org/15/992/ but people on the list told be that patch is too complex and I should commit it part by part. So I am doing it now. I am almost done. But to provide clear and logical patch that introduces my concept, I need StdRdOption to be divided into separated structures. At least for AM. And I need at to be done as simply as possible because the rest of the code is going to be rewritten anyway. That is why I want to follow the steps: make the code uniform, and then improve it. I have improvement in the pocket, but I need a uniform code before revealing it. If you think it is absolutely necessary to put these line into one function, I will do it. It will not make code more clear, I guess. I See no benefits, but I can do it, but I would avoid doing it, if possible. At least at this step. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Hi Nikolay, Sorry for the late reply. On Fri, Oct 11, 2019 at 7:54 PM Nikolay Shaplov <dhyan@nataraj.su> wrote: > В письме от четверг, 10 октября 2019 г. 17:17:30 MSK пользователь Amit Langote > написал: > > I read comments that Tomas left at: > > https://www.postgresql.org/message-id/20190727173841.7ypzo4xuzizvijge%40deve > > lopment > > > > I'd like to join Michael in reiterating one point from Tomas' review. > > I think the patch can go further in trying to make the code in this > > area more maintainable. > > > > For example, even without this patch, the following stanza is repeated > > in many places: > > > > options = parseRelOptions(reloptions, validate, foo_relopt_kind, > > &numoptions); > > rdopts = allocateReloptStruct(sizeof(FooOptions), options, numoptions); > > fillRelOptions((void *) rdopts, sizeof(FooOptions), options, numoptions, > > validate, foo_relopt_tab, lengthof(foo_relopt_tab)); return (bytea *) > > rdopts; > > > > and this patch adds few more instances as it's adding more Options structs. > > > > I think it wouldn't be hard to encapsulate the above stanza in a new > > public function in reloptions.c and call it from the various places > > that now have the above code. > > The code of reloptions is very historical and illogical. I also noticed that > these lines are repeated several time. And may be it would be better to put > them into reloptions.c. But could anybody clearly explain what are they doing? > Just to give function a proper name. I understand what they are doing, but I > am unable to give short and clear explanation. Maybe call it BuildRelOptions(), which takes in reloptions in the text array format and returns a struct whose size is specified by the caller. See the attached patch. > I am planning to rewrite this part completely. So we have none of this lines > repeated. I had a proposal you can see it here https:// > commitfest.postgresql.org/15/992/ but people on the list told be that patch is > too complex and I should commit it part by part. > > So I am doing it now. I am almost done. But to provide clear and logical patch > that introduces my concept, I need StdRdOption to be divided into separated > structures. At least for AM. And I need at to be done as simply as possible > because the rest of the code is going to be rewritten anyway. > > That is why I want to follow the steps: make the code uniform, and then > improve it. I have improvement in the pocket, but I need a uniform code before > revealing it. > > If you think it is absolutely necessary to put these line into one function, I > will do it. It will not make code more clear, I guess. I See no benefits, but > I can do it, but I would avoid doing it, if possible. At least at this step. IMO, parts of the patch that only refactors the existing code should be first in the list as it is easier to review, especially if it adds no new concepts. In this case, your patch to break StdRdOptions into more manageable chunks would be easier to understand if it built upon a simplified framework of parsing reloptions text arrays. Thanks, Amit
Attachment
On Wed, Oct 23, 2019 at 11:16:25AM +0900, Amit Langote wrote: > IMO, parts of the patch that only refactors the existing code should > be first in the list as it is easier to review, especially if it adds > no new concepts. In this case, your patch to break StdRdOptions into > more manageable chunks would be easier to understand if it built upon > a simplified framework of parsing reloptions text arrays. Thanks for doing a split. This helps in proving the point that this portion has independent value. s/BuildRelOptions/buildRelOptions/ for consistency with the other routines (see first character's case-ing)? +/* + * Using parseRelOptions(), allocateReloptStruct(), and fillRelOptions() + * directly is Deprecated; use BuildRelOptions() instead. + */ extern relopt_value *parseRelOptions(Datum options, bool validate, Compatibility is surely a concern for existing extensions, but that's not the first interface related to reloptions that we'd break in this release (/me whistles). So my take would be to move all the past routines to be static and only within reloptions.c, and just publish the new one. That's by far not the most popular API we provide. + /* + * Allocate and fill the struct. Caller-specified struct size and the + * relopt_parse_elt table (relopt_elems + num_relopt_elems) must match. + */ The comment should be about a multiplication, no? It seems to me that an assertion would be appropriate here then to insist on the relationship between all that, and also it would be nice to document better what's expected from the caller of the new routine regarding all the arguments needed. In short, what's expected of relopt_struct_size, relopt_elems and num_relopt_elems? -- Michael
Attachment
Hi Michael, Thanks for taking a look at this. On Wed, Oct 23, 2019 at 12:51 PM Michael Paquier <michael@paquier.xyz> wrote: > On Wed, Oct 23, 2019 at 11:16:25AM +0900, Amit Langote wrote: > > IMO, parts of the patch that only refactors the existing code should > > be first in the list as it is easier to review, especially if it adds > > no new concepts. In this case, your patch to break StdRdOptions into > > more manageable chunks would be easier to understand if it built upon > > a simplified framework of parsing reloptions text arrays. > > Thanks for doing a split. This helps in proving the point that this > portion has independent value. > > s/BuildRelOptions/buildRelOptions/ for consistency with the other > routines (see first character's case-ing)? Hmm, if we're inventing a new API to replace the old one, why not use that opportunity to be consistent with our general style, which predominantly seems to be either words_separated_by_underscore() or UpperCamelCase(). Thoughts? > +/* > + * Using parseRelOptions(), allocateReloptStruct(), and fillRelOptions() > + * directly is Deprecated; use BuildRelOptions() instead. > + */ > extern relopt_value *parseRelOptions(Datum options, bool validate, > Compatibility is surely a concern for existing extensions, but that's > not the first interface related to reloptions that we'd break in this > release (/me whistles). So my take would be to move all the past > routines to be static and only within reloptions.c, and just publish > the new one. That's by far not the most popular API we provide. OK, done. > + /* > + * Allocate and fill the struct. Caller-specified struct size and the > + * relopt_parse_elt table (relopt_elems + num_relopt_elems) must match. > + */ > The comment should be about a multiplication, no? I didn't really mean to specify any mathematical operation by the "+" in that comment, but I can see how it's confusing. :) > It seems to me that > an assertion would be appropriate here then to insist on the > relationship between all that, and also it would be nice to document > better what's expected from the caller of the new routine regarding > all the arguments needed. In short, what's expected of > relopt_struct_size, relopt_elems and num_relopt_elems? You might know already, but in short, the values in the passed-in relopt_parse_elts array (relopt_elems) must fit within relopt_struct_size. Writing an Assert turned out to be tricky given that alignment must be considered, but I have tried to add one. Pleas check, it very well might be wrong. ;) Attached updated patch. It would be nice to hear whether this patch is really what Nikolay intended to eventually do with this code. Thanks, Amit
Attachment
On Fri, Oct 25, 2019 at 04:42:24PM +0900, Amit Langote wrote: > Hmm, if we're inventing a new API to replace the old one, why not use > that opportunity to be consistent with our general style, which > predominantly seems to be either words_separated_by_underscore() or > UpperCamelCase(). Thoughts? Not wrong. Using small-case characters separated with underscores would be more consistent with the rest perhaps? We use that for the initialization of custom variables and for all the relkind-related interfaces. > You might know already, but in short, the values in the passed-in > relopt_parse_elts array (relopt_elems) must fit within > relopt_struct_size. Writing an Assert turned out to be tricky given > that alignment must be considered, but I have tried to add one. Pleas > check, it very well might be wrong. ;) Hmm. I didn't expect it to be this confusing with relopt_type_size[]. I'll try to think about something :( + * Parses reloptions provided by the caller in text array format and + * fills and returns a struct containing the parsed option values The sentence structure is weird, perhaps: This routine parses "reloptions" provided by the caller in text-array format. The parsing is done with a table describing the allowed options, defined by "relopt_elems" of length "num_relopt_elems". The returned result is a structure containing all the parsed option values. > Attached updated patch. It would be nice to hear whether this patch > is really what Nikolay intended to eventually do with this code. Okay, let's check if Nikolay likes this idea. -- Michael
Attachment
On Sat, Oct 26, 2019 at 11:45 AM Michael Paquier <michael@paquier.xyz> wrote: > On Fri, Oct 25, 2019 at 04:42:24PM +0900, Amit Langote wrote: > > Hmm, if we're inventing a new API to replace the old one, why not use > > that opportunity to be consistent with our general style, which > > predominantly seems to be either words_separated_by_underscore() or > > UpperCamelCase(). Thoughts? > > Not wrong. Using small-case characters separated with underscores > would be more consistent with the rest perhaps? We use that for the > initialization of custom variables and for all the relkind-related > interfaces. OK, I went with build_reloptions(), which looks very similar to nearby exported functions. > + * Parses reloptions provided by the caller in text array format and > + * fills and returns a struct containing the parsed option values > The sentence structure is weird, perhaps: > This routine parses "reloptions" provided by the caller in text-array > format. The parsing is done with a table describing the allowed > options, defined by "relopt_elems" of length "num_relopt_elems". The > returned result is a structure containing all the parsed option > values. Thanks, I have expanded the header comment based on your text. > > Attached updated patch. It would be nice to hear whether this patch > > is really what Nikolay intended to eventually do with this code. > > Okay, let's check if Nikolay likes this idea. Attached updated patch. Thanks, Amit
Attachment
On 2019-Oct-23, Michael Paquier wrote: > On Wed, Oct 23, 2019 at 11:16:25AM +0900, Amit Langote wrote: > > IMO, parts of the patch that only refactors the existing code should > > be first in the list as it is easier to review, especially if it adds > > no new concepts. In this case, your patch to break StdRdOptions into > > more manageable chunks would be easier to understand if it built upon > > a simplified framework of parsing reloptions text arrays. > > Thanks for doing a split. This helps in proving the point that this > portion has independent value. Not a split, yes? AFAICS this code is nowhere in Nikolay's proposed patchset -- it seems completely new development by Amit. Am I wrong? I also think that this has value -- let's go for it. I think I'll be back on Wednesday to review it, if you would prefer to wait. Thanks -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Oct 28, 2019 at 12:02:20PM -0300, Alvaro Herrera wrote: > I also think that this has value -- let's go for it. I think I'll be > back on Wednesday to review it, if you would prefer to wait. No worries, thanks for looking it. -- Michael
Attachment
Hi Alvaro, On Tue, Oct 29, 2019 at 12:02 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2019-Oct-23, Michael Paquier wrote: > > On Wed, Oct 23, 2019 at 11:16:25AM +0900, Amit Langote wrote: > > > IMO, parts of the patch that only refactors the existing code should > > > be first in the list as it is easier to review, especially if it adds > > > no new concepts. In this case, your patch to break StdRdOptions into > > > more manageable chunks would be easier to understand if it built upon > > > a simplified framework of parsing reloptions text arrays. > > > > Thanks for doing a split. This helps in proving the point that this > > portion has independent value. > > Not a split, yes? AFAICS this code is nowhere in Nikolay's proposed > patchset -- it seems completely new development by Amit. Am I wrong? IIUC, Nikolay intended to write such a patch but only after getting some consensus on breaking up StdRdOptions. I didn't look closely but an idea similar to the patch I posted (really as a PoC) might have been discussed couple of years ago, as Nikolay mentioned upthread: https://commitfest.postgresql.org/15/992/ Thanks, Amit
On Mon, Oct 28, 2019 at 05:16:54PM +0900, Amit Langote wrote: > On Sat, Oct 26, 2019 at 11:45 AM Michael Paquier <michael@paquier.xyz> wrote: >> On Fri, Oct 25, 2019 at 04:42:24PM +0900, Amit Langote wrote: >> > Hmm, if we're inventing a new API to replace the old one, why not use >> > that opportunity to be consistent with our general style, which >> > predominantly seems to be either words_separated_by_underscore() or >> > UpperCamelCase(). Thoughts? >> >> Not wrong. Using small-case characters separated with underscores >> would be more consistent with the rest perhaps? We use that for the >> initialization of custom variables and for all the relkind-related >> interfaces. > > OK, I went with build_reloptions(), which looks very similar to nearby > exported functions. Thanks. >> + * Parses reloptions provided by the caller in text array format and >> + * fills and returns a struct containing the parsed option values >> The sentence structure is weird, perhaps: >> This routine parses "reloptions" provided by the caller in text-array >> format. The parsing is done with a table describing the allowed >> options, defined by "relopt_elems" of length "num_relopt_elems". The >> returned result is a structure containing all the parsed option >> values. > > Thanks, I have expanded the header comment based on your text. Looks fine. I have done some refinements as per the attached. Running the regression tests of dummy_index_am has proved to be able to break the assertion you have added. I don't have a good idea of how to make that more simple and reliable, but there is one thing outstanding here: the number of options parsed by parseRelOptions should never be higher than num_relopt_elems. So let's at least be safer about that. Also if some options are parsed options will never be NULL, so there is no need to check for it before pfree()-ing it, no? Any comments from others? Alvaro perhaps? -- Michael
Attachment
Hi Michael,
On Wed, Oct 30, 2019 at 12:11 PM Michael Paquier <michael@paquier.xyz> wrote:
> Looks fine. I have done some refinements as per the attached.
Thanks. This stood out to me:
+ * The result is a structure containing all the parsed option values in
+ * text-array format.
This sentence sounds wrong, because the result structure doesn't
contain values in text-array format. Individual values in the struct
would be in their native format (C bool for RELOPT_TYPE_BOOL, options,
etc.).
Maybe we don't need this sentence, because the first line already says
we return a struct.
> Running the regression tests of dummy_index_am has proved to be able
> to break the assertion you have added.
This breakage seems to have to do with the fact that the definition of
DummyIndexOptions struct and the entries of relopt_parse_elt table
don't agree?
These are the last two members of DummyIndexOptions struct:
int option_string_val_offset;
int option_string_null_offset;
} DummyIndexOptions;
but di_relopt_tab's last two entries are these:
add_string_reloption(di_relopt_kind, "option_string_val",
"String option for dummy_index_am with
non-NULL default",
"DefaultValue", &validate_string_option,
AccessExclusiveLock);
di_relopt_tab[4].optname = "option_string_val";
di_relopt_tab[4].opttype = RELOPT_TYPE_STRING;
di_relopt_tab[4].offset = offsetof(DummyIndexOptions,
option_string_val_offset);
/*
* String option for dummy_index_am with NULL default, and without
* description.
*/
add_string_reloption(di_relopt_kind, "option_string_null",
NULL, /* description */
NULL, &validate_string_option,
AccessExclusiveLock);
di_relopt_tab[5].optname = "option_string_null";
di_relopt_tab[5].opttype = RELOPT_TYPE_STRING;
di_relopt_tab[5].offset = offsetof(DummyIndexOptions,
option_string_null_offset);
If I fix the above code like this:
@@ -113,7 +113,7 @@ create_reloptions_table(void)
"DefaultValue", &validate_string_option,
AccessExclusiveLock);
di_relopt_tab[4].optname = "option_string_val";
- di_relopt_tab[4].opttype = RELOPT_TYPE_STRING;
+ di_relopt_tab[4].opttype = RELOPT_TYPE_INT;
di_relopt_tab[4].offset = offsetof(DummyIndexOptions,
option_string_val_offset);
@@ -126,7 +126,7 @@ create_reloptions_table(void)
NULL, &validate_string_option,
AccessExclusiveLock);
di_relopt_tab[5].optname = "option_string_null";
- di_relopt_tab[5].opttype = RELOPT_TYPE_STRING;
+ di_relopt_tab[5].opttype = RELOPT_TYPE_INT;
di_relopt_tab[5].offset = offsetof(DummyIndexOptions,
option_string_null_offset);
}
test passes.
But maybe this Assert isn't all that robust, so I'm happy to take it out.
> Also if some options are parsed options will never be NULL, so there
> is no need to check for it before pfree()-ing it, no?
I haven't fully read parseRelOptions(), but I will trust you. :)
Thanks,
Amit
On Thu, Oct 31, 2019 at 04:38:55PM +0900, Amit Langote wrote: > On Wed, Oct 30, 2019 at 12:11 PM Michael Paquier <michael@paquier.xyz> wrote: > This sentence sounds wrong, because the result structure doesn't > contain values in text-array format. Individual values in the struct > would be in their native format (C bool for RELOPT_TYPE_BOOL, options, > etc.). > > Maybe we don't need this sentence, because the first line already says > we return a struct. Let's remove it then. > This breakage seems to have to do with the fact that the definition of > DummyIndexOptions struct and the entries of relopt_parse_elt table > don't agree? > > These are the last two members of DummyIndexOptions struct: > > @@ -126,7 +126,7 @@ create_reloptions_table(void) > NULL, &validate_string_option, > AccessExclusiveLock); > di_relopt_tab[5].optname = "option_string_null"; > - di_relopt_tab[5].opttype = RELOPT_TYPE_STRING; > + di_relopt_tab[5].opttype = RELOPT_TYPE_INT; > di_relopt_tab[5].offset = offsetof(DummyIndexOptions, > option_string_null_offset); > } > > test passes. > > But maybe this Assert isn't all that robust, so I'm happy to take it out. This one should remain a string reloption, and what's stored in the structure is an offset to get the string. See for example around RelationHasCascadedCheckOption before it got switched to an enum in 773df88 regarding the way to get the value. -- Michael
Attachment
On Thu, Oct 31, 2019 at 4:49 PM Michael Paquier <michael@paquier.xyz> wrote: > On Thu, Oct 31, 2019 at 04:38:55PM +0900, Amit Langote wrote: > > On Wed, Oct 30, 2019 at 12:11 PM Michael Paquier <michael@paquier.xyz> wrote: > > This sentence sounds wrong, because the result structure doesn't > > contain values in text-array format. Individual values in the struct > > would be in their native format (C bool for RELOPT_TYPE_BOOL, options, > > etc.). > > > > Maybe we don't need this sentence, because the first line already says > > we return a struct. > > Let's remove it then. Removed in the attached. > > This breakage seems to have to do with the fact that the definition of > > DummyIndexOptions struct and the entries of relopt_parse_elt table > > don't agree? > > > > These are the last two members of DummyIndexOptions struct: > > > > @@ -126,7 +126,7 @@ create_reloptions_table(void) > > NULL, &validate_string_option, > > AccessExclusiveLock); > > di_relopt_tab[5].optname = "option_string_null"; > > - di_relopt_tab[5].opttype = RELOPT_TYPE_STRING; > > + di_relopt_tab[5].opttype = RELOPT_TYPE_INT; > > di_relopt_tab[5].offset = offsetof(DummyIndexOptions, > > option_string_null_offset); > > } > > > > test passes. > > > > But maybe this Assert isn't all that robust, so I'm happy to take it out. > > This one should remain a string reloption, and what's stored in the > structure is an offset to get the string. See for example around > RelationHasCascadedCheckOption before it got switched to an enum in > 773df88 regarding the way to get the value. I see. I didn't know that about STRING options when writing that Assert, so it was indeed broken to begin with. v5 attached. Thanks, Amit
Attachment
On Thu, Oct 31, 2019 at 05:18:46PM +0900, Amit Langote wrote: > On Thu, Oct 31, 2019 at 4:49 PM Michael Paquier <michael@paquier.xyz> wrote: >> Let's remove it then. > > Removed in the attached. Thanks. I exactly did the same thing on my local branch. -- Michael
Attachment
On Thu, Oct 31, 2019 at 05:55:55PM +0900, Michael Paquier wrote: > Thanks. I exactly did the same thing on my local branch. Hearing nothing more, I have done some adjustments to the patch and committed it. Please note that I have not switched the old interface to be static to reloptions.c as if you look closely at reloptions.h we allow much more flexibility around HANDLE_INT_RELOPTION to fill and parse the reloptions in custom AM. AM maintainers had better use the new interface, but there could be a point for more customized error messages. -- Michael
Attachment
On Tue, Nov 5, 2019 at 9:22 AM Michael Paquier <michael@paquier.xyz> wrote: > On Thu, Oct 31, 2019 at 05:55:55PM +0900, Michael Paquier wrote: > > Thanks. I exactly did the same thing on my local branch. > > Hearing nothing more, I have done some adjustments to the patch and > committed it. Thank you. > Please note that I have not switched the old interface > to be static to reloptions.c as if you look closely at reloptions.h we > allow much more flexibility around HANDLE_INT_RELOPTION to fill and > parse the reloptions in custom AM. AM maintainers had better use the > new interface, but there could be a point for more customized error > messages. I looked around but don't understand why these macros need to be exposed. I read this comment: * Note that this is more or less the same that fillRelOptions does, so only * use this if you need to do something non-standard within some option's * code block. but don't see how an AM author might be able to do something non-standard with this interface. Maybe Alvaro knows this better. Thanks, Amit
On Thu, Nov 07, 2019 at 10:49:38AM +0900, Amit Langote wrote: > I looked around but don't understand why these macros need to be > exposed. I read this comment: > > * Note that this is more or less the same that fillRelOptions does, so only > * use this if you need to do something non-standard within some option's > * code block. > > but don't see how an AM author might be able to do something > non-standard with this interface. > > Maybe Alvaro knows this better. Perhaps there is a point in cleaning up all that more, but I am not sure that it is worth potentially breaking other people's code. -- Michael
Attachment
On Thu, Nov 7, 2019 at 10:54 AM Michael Paquier <michael@paquier.xyz> wrote: > On Thu, Nov 07, 2019 at 10:49:38AM +0900, Amit Langote wrote: > > I looked around but don't understand why these macros need to be > > exposed. I read this comment: > > > > * Note that this is more or less the same that fillRelOptions does, so only > > * use this if you need to do something non-standard within some option's > > * code block. > > > > but don't see how an AM author might be able to do something > > non-standard with this interface. > > > > Maybe Alvaro knows this better. > > Perhaps there is a point in cleaning up all that more, but I am not > sure that it is worth potentially breaking other people's code. Sure. Maybe, we could add a deprecation note for these more fine-grained APIs like my first patch did. +/* + * Using parseRelOptions(), allocateReloptStruct(), and fillRelOptions() + * directly is Deprecated; use build_reloptions() instead. + */ Thanks, Amit
On 2019-Nov-07, Amit Langote wrote: > On Tue, Nov 5, 2019 at 9:22 AM Michael Paquier <michael@paquier.xyz> wrote: > > Please note that I have not switched the old interface > > to be static to reloptions.c as if you look closely at reloptions.h we > > allow much more flexibility around HANDLE_INT_RELOPTION to fill and > > parse the reloptions in custom AM. AM maintainers had better use the > > new interface, but there could be a point for more customized error > > messages. > > I looked around but don't understand why these macros need to be > exposed. I read this comment: > > * Note that this is more or less the same that fillRelOptions does, so only > * use this if you need to do something non-standard within some option's > * code block. > > but don't see how an AM author might be able to do something > non-standard with this interface. > > Maybe Alvaro knows this better. I think the idea was that you could have external code doing things in a different way for some reason, ie. not use a bytea varlena struct that could be filled by fillRelOptions but instead ... do something else. That's why those macros are exposed. Now, this idea doesn't seem to be aged very well. Maybe exposing that stuff is unnecessary. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Alvaro, On Wed, Nov 13, 2019 at 6:55 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2019-Nov-07, Amit Langote wrote: > > On Tue, Nov 5, 2019 at 9:22 AM Michael Paquier <michael@paquier.xyz> wrote: > > > Please note that I have not switched the old interface > > > to be static to reloptions.c as if you look closely at reloptions.h we > > > allow much more flexibility around HANDLE_INT_RELOPTION to fill and > > > parse the reloptions in custom AM. AM maintainers had better use the > > > new interface, but there could be a point for more customized error > > > messages. > > > > I looked around but don't understand why these macros need to be > > exposed. I read this comment: > > > > * Note that this is more or less the same that fillRelOptions does, so only > > * use this if you need to do something non-standard within some option's > > * code block. > > > > but don't see how an AM author might be able to do something > > non-standard with this interface. > > > > Maybe Alvaro knows this better. > > I think the idea was that you could have external code doing things in a > different way for some reason, ie. not use a bytea varlena struct that > could be filled by fillRelOptions but instead ... do something else. > That's why those macros are exposed. Now, this idea doesn't seem to be > aged very well. Maybe exposing that stuff is unnecessary. Thanks for chiming in about that. I guess that means that we don't need those macros, except GET_STRING_RELOPTION_LEN() that's used in allocateReloptStruct(), which can be moved to reloptions.c. Is that correct? Thanks, Amit
On Wed, Nov 13, 2019 at 10:52:52AM +0900, Amit Langote wrote: > Thanks for chiming in about that. I guess that means that we don't > need those macros, except GET_STRING_RELOPTION_LEN() that's used in > allocateReloptStruct(), which can be moved to reloptions.c. Is that > correct? I have been looking on the net to see if there are any traces of code using those macros, but could not find any. The last trace of a macro use is in 8ebe1e3, which just relies on GET_STRING_RELOPTION_LEN. So it looks rather convincing now to just remove this code. Attached is a patch for that. There could be an argument for keeping GET_STRING_RELOPTION actually which is still useful to get a string value in an option set using the stored offset, and we have the recently-added dummy_index_am in this category. Any thoughts? -- Michael
Attachment
On Wed, Nov 13, 2019 at 2:18 PM Michael Paquier <michael@paquier.xyz> wrote: > On Wed, Nov 13, 2019 at 10:52:52AM +0900, Amit Langote wrote: > > Thanks for chiming in about that. I guess that means that we don't > > need those macros, except GET_STRING_RELOPTION_LEN() that's used in > > allocateReloptStruct(), which can be moved to reloptions.c. Is that > > correct? > > I have been looking on the net to see if there are any traces of code > using those macros, but could not find any. The last trace of a macro > use is in 8ebe1e3, which just relies on GET_STRING_RELOPTION_LEN. So > it looks rather convincing now to just remove this code. Attached is > a patch for that. Thank you. > There could be an argument for keeping > GET_STRING_RELOPTION actually which is still useful to get a string > value in an option set using the stored offset, and we have > the recently-added dummy_index_am in this category. Any thoughts? Not sure, maybe +0.5 on keeping GET_STRING_RELOPTION. Thanks, Amit
On Wed, Nov 13, 2019 at 02:29:49PM +0900, Amit Langote wrote: > On Wed, Nov 13, 2019 at 2:18 PM Michael Paquier <michael@paquier.xyz> wrote: >> There could be an argument for keeping >> GET_STRING_RELOPTION actually which is still useful to get a string >> value in an option set using the stored offset, and we have >> the recently-added dummy_index_am in this category. Any thoughts? > > Not sure, maybe +0.5 on keeping GET_STRING_RELOPTION. Thinking more about it, I would tend to keep this one around. I'll wait a couple of days before coming back to it. -- Michael
Attachment
В письме от среда, 13 ноября 2019 г. 16:05:20 MSK пользователь Michael Paquier написал: Guys! Sorry for being away for so long. I did not have much opportunities to pay my attention to postgres recently. Thank you for introducing build_reloptions function. It is approximately the direction I wanted to move afterwards myself. But nevertheless, I am steady on my way, and I want to get rid of StdRdOptions before doing anything else myself. This structure is long outdated and is not suitable for access method's options at all. I've changed the patch to use build_reloptions function and again propose it to the commitfest. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Attachment
On Wed, Nov 13, 2019 at 04:05:20PM +0900, Michael Paquier wrote: > On Wed, Nov 13, 2019 at 02:29:49PM +0900, Amit Langote wrote: > > On Wed, Nov 13, 2019 at 2:18 PM Michael Paquier <michael@paquier.xyz> wrote: > >> There could be an argument for keeping > >> GET_STRING_RELOPTION actually which is still useful to get a string > >> value in an option set using the stored offset, and we have > >> the recently-added dummy_index_am in this category. Any thoughts? > > > > Not sure, maybe +0.5 on keeping GET_STRING_RELOPTION. > > Thinking more about it, I would tend to keep this one around. I'll > wait a couple of days before coming back to it. Committed this one and kept GET_STRING_RELOPTION(). With the removal of those macros, it is possible to actually move a portion of the parsing definitions to reloptions.c for each type, but as we expose the validation function string and the enum element definition that would be more confusing IMO, so I have left that out. -- Michael
Attachment
On Wed, Nov 13, 2019 at 04:26:53PM +0300, Nikolay Shaplov wrote: > I've changed the patch to use build_reloptions function and again propose it > to the commitfest. Thanks for the new patch. I have not reviewed the patch in details, but I have a small comment. > +#define SpGistGetFillFactor(relation) \ > + ((relation)->rd_options ? \ > + ((SpGistOptions *) (relation)->rd_options)->fillfactor : \ > + SPGIST_DEFAULT_FILLFACTOR) > + Wouldn't it make sense to add assertions here to make sure that the relkind is an index? You basically did that in commit 3967737. -- Michael
Attachment
В письме от четверг, 14 ноября 2019 г. 16:50:18 MSK пользователь Michael Paquier написал: > > I've changed the patch to use build_reloptions function and again propose > > it to the commitfest. > > Thanks for the new patch. I have not reviewed the patch in details, > but I have a small comment. > > > +#define SpGistGetFillFactor(relation) \ > > + ((relation)->rd_options ? \ > > + ((SpGistOptions *) (relation)->rd_options)->fillfactor : \ > > + SPGIST_DEFAULT_FILLFACTOR) > > + > > Wouldn't it make sense to add assertions here to make sure that the > relkind is an index? You basically did that in commit 3967737. For me there is no mush sense in it, as it does not prevent us from wrong type casting. Indexes can use all kinds of structures for reloptions, and checking that this is index, will not do much. Do you know way how to distinguish one index from another? If we can check in assertion this is index, and this index is spgist, then assertion will make sense for 100%. I just have no idea how to do it. As far as I can see it is not possible now. Another issue here is, that we should to it to all indexes, not only that have been using StdRdOptions, but to all indexes we have. (Damn code inconsistency). So I guess this should go as another patch to keep it step by step improvements. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
On Thu, Nov 14, 2019 at 11:20:25AM +0300, Nikolay Shaplov wrote: > For me there is no mush sense in it, as it does not prevent us from wrong type > casting. Indexes can use all kinds of structures for reloptions, and checking > that this is index, will not do much. It seems to me that if the plan is to have one option structure for each index AM, which has actually the advantage to reduce the bloat of each relcache entry currently relying on StdRdOptions, then we could have those extra assertion checks in the same patch, because the new macros are introduced. > Do you know way how to distinguish one index from another? If we can check in > assertion this is index, and this index is spgist, then assertion will make > sense for 100%. I just have no idea how to do it. As far as I can see it is > not possible now. There is rd_rel->relam. You can for example refer to pgstatindex.c which has AM-related checks to make sure that the correct index AM is being used. -- Michael
Attachment
On Fri, Nov 15, 2019 at 10:34:55AM +0900, Michael Paquier wrote: > It seems to me that if the plan is to have one option structure for > each index AM, which has actually the advantage to reduce the bloat of > each relcache entry currently relying on StdRdOptions, then we could > have those extra assertion checks in the same patch, because the new > macros are introduced. I have looked at this patch, and did not like much having the calculations of the page free space around, so I have moved that into each AM's dedicated header. > There is rd_rel->relam. You can for example refer to pgstatindex.c > which has AM-related checks to make sure that the correct index AM is > being used. We can do something similar for GIN and BRIN on top of the rest, so updated the patch with that. Nikolay, I would be fine to commit this patch as-is. Thanks for your patience on this stuff. -- Michael
Attachment
On Wed, Nov 20, 2019 at 04:44:18PM +0900, Michael Paquier wrote: > We can do something similar for GIN and BRIN on top of the rest, so > updated the patch with that. Nikolay, I would be fine to commit this > patch as-is. Thanks for your patience on this stuff. So, I have reviewed the full thread, and this patch presents a couple of advantages: 1) Making the code more uniform in terms of reloption build and handling for index AMs by using more build_reloptions() with custom parsing tables. 2) Saving a couple of bytes for each relcache entries when rd_options are built for Btree, Hash and SpGist (StdRdOptions gets a small cut). The results of this shaving are not much but my take is that it is always good to shave things if this does not cause extra code readability churns (even if we have more places which waste more). Andres, Tomas, I can see that upthread you voiced concerns about the memory part but not the consistency part. The patch has become much smaller after the initial refactoring steps and it is easier to follow. Any opinions or objections to share regarding the recent progress done? -- Michael
Attachment
В письме от среда, 20 ноября 2019 г. 16:44:18 MSK пользователь Michael Paquier написал: > > It seems to me that if the plan is to have one option structure for > > each index AM, which has actually the advantage to reduce the bloat of > > each relcache entry currently relying on StdRdOptions, then we could > > have those extra assertion checks in the same patch, because the new > > macros are introduced. > I have looked at this patch, and did not like much having the > calculations of the page free space around, so I have moved that into > each AM's dedicated header. Sounds as a good idea. I try not touch such things following the rule "is not broken do not fix" but this way it is definitely better. Thanks! > > There is rd_rel->relam. You can for example refer to pgstatindex.c > > which has AM-related checks to make sure that the correct index AM is > > being used. > > We can do something similar for GIN and BRIN on top of the rest, so > updated the patch with that. That is what I've been trying to tell speaking about code consistency. But ok, this way is also good. > Nikolay, I would be fine to commit this patch as-is. Yeah. I've read the patch. I like it, actually I started doing same thing myself but you were faster. I have opportunity to pay attention to postgres once a week these days... I like the patch, and also agree that it should be commited as is. Though I have a notion to think about. BRIN_AM_OID and friends are defined in catalog/pg_am_d.h so for core indexes we can do relation->rd_rel->relam == BRIN_AM_OID check. But for contrib indexes we can't do such a thing. Bloom index does not need such check as it uses options only when index is created. At that point you can not choose wrong relation. But if in future we will have some contrib index that uses options when it some data is inserted (as it is done with fillfactor in core indexes) then index author will not be able to do such relam check. I would not call it a big problem, but it is something to think about, for sure... > Thanks for your patience on this stuff. Thaks for joining this work, and sorry for late replies. Now I quite rarely have time for postgres :-( -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Hello, On Thu, Nov 21, 2019 at 2:22 PM Michael Paquier <michael@paquier.xyz> wrote: > Any opinions or objections to share regarding the recent > progress done? The latest patch looks good to me, except, maybe the comment of StdRdOptions should be updated: * StdRdOptions * Standard contents of rd_options for heaps and generic indexes. IIUC, StdRdOptions no longer applies to indexes, right? Thanks, Amit
On Thu, Nov 21, 2019 at 09:39:53PM +0300, Nikolay Shaplov wrote: > BRIN_AM_OID and friends are defined in catalog/pg_am_d.h so for core indexes > we can do relation->rd_rel->relam == BRIN_AM_OID check. But for contrib > indexes we can't do such a thing. > Bloom index does not need such check as it uses options only when index is > created. At that point you can not choose wrong relation. But if in future we > will have some contrib index that uses options when it some data is inserted > (as it is done with fillfactor in core indexes) then index author will not be > able to do such relam check. I would not call it a big problem, but it is > something to think about, for sure... I don't think that you actually need that for custom index AMs anyway, as all code paths leading to the lookup of their reloption values is within the module they are defined in. > Thaks for joining this work, and sorry for late replies. Now I quite rarely > have time for postgres :-( We all have a life, don't worry. I am glad to see you around. -- Michael
Attachment
On Fri, Nov 22, 2019 at 11:01:54AM +0900, Amit Langote wrote: > The latest patch looks good to me, except, maybe the comment of > StdRdOptions should be updated: > > * StdRdOptions > * Standard contents of rd_options for heaps and generic indexes. > > IIUC, StdRdOptions no longer applies to indexes, right? Noted, thanks. I'll sit on this thing for a couple of days, and will likely look at it again on Monday in order to commit it. -- Michael
Attachment
On Fri, Nov 22, 2019 at 04:44:50PM +0900, Michael Paquier wrote: > Noted, thanks. I'll sit on this thing for a couple of days, and will > likely look at it again on Monday in order to commit it. And done. -- Michael