Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement

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.

Re: generic reloptions improvement

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.

Re: generic reloptions improvement

Re: generic reloptions improvement

ITAGAKI Takahiro wrote:

> Alvaro Herrera  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.

Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement

Alvaro Herrera wrote:
> Tom Lane wrote:
> > Alvaro Herrera  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.

Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement

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

Re: generic reloptions improvement

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

Re: generic reloptions improvement

Re: generic reloptions improvement

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

Re: generic reloptions improvement

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.

Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement

Re: generic reloptions improvement