Repository navigation
fix: run extension custom scripts with search_path set to pg_catalog - #273
utkarash2991 wants to merge 1 commit into
Conversation
Custom scripts now run with search_path set to pg_catalog, pg_temp, saved and restored through a GUC nest level, the same way core runs extension scripts. Adds @extdefaultschema@ so scripts can still tell where a schema-less create extension lands. Also qualifies the = operator in the pg_available_extensions lookup.
| appendStringInfo(&sql, | ||
| "select 1 from pg_catalog.pg_available_extensions where " | ||
| "name operator(pg_catalog.=) %s", | ||
| quote_literal_cstr(extname)); |
There was a problem hiding this comment.
I don't think this is strictly apart of the goal of this PR but a good catch nonetheless. Can we get follow up PR's to do the same schema qualification elsewhere, like below in pg_net (but also vault, pg_jsonschema, etc)? Unfortunately plpgsql_check does not catch stuff like this and its embedded in a C file so Postgres-language-server could not help even if they did support this functionality. I kind of want to just get AI to author a pgsql_check extension that just intercepts at runtime any SQL that does not explicitly qualify everything, as I dont think anything like that exists currently, for use in our CI systems.
https://github.com/supabase/pg_net/blob/1b8efcbe0305ae1ffd8e05e0d5e128e925e29e34/src/core.c#L189
There was a problem hiding this comment.
Make sense @AndrewJackson2020 . I will give it a more thought about what we can do more in this area
| └── before-create.sql | ||
| ``` | ||
|
|
||
| Scripts run with `search_path` set to `pg_catalog, pg_temp`, the same as core extension scripts, so schema-qualify anything a script references outside `pg_catalog`. The caller's `search_path` is restored once the script finishes. A session-level `set_config(..., false)` done by a script still persists. |
There was a problem hiding this comment.
what does "the same as core extension scripts" refer to here?
| supautils.reserved_roles='supabase_storage_admin, anon, reserved_but_not_yet_created, authenticator*' | ||
| supautils.reserved_memberships='pg_read_server_files,pg_write_server_files,pg_execute_server_program,role_with_reserved_membership' | ||
| supautils.privileged_extensions='autoinc, citext, hstore, sslinfo, insert_username, dict_xsyn, postgres_fdw, pageinspect, plls, no_control_file_extension, dblink, ltree, unaccent, tablefunc, tcn' | ||
| supautils.privileged_extensions='autoinc, citext, hstore, sslinfo, insert_username, dict_xsyn, postgres_fdw, pageinspect, plls, no_control_file_extension, dblink, ltree, unaccent, tablefunc, tcn, amcheck' |
There was a problem hiding this comment.
I find this pattern annoying of adding a new extension install every time we want to test new functionality but I dont have a better alternative right now.
| // execute_extension_script. saved and restored via a GUC nest level so on | ||
| // error the change is rolled back with the transaction | ||
| int save_nestlevel = NewGUCNestLevel(); | ||
| (void)set_config_option("search_path", "pg_catalog, pg_temp", PGC_USERSET, |
There was a problem hiding this comment.
Question: is pg_catalog is implicitly added at the left side if not present, do we actually need this here? Should we keep that here for the sake of being explicit?
There was a problem hiding this comment.
When postgres sanitizes it seems like it just removes pg_catalog from the search path (if present) and then appends pg_temp at the end.
|
So basically in the extension creation code path we now set search path potentially 5 times: 4 times for each potential before/after file (global and individual) and then once for the actual extension creation. Question: could we not just sanitize the search path prior to extension creation? Create a str buffer, loop through the path, remove any instances of pg_catalog and pg_temp, append pg_temp and the and prepend the default in front, something like that. Is that a bad idea because we can't trust what is the global default schema? |
Title: fix: run extension custom scripts with search_path set to pg_catalog
Custom scripts ran under the caller's search_path. They now run with search_path set to
pg_catalog, pg_temp, saved and restored through a GUC nest level, the same way core runs extension scripts. The caller's search_path is restored when the script finishes. A session-level set_config in a script still persists.Scripts can no longer read the caller's search_path, so this adds
@extdefaultschema@: the schema a schema-less create extension lands in, resolved before the switch with the same lookup core uses. Scripts that create dependencies, like our global before-create script, should use it.Objects a script creates without a schema now land in pg_catalog. The fuzzystrmatch test fixture is updated to qualify its tables.
Also qualifies the
=operator in the pg_available_extensions lookup.