Index over ST_Transform(constant, SRID) breaks pg_upgrade

[Algunenano]

As <subject> says, having an index which uses a ST_Transform(constant, XXXX) will break pg_upgrade.

Test case.

## Create the table

# create database test95;

# \c test95

# create extension postgis;

# create table t95 ( the_geom geometry );

# insert into t95 values ('SRID=4326; POINT(-133 40.2)'::geometry);

# CREATE INDEX "idx_transf" ON "public"."t95" USING "btree" ("st_intersects"("the_geom", "st_transform"('0101000020E61000000000000000A060C0FAEDEBC0395B4440'::"geometry", 3857)));

## Run pg_upgrade

I've tested this doing:

• PG9.5, postgis 2.2 → PG10, postgis 2.4
• PG10, postgis 2.4 → PG11, postgis 2.5

pg_restore: creating CONSTRAINT "public.spatial_ref_sys spatial_ref_sys_pkey"
pg_restore: creating INDEX "public.idx_transf"
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 3342; 1259 17766 INDEX idx_transf postgres
pg_restore: [archiver (db)] could not execute query: ERROR:  GetProj4StringSPI: Cannot find SRID (4326) in spatial_ref_sys
    Command was: 
-- For binary upgrade, must preserve pg_class oids
SELECT pg_catalog.binary_upgrade_set_next_index_pg_class_oid('17766'::pg_catalog.oid);

Any function that calls ST_Intersects is also affected, e.g. ST_Buffer(geography).

AFAIK, it affects all stable postgis releases, although I'd expect that if the destination installation has PROJ 6 it'll do fine as it likely won't need the lookup of spatial_ref_sys.

[Algunenano]

Speculation over IRC:

[19:07] <robe2> I guess maybe the index tries to evaluate the constant even though the table is empty
[19:08] <Algunenano> That's also what I think
[19:08] <robe2> though still I thought pg_upgrade would load all the data first before applying the index, so still doesn't make sense to me
[19:08] <Algunenano> As far as I can see, it first loads the schemas and then the data
[19:08] <Algunenano> I need to investigate further
[19:08] <robe2> and the index after?
[19:09] <Algunenano> Schema + bare extension + Index; then data
[19:10] <Algunenano> It loads postgis doing something like: `SELECT pg_catalog.binary_upgrade_create_empty_extension('postgis', 'public', false, '2.4.7', '{19728}', '{"WHERE NOT (`
[19:10] <Algunenano> It doesn't do a CREATE EXTENSION
[19:15] <robe2> Algunenano: yah it adds each bit directly to the extension
[19:15] <Algunenano> And it doesn't add the data to the srid table as far as I can tell
[19:16] <Algunenano> I haven't figured out yet when it's added
[19:16] <robe2> I asked Tom about that once and he said to preserve the existing extension stuff
[19:16] <robe2> so it does a naked extension and then adds the bits to it
[19:17] <robe2> but hmm in a regular extension install the data would be loaded as part of the create extension step
[19:17] <robe2> it might treat the actual data in the table like any other I suppose which would cause an order dependency
[19:18] <robe2> but still I think indexes get created at the end so should not be an issue
[19:18] <robe2> unless if that assumption is wrong
[19:20] <Algunenano> They seem to be created at the end, but before adding data
[19:20] <robe2> the indexes?
[19:20] <Algunenano> At least I don't see any data insertion in the dump created by pg_upgrade
[19:20] <Algunenano> Yes
[19:20] <robe2> hmm 
[19:20] <Algunenano> The dump done by pg_upgrade is `--schema-only`
[19:21] <robe2> okay guess that explains it
[19:21] <robe2> Algunenano: well technically we are breaking the contract of immutability
[19:22] <robe2> because the index depends on a function that is not immutable that we lied and said was
[19:22] <Algunenano> "I didn't sign no contract", but yes we are
[19:23] <robe2> Algunenano: but you are saying your spatial_ref_sys table then never loads and pg_upgrade gives no error?
[19:23] <-- jmarsac (~jmarsac@lfbn-nic-1-72-223.w2-15.abo.wanadoo.fr) ha dejado este servidor (Ping timeout: 272 seconds).
[19:23] <-- kapilp (uid36151@gateway/web/irccloud.com/x-boohfisijcwgqwib) ha dejado este servidor (Quit: Connection closed for inactivity).
[19:24] <Algunenano> No, pg_upgrade does error and stops loading. But since it doesn't clean up after that I can get in and see that there isn't any data in spatial_sys_ref
[19:24] <robe2> ah okay that all makes sense now
[19:24] <robe2> but ST_Transform works
[19:24] <robe2> I wonder if it does load the user data first
[19:25] <Algunenano> I'd say so
[19:25] <robe2> but since you have a constant calls the function

