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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> On Sat, May 21, 2016 at 11:45 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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
* of any prerequisite extensions, and nothing else. In particular this
* makes the target schema be the default creation target namespace.
*
* Note: it might look tempting to use PushOverrideSearchPath for this,
* but we cannot do that. We have to actually set the search_path GUC in
* case the extension script examines or changes it. In any case, the
* GUC_ACTION_SAVE method 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