Re: Parallel safety tagging of extension functions

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

Hmm. Yeah, that helps.

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