Re: [PATCH] Store Extension Options

Tom Lane escribió:
> =?ISO-8859-1?Q?Fabr=EDzio_de_Royes_Mello?= <fabriziomello(at)gmail(dot)com> writes:
> > You are correct. pg_dump export reloptions using "WITH" clause of CREATE
> > TABLE statement. I.e.:
>
> > CREATE TABLE foo (
> > )
> > WITH (autovacuum_enabled=false, bdr.do_replicate=false);
>
> > So if this statement checks for 'bdr' extension is loaded then in partial
> > restore it can be fail.
>
> I see absolutely *nothing* wrong with failing that command if bdr is not
> installed. For an analogy, if this table includes a column of type bar
> defined by some extension baz, we are certainly going to fail the
> CREATE TABLE if baz isn't installed.
>
> Now, if bdr is installed but the validation doesn't happen unless bdr
> is "loaded" in some sense, then that is an implementation deficiency
> that I think we can insist be rectified before this feature is accepted.

So, I spent some time on this patch the last couple of days to add some
validation. But after submitting it, it seems to me that there wasn't
as much consensus on how to handle validation than at first I thought.

So, the first and simplest way to go about this, of course, is just not
validate anything. This is what Fabrízio's patch does. So the table
owner can execute
ALTER TABLE mary SET (supercalifragilisticexpialidocious.umbrella_flight = 'hell yeah')
and that would be it. Whether a module makes use of this later or not,
is not that guy's problem. This is mostly what we do for GUCs, note, so
it's not exactly groundbreaking.

As a second possibility, my patch as posted allows one to register a
namespace. So pg_dump can do this:
SELECT pg_register_option_namespace('supercalifragilisticexpialidocious');
and then create the table just like the above ALTER TABLE. Note that
the variable name, and the value, are not checked until later. If a
module comes later and says "hey, I own that super- option namespace,
and I have option umbrella_flight but it's a boolean" (by calling
add_bool_reloption), that will raise an error. Note that in my patch as
posted, if you set the parameter umbrella_flight='better not' to an
index, but the parameter has only been defined for tables
(RELOPT_KIND_HEAP), it will be silently accepted. Also note that we can
add a function equivalent to EmitWarningOnPlaceholders (Andres' idea),
so that any unrecognized option will cause some noise and it can be
identified right away. Since only table owners can set options, this
seems more than good to me; it's not like table owners are going to mess
up by adding pointless options just for the heck of it.

A further possibility requires modules to also register options (not
only namespaces), and to validate each and every option as soon as it's
created. So if you try to set an option that has not been previously
registered by a module, that will raise an error right there. This
seems to be what Tom, Robert and Peter want. However, getting there
seems very laborious; apparently, we will need a new system catalog to
register option validators, for starters. We'll also need a way to load
the module whenever a table gets an option in a not-loaded module. (I
think this will fall off automatically if the validator is registered,
because when the validator is called, the module is loaded by the
system).

One slight problem with this is what to do with extensions that don't
provide any C code. Some use cases require options that can be set and
accessed only from SQL code.

My question here for the hackers community at large is this: are we okay
with implementing the second option I propose above? If we are, then I
will see about finalizing the patch and getting it committed. If we are
not, and we're determined that only the third option is acceptable, I
will jump out of this thread and stop wasting everyone's time.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services