Re: Parallel safety tagging of extension functions

On 2016-06-09 17:40:24 -0400, Robert Haas wrote:
> On Thu, Jun 9, 2016 at 4:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> >> On Sat, May 21, 2016 at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> Yes, let's fix it.  This will also take care of the questions about
> >>> whether the GIN/GIST opclass tweaks I made a few months ago require
> >>> module version bumps.
> >
> >> Tom, there's a patch for this at
> >> https://www.postgresql.org/message-id/574F091A.3050800@proxel.se which
> >> I think you should review, since you were the one who made the tweaks
> >> involved.  Any chance you can do that RSN?
> >
> > I've pushed this with some revisions to make the queries more
> > search-path-safe.  I'm not too happy with the safety of the queries
> > I see already present from the previous patches.  I think stuff
> > like this:
> >
> > UPDATE pg_proc SET proparallel = 's'
> > WHERE oid = 'min(citext)'::regprocedure;
> >
> > needs to be more like
> >
> > UPDATE pg_catalog.pg_proc SET proparallel = 's'
> > WHERE oid = 'min(citext)'::pg_catalog.regprocedure;
> 
> We could do that, but there's no guarantee that "min" or "citext"
> resolve correctly either, is there?  Basically, the search-path-safety
> of many of the scripts already in contrib looks pretty horrendous to
> me.  For example:
> 
> CREATE VIEW pg_buffercache AS
>         SELECT P.* FROM pg_buffercache_pages() AS P
>         (bufferid integer, relfilenode oid, reltablespace oid, reldatabase oid,
>          relforknumber int2, relblocknumber int8, isdirty bool, usagecount int2,
>          pinning_backends int4);
> 
> Well, what guarantee have we that we'll get the right
> pg_buffercache_pages() function?

Aren't both of the above guaranteed due to/* * Set up the search path to contain the target schema, then the schemas *
ofany prerequisite extensions, and nothing else.  In particular this * makes the target schema be the default creation
targetnamespace. * * Note: it might look tempting to use PushOverrideSearchPath for this, * but we cannot do that.  We
haveto actually set the search_path GUC in * case the extension script examines or changes it.  In any case, the *
GUC_ACTION_SAVEmethod is just as convenient. */initStringInfo(&pathbuf);appendStringInfoString(&pathbuf,
quote_identifier(schemaName));foreach(lc,requiredSchemas){    Oid            reqschema = lfirst_oid(lc);    char
*reqname= get_namespace_name(reqschema);
 
    if (reqname)        appendStringInfo(&pathbuf, ", %s", quote_identifier(reqname));}
(void) set_config_option("search_path", pathbuf.data,                         PGC_USERSET, PGC_S_SESSION,
         GUC_ACTION_SAVE, true, 0, false);
 
in extension.c:execute_extension_script()?

> CREATE FUNCTION earth() RETURNS float8
> LANGUAGE SQL IMMUTABLE PARALLEL SAFE
> AS 'SELECT ''6378168''::float8';
> 
> What guarantees we'll get the correct float8 type?
> 
> CREATE FUNCTION sec_to_gc(float8)
> RETURNS float8
> LANGUAGE SQL
> IMMUTABLE STRICT
> PARALLEL SAFE
> AS 'SELECT CASE WHEN $1 < 0 THEN 0::float8 WHEN $1/(2*earth()) > 1
> THEN pi()*earth() ELSE 2*earth()*asin($1/(2*earth())) END';

These aren't though.

Andres