Opened 16 months ago
Closed 15 months ago
#5442 closed patch (fixed)
Database search_path does not what it intends to do
Reported by: | JelteF | Owned by: | robe |
---|---|---|---|
Priority: | medium | Milestone: | PostGIS 3.3.5 |
Component: | build | Version: | master |
Keywords: | search_path topology tiger | Cc: | JelteF |
Description
So there's two functions that run ALTER DATABASE ... SET to include a postgis extension schema in the default search_path. One that's used by the topology extension and one that's used by the tiger extension.
The topology extension has the most bugs, because it was not kept up to date with the tiger one. The tiger one also has bugs though:
- It uses reset_val instead of boot_val to build the search_path. This is problematic if the extension is being created for a user that has a specific search_path configured. In our case we do ALTER USER postgres SET search_path = pg_catalog. This is done for security reasons to avoid executing functions from the public schema with the superuser. By using reset_val as the base for the database its search_path, the new search_path will remove public for all users.
- At the end of the function it sets the search_path to the original default search_path using:
EXECUTE 'SET search_path = ' || var_cur_search_path;
This is incorrect because it removes any changes from the search_path that were done in the session. This is problematic for us, because Postgres includes the schemas of dependent extensions in the search_path of the session. But this removes them again.
Attached is a patch that fixes the tiger one, and changes the topology one with the same new code.
Attachments (2)
Change History (11)
by , 16 months ago
Attachment: | v1-0001-Fix-add-to-search_path.patch added |
---|
comment:1 by , 16 months ago
Before falling back to boot_val (when there's no database specific search_path), I actually think we might want to look at pg_file_settings
to see if search_path was set in one of the config files. You can get this using:
select setting from pg_file_settings where name = 'search_path' and applied;
comment:2 by , 16 months ago
This would be a good occasion to remove the duplicated code. Could both places include the same file predefining a variable ?
comment:3 by , 16 months ago
Yeah, I agree. The main reason I didn't do that in my patch was because my knowledge on the build system for the SQL files of postgis is quite minimal. So I wasn't sure how to organize the files so that both could include the same one.
by , 16 months ago
Attachment: | v2-0001-Fix-add-to-search_path.patch added |
---|
Fix-add-to-search_path.patch
comment:4 by , 16 months ago
I figured out how to build PostGIS. So I added a new patch that reduces the code duplication and adds the check for search_path from pg_file_settings.
comment:5 by , 16 months ago
I don't fully like the idea of including something from extension/ into the non-extension scripts. Also the function in that file are supposed to be temporary, and should not be left in the database. Running make staged-install && make -C topology/ check-regress
should report such errors to you.
Consider also filing a pull request ( on https://git.osgeo.org/gitea/postgis/postgis ) to have the bots run possibly other tests.
I think we also still want to tweak the session's search_path so someone can create extension and start using it immediately without the need to reconnect. You removed that part and thus had to change run_test.pl to do it for the sake of tests.
As this change needs more discussion I'd push to next milestone.
comment:6 by , 16 months ago
I created a PR here and addressed your feedback: https://git.osgeo.org/gitea/postgis/postgis/pulls/134
Let's continue discussion there.
comment:7 by , 16 months ago
Milestone: | PostGIS 3.3.4 → PostGIS 3.3.5 |
---|
comment:8 by , 15 months ago
Component: | postgis → build/upgrade/install |
---|---|
Owner: | changed from | to
Fix-add-to-search_path.patch