Thread: generic reloptions improvement
Hi, Here's a patch for improving the general reloptions mechanism. What this patch does is add a table-based option parser. This allows adding new options very easily, and stops the business of having to pass the minimum and default fillfactor each time you want the reloptions processed. (This approach would not scale very well; each new reloption requires more parameters to default_reloptions, and at the same time we are forcing external AMs to support fillfactor, which they may very well do not. For example GIN was already passing useless values pointlessly.) I kept StdRdOptions as a fixed struct, which fixes the previous complain about speed. The new code parses the array and stores the values into the fixed struct. The only new thing in this area is that default_reloptions has to walk the returned array of relopt_gen to store the values in the struct. So in order to add a new option, it is necessary to patch both the options table (intRelOpts, etc) *and* default_reloptions. This is a bit ugly but it's the only way I found to keep both generality and speed. Right now, external AMs cannot do anything much apart from fillfactor, but it is very simple to add a routine to register a new "reloption kind" and another to register options in the table. That and a new *_reloptions routine (which needs to be registered in pg_am) would allow an AM to create whatever options it needs. Note that the only types supported are int, bool, real. We don't support strings. I don't see this as a problem, but shout if you disagree. (In the current patch, the bool and real lists are empty. The autovacuum patch adds items to both.) The patch to add the autovacuum options on top of this is very light on reloptions.c but heavy on lots of other places, which is why I'm submitting this separately. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Attachment
A small correction to this patch: this is needed because otherwise the autovac code to parse the option becomes all tangled; this avoids having to invent special values for "use the default value", and it also avoid having the default value stored elsewhere in the code than in the reloptions table. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Attachment
Hi, I have a comment about the generic-reloptions patch.
Alvaro Herrera <alvherre@commandprompt.com> wrote:
> Here's a patch for improving the general reloptions mechanism. What
> this patch does is add a table-based option parser. This allows adding
> new options very easily, and stops the business of having to pass the
> minimum and default fillfactor each time you want the reloptions
> processed.
You use struct relopt_gen (and its subclasses) for the purpose of
both "definition of options" and "parsed result". But I think
it is cleaner to separete parsed results into another struct
something like:
struct relopt_value
{ const relopt_gen *which; bool isset; union { bool val_bool; int val_int; double
val_real; } types;
};
the ariables 'isset' and 'parsed_val' are not used in definition, AFAICS.
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
ITAGAKI Takahiro wrote: > Alvaro Herrera <alvherre@commandprompt.com> wrote: > > > Here's a patch for improving the general reloptions mechanism. What > > this patch does is add a table-based option parser. This allows adding > > new options very easily, and stops the business of having to pass the > > minimum and default fillfactor each time you want the reloptions > > processed. > > You use struct relopt_gen (and its subclasses) for the purpose of > both "definition of options" and "parsed result". But I think > it is cleaner to separete parsed results into another struct > something like: Thanks for the suggestion -- yes, it is better as you suggest. I think putting the default on the same struct was just out of laziness at first, and inertia later. Here's the next version, which also fixes some particularly embarrasing bugs. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Attachment
On Monday 22 December 2008 18:24:53 Alvaro Herrera wrote: > > Alvaro Herrera <alvherre@commandprompt.com> wrote: > > > Here's a patch for improving the general reloptions mechanism. What > > > this patch does is add a table-based option parser. This allows adding > > > new options very easily, and stops the business of having to pass the > > > minimum and default fillfactor each time you want the reloptions > > > processed. > Here's the next version, which also fixes some particularly embarrasing > bugs. I'm not sure how important this is, but if you are enumerating the access methods (RELOPT_KIND_BTREE, etc.), how will this work with user-defined access methods?
Peter Eisentraut wrote: > I'm not sure how important this is, but if you are enumerating the access > methods (RELOPT_KIND_BTREE, etc.), how will this work with user-defined > access methods? It is important. I'm intending to have a new routine which would reserve a value at runtime. This value would be later be passed by the AM to create new options on the table. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Peter Eisentraut wrote:
>> I'm not sure how important this is, but if you are enumerating the access
>> methods (RELOPT_KIND_BTREE, etc.), how will this work with user-defined
>> access methods?
> It is important.
> I'm intending to have a new routine which would reserve a value at
> runtime. This value would be later be passed by the AM to create new
> options on the table.
What do you mean by "at runtime"? Surely the value would have to remain
stable across database restarts, since it's going to relate to stuff
that is in catalog entries.
I'd feel more comfortable with a scheme that used the AM's pg_am OID.
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > I'm intending to have a new routine which would reserve a value at > > runtime. This value would be later be passed by the AM to create new > > options on the table. > > What do you mean by "at runtime"? Surely the value would have to remain > stable across database restarts, since it's going to relate to stuff > that is in catalog entries. No, there's no need for the value to be stable across restart; what's stored in catalogs is the option name, which is linked to the kind number only in the parser table. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Tom Lane wrote: > > Alvaro Herrera <alvherre@commandprompt.com> writes: > > > > I'm intending to have a new routine which would reserve a value at > > > runtime. This value would be later be passed by the AM to create new > > > options on the table. > > > > What do you mean by "at runtime"? Surely the value would have to remain > > stable across database restarts, since it's going to relate to stuff > > that is in catalog entries. > > No, there's no need for the value to be stable across restart; what's > stored in catalogs is the option name, which is linked to the kind > number only in the parser table. So this is an updated patch. This now allows a user-defined AM to create new reloptions and pass them down to the parser for parsing and checking. I also attach a proof-of-concept patch that adds three new options to btree (which do nothing apart from logging a message at insert time). This patch demonstrates the coding pattern that a user-defined AM should follow to add and use new storage options. The main thing I find slightly hateful about this patch is that the code to translate from the returned relopt_value array and the fixed struct is rather verbose; and that the AM needs to duplicate the code in default_reloptions. I don't find it ugly enough to warrant objecting to the patch as a whole however. The neat thing about this code is that the parsing and searching is done only once, when the relcache entry is loaded. Later accesses to the option values themselves is just a struct access, and thus plenty quick. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Attachment
Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>> Tom Lane wrote:
>>> Alvaro Herrera <alvherre@commandprompt.com> writes:
>>>> I'm intending to have a new routine which would reserve a value at
>>>> runtime. This value would be later be passed by the AM to create new
>>>> options on the table.
>>> What do you mean by "at runtime"? Surely the value would have to remain
>>> stable across database restarts, since it's going to relate to stuff
>>> that is in catalog entries.
>> No, there's no need for the value to be stable across restart; what's
>> stored in catalogs is the option name, which is linked to the kind
>> number only in the parser table.
>
> So this is an updated patch. This now allows a user-defined AM to
> create new reloptions and pass them down to the parser for parsing and
> checking. I also attach a proof-of-concept patch that adds three new
> options to btree (which do nothing apart from logging a message at
> insert time). This patch demonstrates the coding pattern that a
> user-defined AM should follow to add and use new storage options.
>
> The main thing I find slightly hateful about this patch is that the code
> to translate from the returned relopt_value array and the fixed struct
> is rather verbose; and that the AM needs to duplicate the code in
> default_reloptions. I don't find it ugly enough to warrant objecting to
> the patch as a whole however.
>
> The neat thing about this code is that the parsing and searching is done
> only once, when the relcache entry is loaded. Later accesses to the
> option values themselves is just a struct access, and thus plenty quick.
This patch does not support reloptions in string expression, like:
CREATE TABLE t1 ( a int, b text ) WITH (default_row_acl='{yamada=r/kaigai}');
Do you have any plan to support reloptions in string?
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
On Tue, 2008-12-30 at 12:31 -0300, Alvaro Herrera wrote: > Alvaro Herrera wrote: > > Tom Lane wrote: > > > Alvaro Herrera <alvherre@commandprompt.com> writes: > > > > > > I'm intending to have a new routine which would reserve a value at > > > > runtime. This value would be later be passed by the AM to create new > > > > options on the table. > > > > > > What do you mean by "at runtime"? Surely the value would have to remain > > > stable across database restarts, since it's going to relate to stuff > > > that is in catalog entries. > > > > No, there's no need for the value to be stable across restart; what's > > stored in catalogs is the option name, which is linked to the kind > > number only in the parser table. > > So this is an updated patch. This now allows a user-defined AM to > create new reloptions and pass them down to the parser for parsing and > checking. I also attach a proof-of-concept patch that adds three new > options to btree (which do nothing apart from logging a message at > insert time). This patch demonstrates the coding pattern that a > user-defined AM should follow to add and use new storage options. > > The main thing I find slightly hateful about this patch is that the code > to translate from the returned relopt_value array and the fixed struct > is rather verbose; and that the AM needs to duplicate the code in > default_reloptions. I don't find it ugly enough to warrant objecting to > the patch as a whole however. > > The neat thing about this code is that the parsing and searching is done > only once, when the relcache entry is loaded. Later accesses to the > option values themselves is just a struct access, and thus plenty quick. I very much like the idea of adding new/custom options to tables. There are many uses for that. What you have here looks fairly hard to program for normal users. I don't want to reject the feature, but the proposal you have here isn't the best it could be... Can we have something like customer variable classes, but just for reloptions? e.g. WITH (mymodule.my_option_name = X) e.g. WITH (funky_trigger.coolness = 25) We can then create new custom reloptions in roughly the same way we can create custom variable classes, or ignore them if module not loaded. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
KaiGai Kohei wrote: > Alvaro Herrera wrote: > > So this is an updated patch. This now allows a user-defined AM to > > create new reloptions and pass them down to the parser for parsing and > > checking. > > This patch does not support reloptions in string expression, like: No, it doesn't. I asked about it some time ago and nobody answered. If people feel it is important, I can implement it. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Simon Riggs wrote: > I very much like the idea of adding new/custom options to tables. There > are many uses for that. Hmm, like what? I'm not sure how would it work for tables; you'd have to add the options during _PG_init or something like that, and I haven't tried it. It's mainly for user-defined AMs that this was done. > What you have here looks fairly hard to program for normal users. I > don't want to reject the feature, but the proposal you have here isn't > the best it could be... Agreed ... > Can we have something like customer variable classes, but just for > reloptions? > > e.g. WITH (mymodule.my_option_name = X) > e.g. WITH (funky_trigger.coolness = 25) > > We can then create new custom reloptions in roughly the same way we can > create custom variable classes, or ignore them if module not loaded. I'm now playing with adding "namespaces" to the options, but that's for handling options for toast tables. I'm not really sure how would it work for regular options. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
On Sat, 2009-01-03 at 12:20 -0300, Alvaro Herrera wrote: > Simon Riggs wrote: > > > I very much like the idea of adding new/custom options to tables. There > > are many uses for that. > > Hmm, like what? I'm not sure how would it work for tables; you'd have > to add the options during _PG_init or something like that, and I haven't > tried it. It's mainly for user-defined AMs that this was done. I understand and agree with your intended use. I see others as well and would like to cater for them all in a generic way that will have many uses over next 10-20 years, many of which I haven't thought of yet. Custom variable classes are often useful, but they are system wide. It would be good to be able to use table-level options and have them work very similarly to something we already have. Table-level options are just an obvious "normalisation" of how we handle parameters. If you really can't see a use for this, OK, then: Please can you put in a plugin API for user defined reloptions as well as what you are proposing. We discussed this before in late July/early Aug on thread "Uncopied parameters..." > > Can we have something like customer variable classes, but just for > > reloptions? > > > > e.g. WITH (mymodule.my_option_name = X) > > e.g. WITH (funky_trigger.coolness = 25) > > > > We can then create new custom reloptions in roughly the same way we can > > create custom variable classes, or ignore them if module not loaded. > > I'm now playing with adding "namespaces" to the options, but that's for > handling options for toast tables. I'm not really sure how would it > work for regular options. toast.option_x btree.option_y autovacuum.option_z -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Alvaro Herrera wrote: > KaiGai Kohei wrote: >> Alvaro Herrera wrote: > >>> So this is an updated patch. This now allows a user-defined AM to >>> create new reloptions and pass them down to the parser for parsing and >>> checking. >> This patch does not support reloptions in string expression, like: > > No, it doesn't. I asked about it some time ago and nobody answered. If > people feel it is important, I can implement it. Oh, I missed to see the message. If it is provided for v8.4, I'm happy at least. The Row-level ACLs need its reloption to specify default ACLs in string expression. Currently, it modifies "reloptions.c", but using it on common framework will be more appropriate implementation. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai Kohei wrote: > Alvaro Herrera wrote: >> No, it doesn't. I asked about it some time ago and nobody answered. If >> people feel it is important, I can implement it. > > Oh, I missed to see the message. > > If it is provided for v8.4, I'm happy at least. > The Row-level ACLs need its reloption to specify default ACLs in string > expression. Currently, it modifies "reloptions.c", but using it on common > framework will be more appropriate implementation. Ok, I see it now. I will have to implement string reloptions then. Thanks for the notice. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Simon Riggs wrote: > If you really can't see a use for this, OK, then: Please can you put in > a plugin API for user defined reloptions as well as what you are > proposing. We discussed this before in late July/early Aug on thread > "Uncopied parameters..." Hmm, I was just looking at the CREATE TABLE LIKE code yesterday; I didn't remember that discussion. I'll have a more detailed look. > > I'm now playing with adding "namespaces" to the options, but that's for > > handling options for toast tables. I'm not really sure how would it > > work for regular options. > > toast.option_x > btree.option_y > autovacuum.option_z autovacuum as a namespace doesn't work, because we need to have autovacuum options for toast too. If we went down this route we would need to have two name levels. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
KaiGai Kohei wrote: > If it is provided for v8.4, I'm happy at least. > The Row-level ACLs need its reloption to specify default ACLs in string > expression. Currently, it modifies "reloptions.c", but using it on common > framework will be more appropriate implementation. Modified to add a string type. Note that the real difficulty is what to do with the string in default_reloptions (or the amoptions routine). I see that your patch has already dealt with that, so it should be pretty easy for you; for any reloption that wants to be stored in rel->rd_options, it will be considerably more difficult (due to memory allocation). Some notes about this patch: - the string type handling (basically all the new code) is untested. I'll have a look tomorrow at the btree test code I sent the other day to add a string option and see how it goes. - I have added some macros to deal with options in the most common scenario, which is that they get stored in a predefined struct. This hides part of the complexity in writing an amoptions routine. - there's no way to define custom reloptions as requested by Simon. I don't have any ideas on how to do that at this time. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachment
Alvaro Herrera wrote:
> Some notes about this patch:
>
> - the string type handling (basically all the new code) is untested.
> I'll have a look tomorrow at the btree test code I sent the other day to
> add a string option and see how it goes.
Okay, it was basically fine except for the attached minor correction.
Warning: I intend to commit this patch fairly soon!
As far as I can see, the new code can work with the options you've
defined in the SEPgsql code just fine. Handling string options in
itself is fine; the complexity (as I already said) is in allocating
memory for the string if you want to store it unchanged in the bytea
stuff in relcache. Since you're not storing the string itself but
convert it to an Oid, there's no problem.
Actually, storing the string itself works fine as long as you have a
single one, because you can define the option struct like this:
/* must follow StdRdOptions conventions */
typedef struct BtOptions
{
int32 vl_len_;
int fillfactor;
char teststring[1];
} BtOptions;
and there are no pointers involved. This doesn't work:
typedef struct BtOptions
{
int32 vl_len_;
int fillfactor;
char *teststring;
} BtOptions;
because then there's a pointer, and it fails as soon as the bytea * is
copied by the relcache code.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachment
Simon Riggs wrote: > Custom variable classes are often useful, but they are system wide. It > would be good to be able to use table-level options and have them work > very similarly to something we already have. Table-level options are > just an obvious "normalisation" of how we handle parameters. > > If you really can't see a use for this, OK, then: Please can you put in > a plugin API for user defined reloptions as well as what you are > proposing. We discussed this before in late July/early Aug on thread > "Uncopied parameters..." I've been giving this a thought and I don't see any easy way to handle it. Since I've been threatened that this whole thing may be punted for 8.5 if I'm not quick about it, I've left this alone for now, and will concentrate on getting the namespace thing done which will allow specifying reloptions for the toast table by an ALTER TABLE on the main table. It's not that I don't see a use for it; it's just that I don't have the time for it right now. (Note: I think there are ways to do this; they'll involve storing the unknown options as a text array. It won't be pretty or performant, but it seems the only way around the fact that heap_reloptions comes hardcoded with the system.) -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Okay, it was basically fine except for the attached minor correction. > Warning: I intend to commit this patch fairly soon! This is the patch in its final form. I have included a few macros to simplify the writing of amoptions routines. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachment
On Sun, Jan 4, 2009 at 15:01, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Alvaro Herrera wrote: > >> Okay, it was basically fine except for the attached minor correction. >> Warning: I intend to commit this patch fairly soon! > > This is the patch in its final form. I have included a few macros to > simplify the writing of amoptions routines. Looks good to me, I just used it to whip up a patch to control some toast compression knobs..
Alvaro Herrera <alvherre@commandprompt.com> writes:
> This is the patch in its final form. I have included a few macros to
> simplify the writing of amoptions routines.
Minor gripes:
* Does initialize_reloptions() need to be exported? It seems to be
only called within parseRelOptions(). It's far from clear who else
should be expected to call it.
* The HANDLE_ macros are dangerous as-is (dangling if/else). Need to
use the usual do/while trick.
regards, tom lane
Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>
>> Okay, it was basically fine except for the attached minor correction.
>> Warning: I intend to commit this patch fairly soon!
>
> This is the patch in its final form. I have included a few macros to
> simplify the writing of amoptions routines.
Thanks for your efforts!
However, I could find a few issues about string reloptions.
(1) Who/Where should allocate a string area?
+ /* Note that this assumes that the variable is already allocated! */
+ #define HANDLE_STRING_RELOPTION(optname, var, option) \
+ if (HAVE_RELOPTION(optname, option)) \
+ { \
+ strcpy(var, \
+ option.isset ? option.values.string_val : \
+ ((relopt_string *) option.gen)->default_val); \
+ continue; \
+ }
I think HANDLE_STRING_RELOPTION() should allocate a string area via
pstrdup(). If we have to put individual pstrdup() for each reloptions,
it will make noisy listing of codes.
How do you think:
#define HANDLE_STRING_RELOPTION(optname, var, option) \ if (HAVE_RELOPTION(optname, option))
\ { \ char *tmp = (option.isset ?
option.values.string_val: \ ((relopt_string *) option.gen)->default_val); \ var =
pstrdup(tmp); \ continue; \
}
(2) How does it represent NULL in string_option?
It seems to me we cannot represent a NULL string in the default.
Is it possible to add a mark to indicate NULL, like "bool default_null"
within struct relopt_string?
(3) heap_reloptions() from RelationParseRelOptions() makes a trouble.
It invokes heap_reloptions() under CurrentMemoryContext, and its result
is copied in CacheMemoryContext later, if the result is not NULL.
But it makes a matter when StdRdOptions contains a pointer.
If a string allocated under CurrentMemoryContext, target of the copied
pointer is not valid on the next query execution, even if StdRdOptions
is valid because it is copied to another memoery context.
I think RelationParseRelOptions() should be patched as follows:
/* Parse into appropriate format; don't error out here */
+ oldctx = MemoryContextSwitchTo(CacheMemoryContext); switch (relation->rd_rel->relkind) { case
RELKIND_RELATION: case RELKIND_TOASTVALUE: case RELKIND_UNCATALOGED: options =
heap_reloptions(relation->rd_rel->relkind,datum, false); break;
caseRELKIND_INDEX: options = index_reloptions(relation->rd_am->amoptions, datum,
false); break; default: Assert(false); /* can't get here */ options =
NULL; /* keep compiler quiet */ break; }
+ MemoryContextSwitchTo(oldctx);
- /* Copy parsed data into CacheMemoryContext */
- if (options)
- {
- relation->rd_options = MemoryContextAlloc(CacheMemoryContext,
- VARSIZE(options));
- memcpy(relation->rd_options, options, VARSIZE(options));
- }
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote:
> (1) Who/Where should allocate a string area?
>
> + /* Note that this assumes that the variable is already allocated! */
> + #define HANDLE_STRING_RELOPTION(optname, var, option) \
> + if (HAVE_RELOPTION(optname, option)) \
> + { \
> + strcpy(var, \
> + option.isset ? option.values.string_val : \
> + ((relopt_string *) option.gen)->default_val); \
> + continue; \
> + }
>
> I think HANDLE_STRING_RELOPTION() should allocate a string area via
> pstrdup(). If we have to put individual pstrdup() for each reloptions,
> it will make noisy listing of codes.
>
> How do you think:
>
> #define HANDLE_STRING_RELOPTION(optname, var, option) \
> if (HAVE_RELOPTION(optname, option)) \
> { \
> char *tmp = (option.isset ? option.values.string_val : \
> ((relopt_string *) option.gen)->default_val); \
> var = pstrdup(tmp); \
> continue; \
> }
Well, that's precisely the problem with string options. If we want
memory to be freed properly, we can only allocate a single chunk which
is what's going to be stored under the rd_options bytea pointer.
Allocating separately doesn't work because we need to rebuild the
relcache entry (freeing it and allocating a new one) when it is
invalidated for whatever reason. Since the relcache code cannot follow
a pointer stored in the bytea area, this would result in a permanent
memory leak.
So the rule I came up with is that the caller is responsible for
allocating it -- but it must be inside the bytea area to be returned.
Below is a sample amoptions routine to show how it works. Note that
this is exactly why I said that only a single string option can be
supported.
If you have a better idea, I'm all ears.
> (2) How does it represent NULL in string_option?
>
> It seems to me we cannot represent a NULL string in the default.
> Is it possible to add a mark to indicate NULL, like "bool default_null"
> within struct relopt_string?
Ah, good point. I'll have a look at this.
> (3) heap_reloptions() from RelationParseRelOptions() makes a trouble.
This is the same as (1) actually.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Attachment
Alex Hunsaker escribió: > On Sun, Jan 4, 2009 at 15:01, Alvaro Herrera <alvherre@commandprompt.com> wrote: > > Alvaro Herrera wrote: > > > >> Okay, it was basically fine except for the attached minor correction. > >> Warning: I intend to commit this patch fairly soon! > > > > This is the patch in its final form. I have included a few macros to > > simplify the writing of amoptions routines. > > Looks good to me, I just used it to whip up a patch to control some > toast compression knobs.. Excellent, thanks for testing :-) -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote:
> KaiGai Kohei wrote:
>
>> (1) Who/Where should allocate a string area?
>>
>> + /* Note that this assumes that the variable is already allocated! */
>> + #define HANDLE_STRING_RELOPTION(optname, var, option) \
>> + if (HAVE_RELOPTION(optname, option)) \
>> + { \
>> + strcpy(var, \
>> + option.isset ? option.values.string_val : \
>> + ((relopt_string *) option.gen)->default_val); \
>> + continue; \
>> + }
>>
>> I think HANDLE_STRING_RELOPTION() should allocate a string area via
>> pstrdup(). If we have to put individual pstrdup() for each reloptions,
>> it will make noisy listing of codes.
>>
>> How do you think:
>>
>> #define HANDLE_STRING_RELOPTION(optname, var, option) \
>> if (HAVE_RELOPTION(optname, option)) \
>> { \
>> char *tmp = (option.isset ? option.values.string_val : \
>> ((relopt_string *) option.gen)->default_val); \
>> var = pstrdup(tmp); \
>> continue; \
>> }
>
> Well, that's precisely the problem with string options. If we want
> memory to be freed properly, we can only allocate a single chunk which
> is what's going to be stored under the rd_options bytea pointer.
> Allocating separately doesn't work because we need to rebuild the
> relcache entry (freeing it and allocating a new one) when it is
> invalidated for whatever reason. Since the relcache code cannot follow
> a pointer stored in the bytea area, this would result in a permanent
> memory leak.
>
> So the rule I came up with is that the caller is responsible for
> allocating it -- but it must be inside the bytea area to be returned.
> Below is a sample amoptions routine to show how it works. Note that
> this is exactly why I said that only a single string option can be
> supported.
If the caller allocates a surplus area to store string on tail of the
StdRdOptions (or others), the string option can be represented as an
offset value and should be accessed via macros, like:
struct StdRdOptions
{ int32 vl_len_; int fillfactor; int test_option_a; /* indicate offset of the surplus area
fromhead */ int test_option_b; /* of this structure. */ /* in actually surplus
areais allocated here */
};
#define GetOptionString(ptr, ofs) (ofs==0 ? NULL : ((char *)(ptr) + (ptr)->(ofs)))
We can access string options as follows:
elog(NOTICE, "test_option_a is %s", GetOptionString(RdOpts, test_option_a)); elog(NOTICE, "test_option_b is %s",
GetOptionString(RdOpts,test_option_b));
It enables to store multiple string options, and represent NULL string.
If possible, HANDLE_STRING_RELOPTION() should be able to manage the head of unused
surplus area and put the given val its offset automatically.
I think using a macro to access string option is more proper restriction than
mutually exclusive string options.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote: > If the caller allocates a surplus area to store string on tail of the > StdRdOptions (or others), the string option can be represented as an > offset value and should be accessed via macros, like: Excellent. I thought about storing an offset but I didn't know how to make it work for accessors -- I was missing the macro idea :-) Thanks, I'll see about this. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support