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(at)gmail(dot)com
> > PostgreSQL version: 10.3
> > Operating system: Ubuntu
> > Description:
> >
> > This is similar to the bug report from 2016
> > (/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")::double precision, ("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(at)momjian(dot)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 +