Re: Parallel safety tagging of extension functions

On Thu, Jun 9, 2016 at 5:45 PM, Andres Freund <andres@anarazel.de> wrote:
> 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
>          * 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()?

Hmm.  Yeah, that helps.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company