Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
В письме от среда, 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
dh...@nataraj.su (e-mail, jabber)
@dhyan:nataraj.su (matrix)
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
On Sun, Sep 13, 2020 at 9:34 PM Nikolay Shaplov 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
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
В письме от пятница, 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 dh...@nataraj.su (e-mail, jabber) @dhyan:nataraj.su (matrix)
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
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
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
Hi Georgios, On 9/13/20 12:04 PM, Nikolay Shaplov wrote: В письме от понедельник, 20 июля 2020 г. 18:36:44 MSK пользователь Georgios Kokolatos написал: thank you for the patch. It applies cleanly, compiles and passes check, check-world. Thank you for reviewing efforts. Please find new version of the patch in the attachment. Any thoughts on the updated patch? Regards, -- -David da...@pgmasters.net
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
В письме от понедельник, 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))
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
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 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. 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 is already 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
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
В письме от четверг, 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)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8ccc228..6118b55 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -46,9 +46,8 @@
* upper and lower bounds (if applicable); for strings, consider a validation
* routine.
* (ii) add a record below (or use add__reloption).
- * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
- * (iv) add it to the appropriate handling routine (perhaps
- * default_reloptions)
+ * (iii) add it to the appropriate options struct
+ * (iv) add it to the appropriate handling routine
* (v) make sure the lock level is set correctly for that operation
* (vi) don't forget to document the option
*
@@ -1375,9 +1374,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
switch (classForm->relkind)
{
case RELKIND_RELATION:
- case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- options = heap_reloptions(classForm->relkind, datum, false);
+ options = heap_reloptions(datum, false);
+ break;
+ case RELKIND_TOASTVALUE:
+ options = toast_reloptions(datum, false);
break;
case RELKIND_PARTITIONED_TABLE:
options = partitioned_table_reloptions(datum, false);
@@ -1811,59 +1812,66 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions.
+ * Option parsing definition for autovacuum. Used in toast and heap options.
+ */
+
+#define AUTOVACUUM_RELOPTIONS(OFFSET)\
+ {"autovacuum_enabled", RELOPT_TYPE_BOOL, \
+ OFFSET + offsetof(AutoVacOpts, enabled)},\
+ {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_threshold)}, \
+ {"autovacuum_vacuum_insert_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_ins_threshold)}, \
+ {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, analyze_threshold)}, \
+ {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_delay)}, \
+ {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_limit)}, \
+ {"autovacuum_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_min_age)}, \
+ {"autovacuum_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_max_age)}, \
+ {"autovacuum_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_table_age)}, \
+ {"autovacuum_multixact_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_min_age)}, \
+ {"autovacuum_multixact_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_max_age)}, \
+ {"autovacuum_multixact_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_table_age)}, \
+ {"log_autovacuum_min_duration", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, log_min_duration)}, \
+ {"autovacuum_vacuum_scale_factor", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_scale_factor)},\
+ {"autovacuum_vacuum_insert_scale_factor", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_ins_scale_factor)}, \
+ {"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,\
+ OFFSET + offsetof(AutoVacOpts, analyze_scale_factor)}
+/*
+ * Option parser for heap
*/
bytea *
-default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
+heap_reloptions(Datum reloptions, bool validate)
{
static const relopt_parse_elt tab[] = {
- {"fillfactor", RELOPT_TYPE_INT, offsetof(StdRdOptions, fillfactor)},
- {"autovacuum_enabled", RELOPT_TYPE_BOOL,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, enabled)},
- {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_threshold)},
- {"autovacuum_vacuum_insert_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_ins_threshold)},
- {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, analyze_threshold)},
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
> On 28 Mar 2020, at 19:57, Nikolay Shaplov 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
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
A new version of the patch.
Autovacuum options were extended in b07642db
So I added that options to the current patch.diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index e136116..55bd1ec 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -46,9 +46,8 @@
* upper and lower bounds (if applicable); for strings, consider a validation
* routine.
* (ii) add a record below (or use add__reloption).
- * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
- * (iv) add it to the appropriate handling routine (perhaps
- * default_reloptions)
+ * (iii) add it to the appropriate options struct
+ * (iv) add it to the appropriate handling routine
* (v) make sure the lock level is set correctly for that operation
* (vi) don't forget to document the option
*
@@ -1147,9 +1146,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
switch (classForm->relkind)
{
case RELKIND_RELATION:
- case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- options = heap_reloptions(classForm->relkind, datum, false);
+ options = heap_reloptions(datum, false);
+ break;
+ case RELKIND_TOASTVALUE:
+ options = toast_reloptions(datum, false);
break;
case RELKIND_PARTITIONED_TABLE:
options = partitioned_table_reloptions(datum, false);
@@ -1521,59 +1522,66 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions.
+ * Option parsing definition for autovacuum. Used in toast and heap options.
+ */
+
+#define AUTOVACUUM_RELOPTIONS(OFFSET)\
+ {"autovacuum_enabled", RELOPT_TYPE_BOOL, \
+ OFFSET + offsetof(AutoVacOpts, enabled)},\
+ {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_threshold)}, \
+ {"autovacuum_vacuum_insert_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_ins_threshold)}, \
+ {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, analyze_threshold)}, \
+ {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_delay)}, \
+ {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_limit)}, \
+ {"autovacuum_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_min_age)}, \
+ {"autovacuum_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_max_age)}, \
+ {"autovacuum_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_table_age)}, \
+ {"autovacuum_multixact_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_min_age)}, \
+ {"autovacuum_multixact_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_max_age)}, \
+ {"autovacuum_multixact_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_table_age)}, \
+ {"log_autovacuum_min_duration", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, log_min_duration)}, \
+ {"autovacuum_vacuum_scale_factor", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_scale_factor)},\
+ {"autovacuum_vacuum_insert_scale_factor", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_ins_scale_factor)}, \
+ {"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,\
+ OFFSET + offsetof(AutoVacOpts, analyze_scale_factor)}
+/*
+ * Option parser for heap
*/
bytea *
-default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
+heap_reloptions(Datum reloptions, bool validate)
{
static const relopt_parse_elt tab[] = {
- {"fillfactor", RELOPT_TYPE_INT, offsetof(StdRdOptions, fillfactor)},
- {"autovacuum_enabled", RELOPT_TYPE_BOOL,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, enabled)},
- {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_threshold)},
- {"autovacuum_vacuum_insert_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_ins_threshold)},
- {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, analyze_threshold)},
- {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_cost_limit)},
- {"autovacuum_freeze_min_age", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, freeze_min_age)},
- {"autovacuum_freeze_max_age", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, freeze_max_age)},
-
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
В письме от суббота, 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;
LOCKMODElockmode;
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;
booldefault_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
{
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
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 signature.asc Description: PGP signature
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
В письме от понедельник, 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.
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index c3d45c7..73d5e4f 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -46,9 +46,8 @@
* upper and lower bounds (if applicable); for strings, consider a validation
* routine.
* (ii) add a record below (or use add__reloption).
- * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
- * (iv) add it to the appropriate handling routine (perhaps
- * default_reloptions)
+ * (iii) add it to the appropriate options struct
+ * (iv) add it to the appropriate handling routine
* (v) make sure the lock level is set correctly for that operation
* (vi) don't forget to document the option
*
@@ -1116,9 +1115,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
switch (classForm->relkind)
{
case RELKIND_RELATION:
- case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- options = heap_reloptions(classForm->relkind, datum, false);
+ options = heap_reloptions(datum, false);
+ break;
+ case RELKIND_TOASTVALUE:
+ options = toast_reloptions(datum, false);
break;
case RELKIND_PARTITIONED_TABLE:
options = partitioned_table_reloptions(datum, false);
@@ -1490,55 +1491,62 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions.
+ * Option parsing definition for autovacuum. Used in toast and heap options.
+ */
+
+#define AUTOVACUUM_RELOPTIONS(OFFSET)\
+ {"autovacuum_enabled", RELOPT_TYPE_BOOL, \
+ OFFSET + offsetof(AutoVacOpts, enabled)},\
+ {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_threshold)}, \
+ {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, analyze_threshold)}, \
+ {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_delay)}, \
+ {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_limit)}, \
+ {"autovacuum_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts,
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
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 signature.asc Description: PGP signature
[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)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 48377ac..8b18381 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -46,9 +46,8 @@
* upper and lower bounds (if applicable); for strings, consider a validation
* routine.
* (ii) add a record below (or use add__reloption).
- * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
- * (iv) add it to the appropriate handling routine (perhaps
- * default_reloptions)
+ * (iii) add it to the appropriate options struct
+ * (iv) add it to the appropriate handling routine
* (v) make sure the lock level is set correctly for that operation
* (vi) don't forget to document the option
*
@@ -1106,9 +1105,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
switch (classForm->relkind)
{
case RELKIND_RELATION:
- case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- options = heap_reloptions(classForm->relkind, datum, false);
+ options = heap_reloptions(datum, false);
+ break;
+ case RELKIND_TOASTVALUE:
+ options = toast_reloptions(datum, false);
break;
case RELKIND_PARTITIONED_TABLE:
options = partitioned_table_reloptions(datum, false);
@@ -1480,55 +1481,62 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions.
+ * Option parsing definition for autovacuum. Used in toast and heap options.
+ */
+
+#define AUTOVACUUM_RELOPTIONS(OFFSET)\
+ {"autovacuum_enabled", RELOPT_TYPE_BOOL, \
+ OFFSET + offsetof(AutoVacOpts, enabled)},\
+ {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_threshold)}, \
+ {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, analyze_threshold)}, \
+ {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_delay)}, \
+ {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_limit)}, \
+ {"autovacuum_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_min_age)}, \
+ {"autovacuum_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_max_age)}, \
+ {"autovacuum_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_table_age)}, \
+ {"autovacuum_multixact_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_min_age)}, \
+
