Re: [PATCH] Store Extension Options

On 2014-03-13 11:15:56 -0400, Robert Haas wrote:
> On Thu, Mar 13, 2014 at 10:27 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2014-03-13 10:24:09 -0400, Tom Lane wrote:
> >> Andres Freund <andres@2ndquadrant.com> writes:
> >> > But security labels are a nice idea, will think about it. AFAICs there's
> >> > no builtin subdivision within the label for one provider which is a bit
> >> > of a shame but solvable. The biggest issue I see is that it essentially
> >> > seems to require that the provider is in
> >> > {shared,local}_preload_libraries? You can't restore into a server
> >> > otherwise afaics?
> >>
> >> Well, if you want to validate the settings then you pretty much have to
> >> require that in some form.
> >
> > If there were a CREATE SECURITY LABEL PROVIDER or something, with the
> > catalog pointing to a validator function, we wouldn't necessarily...
> 
> I seriously doubt that's going to work nicely.  Now you've implicitly
> introduced a dependency from every object that has a label to the
> label provider.  pg_dump is going to have to restore the validator
> function before it restores anything that has a label, and before that
> it's going to have to restore the languages used to create those
> validator functions, and those languages might themselves be labeled,
> either by that provider or by other providers.

Aren't pretty much all of those problems already solved because already
need to be able to order all this to dump the extension for a datatype
before the relation with a column of that type?

> Perhaps you could untangle that mess, but I'm disinclined to try
> because I can't see what real problem we're solving here.  Extension
> that just provide particular functions or datatypes can be loaded on
> demand, but those that change underlying system behavior need to be
> loaded by the postmaster, or at least at backend startup.

Why is adding an annotation to a table "changing the underlying system
behaviour"? There might be cases where it is, and those can easily
require having been loaded via s_p_l.

> We've tried to patch around that fact with GUCs and it seems to me that we've
> thoroughly destroyed validation in the process but without really
> buying ourselves much.

I think you're making a much bigger issue of GUC validation problems
than there is. It's perfectly possible to assign datatypes, check
functions et all to custom GUCs? And there's
EmitWarningsOnPlaceholders() to warn about unknown GUCs inside a
namespace.

>  There are five contrib modules that define
> custom variables: auth_delay, auto_explain, pg_stat_statements,
> sepgsql, and worker_spi.  auth_delay, worker_spi and
> pg_stat_statements have to be loaded at postmaster startup time, and
> you have to decide whether you want sepgsql at *initdb* time.

You forgot at least plpgsql. Which is already a good showcase why we
want this to be per database, not per cluster, i.e. not preloaded.

There's also pretty good reasons to use auto_explain at the session
level, because you otherwise can't look inside a function's plans.

> Maybe there are better examples outside the
> core distribution, but to me it's looking like the idea that you can
> add GUCs on the fly into individual sessions is a big fizz.

It seems to be on a somewhat odd warpath against against custom gucs ;)
. I've used the capability to do so *dozens* of times. What problems
have they actually caused?

Note that postgresql.conf is parsed long before we initiate
shared_preload_libraries et al are taking effect, so even if we'd
require libraries to be loaded before custom GUCs can be defined, we'd
need to create a entirely new mechanism of loading libraries for
it. With a very odd circularity, because to parse postgresql.conf you'd
need to have it parsed to load the libraries.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services