[Algunenano]

After looking into the code it appears that will also be present with PROJ 6 path as it also requires the projection to be available in spatial_ref_sys.

[robe]

I think this may be fixed already. I'll test in a bit and close out if I can't replicate issue.

[robe]

okay it's still an issue.

I tried upgrading from PostGIS 2.5.5 dev PG 12 to PG 13 PostGIS 2.5.5dev and got same error

pg_restore: from TOC entry 3507; 1259 54514 INDEX idx_transf postgres
pg_restore: error: could not execute query: ERROR:  GetProj4StringSPI: Cannot find SRID (4326) in spatial_ref_sys
Command was: 
-- For binary upgrade, must preserve pg_class oids
SELECT pg_catalog.binary_upgrade_set_next_index_pg_class_oid('54514'::pg_catalog.oid);

CREATE INDEX "idx_transf" ON "public"."t95" USING "btree" ("public"."st_intersects"("the_geom", "public"."st_transform"('0101000020E61000000000000000A060C0FAEDEBC0395B4440'::"public"."geometry", 3857)));

you think this is fixable. I haven't tried Proj 6 (this was testing with Proj 5.2). Seems like such a rare case and we have a workaround (just drop the index before you pg_upgrade)

[Algunenano]

I don't think this is fixable with our current spatial_ref_sys desisgn. To fix it in my migrations I've hardcoded the necessary SRID transformation strings inside Postgis, but that isn't a proper solution.

I think that ideally we would have a set of static SRIDs inside Postgis/PROJ (independent of spatial_ref_sys) that can't be modified by the end user and then allow having other SRIDS (as long as they don't collide with the static ones). This would mean a major change which I'm not sure we want to tackle.

[strk]

with [c1b8d14d8900dc835fd5782f364364ffa963919e/git] we have a an util/check_cluster_upgrade.sh script that can be useful to put this case under automated testing. I've tried it and it indeed fails with:

pg_restore: error: could not execute query: ERROR:  Cannot find SRID (4326) in spatial_ref_sys

When upgrading a cluster from 12 to 13

[Algunenano]

Not fixable as long as we keep spatial_ref_sys as the source of truth

[strk]

Is this due to spatial_ref_sys contents NOT being visible at the time the table using the index is created ?

[strk]

A reorganization of spatial_ref_sys might be an occasion to look at this, see #4987

[robe]

Replying to strk:

Is this due to spatial_ref_sys contents NOT being visible at the time the table using the index is created ?

Yes - NOT having data is the issue for pg_upgrade.

The issue is when pg_upgrade runs I think it restores all the structures first and the data later but for some reason it restores the indexes before the data. I think I complained about this on pg-hackers (thought I forget). The assumption is since all functions used in indexes are immutable, they should not rely on data in any tables. AS such it shouldn't matter the order you load the tables. In this case other tables get loaded first and fail the index check because the spatial_ref_sys table is empty. Actually I don't even think a table has to have data for it to fail the index check cause the index goes to check on the function it relies on to index empty data (I could be mistaken here).

Why they can't create indexes after loading data I don't understand. Perhaps because it might slow the build of materialized views. Have no clue.

I would think if they just changed the order of how they do things it would be fine.

[Algunenano]

A reorganization of spatial_ref_sys might be an occasion to look at this, see #4987

In this case, that reorganization would not fix the issue. As robe said, the problem is that the table is created, then the indexes, then the data is added.