#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:

  1. 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.
  2. 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)

v1-0001-Fix-add-to-search_path.patch (3.7 KB ) - added by JelteF 16 months ago.
Fix-add-to-search_path.patch
v2-0001-Fix-add-to-search_path.patch (6.4 KB ) - added by JelteF 16 months ago.
Fix-add-to-search_path.patch

Download all attachments as: .zip

Change History (11)

by JelteF, 16 months ago

Fix-add-to-search_path.patch

comment:1 by JelteF, 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 strk, 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 JelteF, 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 JelteF, 16 months ago

Fix-add-to-search_path.patch

comment:4 by JelteF, 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 strk, 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 JelteF, 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.

Last edited 16 months ago by JelteF (previous) (diff)

comment:7 by strk, 16 months ago

Milestone: PostGIS 3.3.4PostGIS 3.3.5

comment:8 by robe, 15 months ago

Component: postgisbuild/upgrade/install
Owner: changed from pramsey to robe

comment:9 by Regina Obe <lr@…>, 15 months ago

Resolution: fixed
Status: newclosed

In a514af6/git:

Fix add to search_path (Jelte Fennema)
Closes #5442 for PostGIS 3.3.5
Closes https://git.osgeo.org/gitea/postgis/postgis/pulls/134

Note: See TracTickets for help on using tickets.