Thread: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
In the thread https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m I've suggested to split one big StdRdOption that is used for options storage into into Options structures individual for each relkind and each relam That patch have been split into smaller parts, most of them were already commit: https://commitfest.postgresql.org/25/2294/ - Remove StdRdOptions from AM https://commitfest.postgresql.org/25/2297/ - Do not use StdRdOption for partitioned tables https://commitfest.postgresql.org/25/2295/ - Some useful Asserts for view- related macroses. And here goes the last part of StrRdOptions removal patch, where StdRdOptions is replaced with HeapOptions and ToastOptions. What did I do here. - Added HeapOptions and ToastOptions structues - Moved options building tab for autovacuum into AUTOVACUUM_RELOPTIONS macro, so it can be used in relopt_parse_elt tab both for heap and toast - Changed everywhere in the code, where old heap_reloptions is used, to use new heap_reloptions or toast_reloptions - Changed heap & toast option fetching macros to use HeapOptions and ToastOptions - Added Asserts to heap and toast options macros. Now we finally can do it. What I did not do - I've split fillfactor related macros into heap and toast like RelationGetFillFactor will become HeapGetFillFactor and ToastGetFillFactor. I have to do it, because now they handle different structure. but there are heap only option macros like RelationGetParallelWorkers that should be better called HeapGetParallelWorkers, as it is heap related. But I did not change the name, as somebody from core team (I think it was Alvaro, it was a while ago) asked me not to change macros names unless in is inavoidable. So I kept the names, though I still think that naming them with Heap prefix will make code more clear. - vacuum_index_cleanup and vacuum_truncate options were added recently. They were added into StdRdOptions. I think their place is inside AutoVacOpts not in StdRdOptions, but did not dare to change it. If you see it the same way as I see, please let me know, I will move it to a proper place. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Attachment
On Fri, Dec 06, 2019 at 06:18:48PM +0300, Nikolay Shaplov wrote: > In the thread > https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m > I've suggested to split one big StdRdOption that is used for options storage > into into Options structures individual for each relkind and each relam > > And here goes the last part of StrRdOptions removal patch, where StdRdOptions > is replaced with HeapOptions and ToastOptions. -typedef struct StdRdOptions +/* + * HeapOptions + * Binary representation of relation options for Heap relations. + */ +typedef struct HeapOptions I think that it makes sense to split relation options dedicated to heap into their own parsing structure, because those options are actually related to the table AM heap. However, I think that this patch is not ambitious enough in the work which is done and that things could move into a more generic direction. At the end of the day, I'd like to think that we should have something like: - Heap-related reloptions are built as part of its AM handler in heapam_handler.c, with reloptions.c holding no more references to heap. At all. - The table AM option parsing follows a model close to what is done for indexes in terms of option parsing, moving the responsibility to define relation options to each table AM. - Toast is an interesting case, as table AMs may want to use toast tables. Or not. Robert may be able to comment more on that as he has worked in this area for bd12499. -- Michael
Attachment
В письме от понедельник, 9 декабря 2019 г. 12:11:17 MSK пользователь Michael Paquier написал: > On Fri, Dec 06, 2019 at 06:18:48PM +0300, Nikolay Shaplov wrote: > > In the thread > > https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m > > I've suggested to split one big StdRdOption that is used for options > > storage into into Options structures individual for each relkind and each > > relam > > > > And here goes the last part of StrRdOptions removal patch, where > > StdRdOptions is replaced with HeapOptions and ToastOptions. > > -typedef struct StdRdOptions > +/* > + * HeapOptions > + * Binary representation of relation options for Heap relations. > + */ > +typedef struct HeapOptions > > I think that it makes sense to split relation options dedicated to > heap into their own parsing structure, because those options are > actually related to the table AM heap. However, I think that this > patch is not ambitious enough in the work which is done and that > things could move into a more generic direction. At the end of the > day, I'd like to think that we should have something like: > - Heap-related reloptions are built as part of its AM handler in > heapam_handler.c, with reloptions.c holding no more references to > heap. At all. > - The table AM option parsing follows a model close to what is done > for indexes in terms of option parsing, moving the responsibility to > define relation options to each table AM. > - Toast is an interesting case, as table AMs may want to use toast > tables. Or not. Robert may be able to comment more on that as he has > worked in this area for bd12499. Oh, yeah, I forget that relations now also have AM :-) But the truth is that my goal is to move all code that defines all option names, min/max values etc, move it inside am code. To move data from boolRelOpts, intRelOpts, realRelOpts, enumRelOpts, enumRelOpts from reloptions.c into the code that implement AMs that uses these options. I did it for indexes in patch I've offered several years ago. Now we have also relaion AM. But I would prefer to fix index AM relioptions first, and then copy that solution for relations. Because if I frist copy AM solution from indexes to relation, then I will have to fix it in two places. So I would prefer to keep reloptions for relations in relations.c, only split them into HeapOptions and ToastOptions, then change AM for indexes moving option definition into AM's and then clone the solution for relations. This seems to be most simple and most logical way. PS. I've checked the patch against current master. No changes were needed, but I am attaching a diff made against current master, just in case.
Attachment
On Wed, Mar 04, 2020 at 10:58:31PM +0300, Nikolay Shaplov wrote: > But the truth is that my goal is to move all code that defines all option > names, min/max values etc, move it inside am code. To move data from > boolRelOpts, intRelOpts, realRelOpts, enumRelOpts, enumRelOpts from > reloptions.c into the code that implement AMs that uses these options. > > I did it for indexes in patch I've offered several years ago. Now we have also > relaion AM. > > But I would prefer to fix index AM relioptions first, and then copy that > solution for relations. How do you think that this part should be changed then, if this needs any changes? It seems to me that we have a rather correct layer for index AMs by requiring each one to define the available option set using indoptions through their handler, with option fetching macros located within each AM. > Because if I first copy AM solution from indexes to relation, then I will have > to fix it in two places. > > So I would prefer to keep reloptions for relations in relations.c, only split > them into HeapOptions and ToastOptions, then change AM for indexes moving > option definition into AM's and then clone the solution for relations. Then, for table AMs, it seems to me that you are right for long-term perspective to have the toast-related options in reloptions.c, or perhaps directly located within more toast-related file (?) as table AMs interact with toast using heapam_relation_needs_toast_table and such callbacks. So for heap, moving the option handling to roughly heapam_handler.c is a natural move, though this requires a redesign of the existing structure to use option handling closer to what indoptions does, but for tables. -- Michael
Attachment
В письме от суббота, 7 марта 2020 г. 10:03:40 MSK пользователь Michael Paquier
написал:
> On Wed, Mar 04, 2020 at 10:58:31PM +0300, Nikolay Shaplov wrote:
> > But the truth is that my goal is to move all code that defines all option
> > names, min/max values etc, move it inside am code. To move data from
> > boolRelOpts, intRelOpts, realRelOpts, enumRelOpts, enumRelOpts from
> > reloptions.c into the code that implement AMs that uses these options.
> >
> > I did it for indexes in patch I've offered several years ago. Now we have
> > also relaion AM.
> >
> > But I would prefer to fix index AM relioptions first, and then copy that
> > solution for relations.
>
> How do you think that this part should be changed then, if this needs
> any changes? It seems to me that we have a rather correct layer for
> index AMs by requiring each one to define the available option set
> using indoptions through their handler, with option fetching macros
> located within each AM.
My idea is like this:
Now information about what reloption AM has and how they should be parsed are
stored in two places. One is relioptions.c and boolRelOpts, intRelOpts,
realRelOpts, enumRelOpts, enumRelOpts arrays, another is
static const relopt_parse_elt tab[] inside amoptions_function amoptions;
function of each AM.
My suggestion is to merge all this data into one structure. Like
option_definition
/* generic struct to hold shared data */
typedef struct option_definition_basic
{
const char *name; /* must be first (used as list termination
* marker) */
const char *desc;
LOCKMODE lockmode;
option_definition_flags flags;
option_type type;
int struct_offset; /* offset of the value in Bytea representation
*/
} option_definition_basic;
typedef struct option_definition_bool
{
option_definition_basic base;
bool default_val;
} option_definition_bool;
typedef struct option_definition_int
{
option_definition_basic base;
int default_val;
int min;
int max;
} option_definition_int;
typedef struct option_definition_real
{
option_definition_basic base;
double default_val;
double min;
double max;
} option_definition_real;
typedef struct option_definition_enum
{
option_definition_basic base;
const char **allowed_values;/* Null terminated array of allowed values for
* the option */
int default_val; /* Number of item of allowed_values array */
} option_definition_enum;
This example from my old code, I guess I should add a union here, this will
make code more readable... But the idea is the same, we have one structure
that describes how this option should be parsed.
Then we gather all options definitions for one object (for example for index)
into a structure called OptionsDefSet
typedef struct OptionDefSet
{
option_definition_basic **definitions;
int num; /* Number of catalog items in use */
int num_allocated; /* Number of catalog items allocated */
bool forbid_realloc; /* If number of items of the catalog were
* strictly set to certain value do no allow
* adding more idems */
Size struct_size; /* Size of a structure for options in binary
* representation */
postprocess_bytea_options_function postprocess_fun; /* This function is
* called after options
* were converted in
* Bytea represenation.
* Can be used for extra
* validation and so on */
char *namespace; /* Catalog is used for options from this
* namespase */
} OptionDefSet;
This DefSet will have all we need to parse options for certain object.
This all will be stored in the AM code.
Then we replace amoptions_function amoptions; function of the AM with
amoptions_def_set_function amoptions_def_set; function. This function will
return OptionDefSet to whoever called it.
So whenever we need an option in index_reloptions from reloption.c we get this
DefSet, and then call a function that parses options using this data.
Why do we need this.
1. To have all options related data in one place. Now when a developer wants
to add an option, he needs to find all places in the code this option should
be added. It is not good thing. It brings troubles and errors.
2. Now we have two different options storages for in-core AM options and for
contib AM options. They are boolRelOpts, intRelOpts, realRelOpts, enumRelOpts,
enumRelOpts arrays for in-core AMm's and static relopt_gen **custom_options
for contrib AM. It if better ho have same tool for both. My idea will allow to
have same code both in contrib and in-core AM.
3. Then we will be able to have reloptions-like options just anywhere, just
define an OptionDefSet, and put code that provides option values from SQL and
pg_catalog. I came to the idea of rewriting option code when I have been
working on the task of adding opclass parameters, Nikita Glukhov works on it
now. It is very simulator to options, but now you can not use reloptions code
for that. When we will have OptionDefSet I've described, it can be used for
opclass parameters too.
The example of how it can be done can be found at https://
commitfest.postgresql.org/15/992/
The code is quite outdated, and has a lot of extra code that already commited
or is not really needed. But you can see the idea.
I will try to provide new version of it soon, with no extra code, that can be
applied to current master, but it would be good to apply it to the master
branch where there is no StdRdOptions. This will make patch more readable and
understandable.
So, can I please have it in the code :-)
> > Because if I first copy AM solution from indexes to relation, then I will
> > have to fix it in two places.
> >
> > So I would prefer to keep reloptions for relations in relations.c, only
> > split them into HeapOptions and ToastOptions, then change AM for indexes
> > moving option definition into AM's and then clone the solution for
> > relations.
> Then, for table AMs, it seems to me that you are right for long-term
> perspective to have the toast-related options in reloptions.c, or
> perhaps directly located within more toast-related file (?) as table
> AMs interact with toast using heapam_relation_needs_toast_table and
> such callbacks. So for heap, moving the option handling to roughly
> heapam_handler.c is a natural move, though this requires a redesign of
> the existing structure to use option handling closer to what
> indoptions does, but for tables.
We also have view reloptions and attribute options, as well as toast options.
We can keep them in reloption.c or find better place for them. It is not a big
problem I think. And as for heap options, yes, as heap now has AM, they should
be moved inside AM. I can do it when we are finished with index options.
A new version of the patch. Autovacuum options were extended in b07642db So I added that options to the current patch.
Attachment
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
From
Daniel Gustafsson
Date:
> On 28 Mar 2020, at 19:57, Nikolay Shaplov <dhyan@nataraj.su> wrote: > > A new version of the patch. > Autovacuum options were extended in b07642db > > So I added that options to the current patch. The heapam.c hunk in this version fails to apply to HEAD, can you please submit a rebased version? Marking the CF entry as Waiting on Author in the meantime. cheers ./daniel
В письме от четверг, 2 июля 2020 г. 17:15:13 MSK пользователь Daniel Gustafsson написал: > > A new version of the patch. > > Autovacuum options were extended in b07642db > > > > So I added that options to the current patch. > > The heapam.c hunk in this version fails to apply to HEAD, can you please > submit a rebased version? Thanks for reminding about it. Here goes a rebased version. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Attachment
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
From
Georgios Kokolatos
Date:
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation: not tested Hi, thank you for the patch. It applies cleanly, compiles and passes check, check-world. I feel as per the discussion, this is a step to the right direction yet it does not get far enough. From experience, I canconfirm that dealing with reloptions in a new table AM is somewhat of a pain. Ultimately, reloptions should be handledby the table AM specific code. The current patch does not address the issue. Yet it does make the issue easier toaddress by clearing up the current state. If you allow me, I have a couple of comments. - saveFreeSpace = RelationGetTargetPageFreeSpace(relation, - HEAP_DEFAULT_FILLFACTOR); + if (IsToastRelation(relation)) + saveFreeSpace = ToastGetTargetPageFreeSpace(); + else + saveFreeSpace = HeapGetTargetPageFreeSpace(relation); For balance, it does make some sense for ToastGetTargetPageFreeSpace() to get relation as an argument, similarly to HeapGetTargetPageFreeSpace(). Also, this pattern is repeated in four places, maybe the branch can be moved inside a macro or static inline instead? - /* Retrieve the parallel_workers reloption, or -1 if not set. */ - rel->rel_parallel_workers = RelationGetParallelWorkers(relation, -1); + /* + * Retrieve the parallel_workers for heap and mat.view relations. + * Use -1 if not set, or if we are dealing with other relation kinds + */ + if (relation->rd_rel->relkind == RELKIND_RELATION || + relation->rd_rel->relkind == RELKIND_MATVIEW) + rel->rel_parallel_workers = RelationGetParallelWorkers(relation, -1); + else + rel->rel_parallel_workers = -1; If the comment above is agreed upon, then it makes a bit of sense to apply the same here. The expression in the branch isalready asserted for in macro, why not switch there and remove the responsibility from the caller? Any thoughts on the above? Cheers, Georgios The new status of this patch is: Waiting on Author
В письме от понедельник, 20 июля 2020 г. 18:36:44 MSK пользователь Georgios Kokolatos написал: Hi! Sorry for really long delay, I was at my summer vacations, and then has urgent things to finish first. :-( Now I hope we can continue... > thank you for the patch. It applies cleanly, compiles and passes check, > check-world. Thank you for reviewing efforts. > I feel as per the discussion, this is a step to the right direction yet it > does not get far enough. From experience, I can confirm that dealing with > reloptions in a new table AM is somewhat of a pain. Ultimately, reloptions > should be handled by the table AM specific code. The current patch does not > address the issue. Yet it does make the issue easier to address by clearing > up the current state. Moving reloptions to AM code is the goal I am slowly moving to. I've started some time ago with big patch https://commitfest.postgresql.org/14/992/ and have been told to split it into smaller parts. So I did, and this patch is last step that cleans options related things up, and then actual moving can be done. > If you allow me, I have a couple of comments. > > - saveFreeSpace = RelationGetTargetPageFreeSpace(relation, > - HEAP_DEFAULT_FILLFACTOR); > + if (IsToastRelation(relation)) > + saveFreeSpace = ToastGetTargetPageFreeSpace(); > + else > + saveFreeSpace = HeapGetTargetPageFreeSpace(relation); > > For balance, it does make some sense for ToastGetTargetPageFreeSpace() to > get relation as an argument, similarly to HeapGetTargetPageFreeSpace(). ToastGetTargetPageFreeSpace return a const value. I've change the code, so it gets relation argument, that is not used, the way you suggested. But I am not sure if it is good or bad idea. May be we will get some "Unused variable" warning on some compilers. I like consistency... But not sure we need it here. > - /* Retrieve the parallel_workers reloption, or -1 if not set. */ > - rel->rel_parallel_workers = RelationGetParallelWorkers(relation, > -1); + /* > + * Retrieve the parallel_workers for heap and mat.view relations. > + * Use -1 if not set, or if we are dealing with other relation > kinds + */ > + if (relation->rd_rel->relkind == RELKIND_RELATION || > + relation->rd_rel->relkind == RELKIND_MATVIEW) > + rel->rel_parallel_workers = > RelationGetParallelWorkers(relation, -1); + else > + rel->rel_parallel_workers = -1; > Also, this pattern is repeated in four places, maybe the branch can be > moved inside a macro or static inline instead? > If the comment above is agreed upon, then it makes a bit of sense to apply > the same here. The expression in the branch is already asserted for in > macro, why not switch there and remove the responsibility from the caller? I guess here you are right, because here the logic is following: for heap relation take option from options, for _all_ others use -1. This can be moved to macro. So I changed it to /* * HeapGetParallelWorkers * Returns the heap's parallel_workers reloption setting. * Note multiple eval of argument! */ #define HeapGetParallelWorkers(relation, defaultpw) \ (AssertMacro(relation->rd_rel->relkind == RELKIND_RELATION || \ relation->rd_rel->relkind == RELKIND_MATVIEW), \ (relation)->rd_options ? \ ((HeapOptions *) (relation)->rd_options)->parallel_workers : \ (defaultpw)) /* * RelationGetParallelWorkers * Returns the relation's parallel_workers reloption setting. * Note multiple eval of argument! */ #define RelationGetParallelWorkers(relation, defaultpw) \ (((relation)->rd_rel->relkind == RELKIND_RELATION || \ (relation)->rd_rel->relkind == RELKIND_MATVIEW) ? \ HeapGetParallelWorkers(relation, defaultpw) : defaultpw) But I would not like to move if (IsToastRelation(relation)) saveFreeSpace = ToastGetTargetPageFreeSpace(relation); else saveFreeSpace = HeapGetTargetPageFreeSpace(relation); into macros, as there is a choice only between heap and toast. All other relation types are not mentioned. So we can not call it RelationGetTargetPageFreeSpace. It would be ToastOrHeapGetTargetPageFreeSpace actually. Better not to have such macro. Please find new version of the patch in the attachment. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Attachment
On Sun, 2020-09-13 at 19:04 +0300, Nikolay Shaplov wrote: > Moving reloptions to AM code is the goal I am slowly moving to. I've > started > some time ago with big patch > https://commitfest.postgresql.org/14/992/ and > have been told to split it into smaller parts. So I did, and this > patch is > last step that cleans options related things up, and then actual > moving can be > done. Thank you for working on this. Can you outline the plan for moving these options to the table AM to make sure this patch is a step in the right direction? I was trying to work through this problem as well[1], and there are a few complications. * Which options apply to any relation (of any table AM), and which apply to only heaps? As far as I can tell, the only one that seems heap-specific is "fillfactor". * Toast tables can be any AM, as well, so if we accept new reloptions for a custom AM, we also need to accept options for toast tables of that AM. * Implementation-wise, the bytea representation of the options is not very easy to extend. Should we have a new text field in the catalog to hold the custom options? Regards, Jeff Davis [1] https://www.postgresql.org/message-id/43c6ec161f930e385dbc3169a065a917cfc60714.camel%40j-davis.com
В письме от пятница, 4 июня 2021 г. 3:19:09 MSK пользователь Jeff Davis написал: > > Moving reloptions to AM code is the goal I am slowly moving to. I've > > started > > some time ago with big patch > > https://commitfest.postgresql.org/14/992/ and > > have been told to split it into smaller parts. So I did, and this > > patch is > > last step that cleans options related things up, and then actual > > moving can be > > done. > > Thank you for working on this. Welcome! Sorry for slow reply, I am on summer vacations now, no chance for fast replies now :-) > Can you outline the plan for moving these options to the table AM to > make sure this patch is a step in the right direction? Yes, I can. First you can see the whole patch, the way it was several years ago: https://commitfest.postgresql.org/15/992/, reloptions8a.diff The things I would say is accurate for postgres ~11, it may have been changed since I last payed attention to them. So, there are three general places where options can be stored: 1. Global boolRelOpts, intRelOpts, realRelOpts, stringRelOpts in src/backend/ access/common/reloptions.c for in-core access methods. 2. custom_options array of accessable via add_bool_reloption, add_int_reloption, add_real_reloption, add_string_reloption for access methods from contribs. (See reloptions.c too) 3. And also each access method has an array of relopt_parse_elt[] that is also about reloptions. 1 and 2 are technically arrays of relopt_get, and store information what kind of options do we have. 3 is array of relopt_parse_elt[] that store information how options should be stored into binary representation. My idea was to merge relopt_get and relopt_parse_elt into one structure (in my patch it is "option_definition_basic"), each access method, that needs options, should have a set of option_definition_basic for their options (called option_definition_basic in my patch, may be should be renamed) and this set of option_definitions is the only data that is needed to parse option into binary representation. So in access method instead of am_option function we will have amrelopt_catalog function that returns "options defenition set" for this am, and this definition set is used by option parser to parse options. So, it my explanation is not quite clear, please ask questions, I will try to answer them. > I was trying to work through this problem as well[1], and there are a > few complications. > > * Which options apply to any relation (of any table AM), and which > apply to only heaps? As far as I can tell, the only one that seems > heap-specific is "fillfactor". From my point of view, each relation kind has it's own set of options. The fact, that almost every kind has a fillfactor is just a coincidence. If we try to do some optimization here, we will be buried under the complexity of it soon. So they are _different_ options just having the same name. > * Toast tables can be any AM, as well, so if we accept new reloptions > for a custom AM, we also need to accept options for toast tables of > that AM. When I wrote this patch, AM was introduced only to index relations. I do not how it is implemented for heap now, but there should be some logic in it. If toast tables has it's own AM, then option definition set should be stored there, and we should find a way to work with it, somehow. > * Implementation-wise, the bytea representation of the options is not > very easy to extend. Should we have a new text field in the catalog to > hold the custom options? I am not really understanding this question. Normally all options can be well represented as binary structure stored at bytea. I see no problem here. If we need some unusual behaviour, we can use string option with custom validation function. This should cover almost all needs I can imagine. ======= So it you are interested in having better option implementation, and has no ideas of your own, I would suggest to revive my patch and try to commit it. What I need first of all is a reviewer. Testing and coauthoring will also be apriciated. My original big patch, I gave you link to, have been split into several parts. The last minor part, that should be commit in advance, and have not been commit yet is https://commitfest.postgresql.org/33/2370/ If you join as a reviewer this would be splendid! :-) -- Nikolay Shaplov dhyan@nataraj.su (e-mail, jabber) @dhyan:nataraj.su (matrix)
On Sun, Sep 13, 2020 at 9:34 PM Nikolay Shaplov <dhyan@nataraj.su> wrote: > > В письме от понедельник, 20 июля 2020 г. 18:36:44 MSK пользователь Georgios > Kokolatos написал: > > Hi! Sorry for really long delay, I was at my summer vacations, and then has > urgent things to finish first. :-( Now I hope we can continue... > > > > thank you for the patch. It applies cleanly, compiles and passes check, > > check-world. > > Thank you for reviewing efforts. > > > I feel as per the discussion, this is a step to the right direction yet it > > does not get far enough. From experience, I can confirm that dealing with > > reloptions in a new table AM is somewhat of a pain. Ultimately, reloptions > > should be handled by the table AM specific code. The current patch does not > > address the issue. Yet it does make the issue easier to address by clearing > > up the current state. > > Moving reloptions to AM code is the goal I am slowly moving to. I've started > some time ago with big patch https://commitfest.postgresql.org/14/992/ and > have been told to split it into smaller parts. So I did, and this patch is > last step that cleans options related things up, and then actual moving can be > done. > > > If you allow me, I have a couple of comments. > > > > - saveFreeSpace = RelationGetTargetPageFreeSpace(relation, > > - HEAP_DEFAULT_FILLFACTOR); > > + if (IsToastRelation(relation)) > > + saveFreeSpace = ToastGetTargetPageFreeSpace(); > > + else > > + saveFreeSpace = HeapGetTargetPageFreeSpace(relation); > > > > For balance, it does make some sense for ToastGetTargetPageFreeSpace() to > > get relation as an argument, similarly to HeapGetTargetPageFreeSpace(). > > ToastGetTargetPageFreeSpace return a const value. I've change the code, so it > gets relation argument, that is not used, the way you suggested. But I am not > sure if it is good or bad idea. May be we will get some "Unused variable" > warning on some compilers. I like consistency... But not sure we need it here. > > > - /* Retrieve the parallel_workers reloption, or -1 if not set. */ > > - rel->rel_parallel_workers = RelationGetParallelWorkers(relation, > > -1); > + /* > > + * Retrieve the parallel_workers for heap and mat.view relations. > > + * Use -1 if not set, or if we are dealing with other relation > > kinds > + */ > > + if (relation->rd_rel->relkind == RELKIND_RELATION || > > + relation->rd_rel->relkind == RELKIND_MATVIEW) > > + rel->rel_parallel_workers = > > RelationGetParallelWorkers(relation, -1); > + else > > + rel->rel_parallel_workers = -1; > > Also, this pattern is repeated in four places, maybe the branch can be > > moved inside a macro or static inline instead? > > > If the comment above is agreed upon, then it makes a bit of sense to apply > > the same here. The expression in the branch is already asserted for in > > macro, why not switch there and remove the responsibility from the caller? > > I guess here you are right, because here the logic is following: for heap > relation take option from options, for _all_ others use -1. This can be moved > to macro. > > So I changed it to > > /* > * HeapGetParallelWorkers > * Returns the heap's parallel_workers reloption setting. > * Note multiple eval of argument! > */ > #define HeapGetParallelWorkers(relation, defaultpw) \ > (AssertMacro(relation->rd_rel->relkind == RELKIND_RELATION || \ > relation->rd_rel->relkind == RELKIND_MATVIEW), \ > (relation)->rd_options ? \ > ((HeapOptions *) (relation)->rd_options)->parallel_workers : \ > (defaultpw)) > > /* > * RelationGetParallelWorkers > * Returns the relation's parallel_workers reloption setting. > * Note multiple eval of argument! > */ > > #define RelationGetParallelWorkers(relation, defaultpw) \ > (((relation)->rd_rel->relkind == RELKIND_RELATION || \ > (relation)->rd_rel->relkind == RELKIND_MATVIEW) ? \ > HeapGetParallelWorkers(relation, defaultpw) : defaultpw) > > > But I would not like to move > > if (IsToastRelation(relation)) > saveFreeSpace = ToastGetTargetPageFreeSpace(relation); > else > saveFreeSpace = HeapGetTargetPageFreeSpace(relation); > > into macros, as there is a choice only between heap and toast. All other > relation types are not mentioned. > > So we can not call it RelationGetTargetPageFreeSpace. It would be > ToastOrHeapGetTargetPageFreeSpace actually. Better not to have such macro. > > Please find new version of the patch in the attachment. The patch does not apply on Head anymore, could you rebase and post a patch. I'm changing the status to "Waiting for Author". Regards, Vignesh
В письме от среда, 14 июля 2021 г. 15:09:12 MSK пользователь vignesh C
написал:
> The patch does not apply on Head anymore, could you rebase and post a
> patch. I'm changing the status to "Waiting for Author".
Thank you for notification.
I've tried to rebase it and found out that some options have been added to
partitioned table.
Handling this needs careful approach, and I will fix it two weeks later, when I
am back from vacations.
Meanwhile I would strongly suggest to change
{"vacuum_index_cleanup", RELOPT_TYPE_BOOL,
to
{"vacuum_index_cleanup", RELOPT_TYPE_ENUM,
in src/backend/access/common/reloptions.c
This change should be done in 3499df0d
But current implementation of reloptions is very error prone , and it is very
easy to miss this part.
--
Nikolay Shaplov
dhyan@nataraj.su (e-mail, jabber)
@dhyan:nataraj.su (matrix)