Re: BUG #15112: Unable to run pg_upgrade with earthdistance extension

On Fri, Mar 30, 2018 at 04:52:29PM -0400, Bruce Momjian wrote:
> On Wed, Mar 14, 2018 at 11:12:26PM +0000, PG Bug reporting form wrote:
> > The following bug has been logged on the website:
> > 
> > Bug reference:      15112
> > Logged by:          Keiko Oda
> > Email address:      keiko713@gmail.com
> > PostgreSQL version: 10.3
> > Operating system:   Ubuntu
> > Description:        
> > 
> > This is similar to the bug report from 2016
> > (https://www.postgresql.org/message-id/20161015001424.1413.93990@wrigleys.postgresql.org).
> > where earthdistance extension is assuming that it's executed with public
> > schema in search_path.
> > 
> > With 10.3 release, Postgres has tighten the search_path during pg_upgrade
> > only to pg_catalog. This is problematic as it's now impossible to run
> > pg_upgrade with earthdistance extension (it fails with create index with
> > ll_to_earth function).
> > Prior to this, we were at least able to workaround by adding public schema
> > in search_path with pg_upgrade.
> > 
> > As it's recommended in the release note, earthdistance should adjust these
> > functions to not assume anything about what search path they are invoked
> > under.
> 
> Uh, I can reproduce this failure.  :-(
> 
> I tested it by installing earchdistance (and cube) and an index using an
> earchdistance function in the old cluster and ran pg_upgrade.  I used
> the instructions in the referenced email:
> 
>     CREATE TABLE zip_code_geo_poly_data (
>         id serial   NOT NULL PRIMARY KEY,
>         zip_code    TEXT,
>         latitude    NUMERIC,
>         longitude   NUMERIC
>     );
>     
>     CREATE INDEX zip_code_geo_poly_data_ll_to_earth 
>     ON zip_code_geo_poly_data 
>     USING gist(ll_to_earth(latitude, longitude));
> 
> The failure is actually in pg_dump/restore, but pg_upgrade relies on
> that, so it fails too.  The failure output is:
> 
>     LINE 1: ...ians($1))*sin(radians($2))),earth()*sin(radians($1)))::earth
>                                                                       ^
>     QUERY:  SELECT
cube(cube(cube(earth()*cos(radians($1))*cos(radians($2))),earth()*cos(radians($1))*sin(radians($2))),earth()*sin(radians($1)))::earth
>     CONTEXT:  SQL function "ll_to_earth" during inlining
>         Command was:
>     -- For binary upgrade, must preserve pg_class oids
>     SELECT pg_catalog.binary_upgrade_set_next_index_pg_class_oid('16498'::pg_catalog.oid);
>     
>     CREATE INDEX "zip_code_geo_poly_data_ll_to_earth" ON "public"."zip_code_geo_poly_data" USING "gist"
("public"."ll_to_earth"(("latitude")::doubleprecision, ("longitude")::double precision));
 
> 
> The function definition of ll_to_earth() is (as defined in
> earthdistance--1.1.sql):
> 
>     CREATE FUNCTION ll_to_earth(float8, float8)
>     RETURNS earth
>     LANGUAGE SQL
>     IMMUTABLE STRICT
>     PARALLEL SAFE
>     AS 'SELECT
cube(cube(cube(earth()*cos(radians($1))*cos(radians($2))),earth()*cos(radians($1))*sin(radians($2))),earth()*sin(radians($1)))::earth';
> 
> As you can see, the SQL function is doing a cast to ::earth, but in this
> test case the earth cast is stored in the public schema, and the restore
> doesn't reference it.
> 
> I am not sure we can fix this without requiring people to drop and
> recreate such indexes.  However, I am even at a loss in how to fix the
> CREATE FUNCTION to reference a cast in the same schema as the function,
> in this case 'public'.  We can rewrite the cast to not use :: and use a
> function call with schema qualification. e.g. public.earth(), but how do
> we know what schema that is in, i.e. what if the extension is loaded
> into a schema other than public?  
> 
> FYI, earthdistance is certainly not the only case of this problem.  
> 
> This is also part of a larger problem with our new schema qualification
> approach.  While we are qualifying where the object is to be created, we
> are not always qualifying the object's contents, i.e. in this case, the
> SQL function body.

I have not received a reply to this so I am CCing the hackers list for
more input.  (If this is not the proper procedure, please let me know.)

The only additional thoughts I have are that when we removed public from
pg_dump's search_path and decided to instead fully qualify object
creation names, I was concerned about function creation failing because
of 'public' schema object references in the function body, but someone
pointed out that we don't check function bodies during restore because
of:

    SET check_function_bodies = false;

However, what I did not consider was creation of indexes based on
function bodies with 'public' schema references.

I have ran some tests and found out that only SQL functions cause such
failures.  I believe this is because they are inlined during CREATE
INDEX.  The attached pg_dump output file recreates the failure, and
shows that prefixing it with "public" avoids the failure, and shows that
indexing of a PL/pgSQL function doesn't fail (see SQL comments).

A more general issue with functions is that if you schema-qualify a call
to a function, you will still need all object references in the function
to reference the schema where the object is installed if they are not in
search_path, i.e. schema qualifying the object doesn't propagate that
schema qualification into the function body (functions look in
search_path, not necessarily in the schema in which they are defined).

I am not sure what to suggest, but I don't think we can ignore this
problem.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +