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.

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.

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.

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.com
PostgreSQL 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.com
PostgreSQL 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

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

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.