Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Postgres OID resolution query does not take into account current search_path #2133

Merged
merged 2 commits into from
Feb 15, 2023

Conversation

95ulisse
Copy link
Contributor

@95ulisse 95ulisse commented Oct 4, 2022

Problem

The current strategy employed to resolve a user-defined type name to an OID does not take into account the current search_path. This might cause some issues if we have different types with the same name in different schemas.

I noticed this behaviour because I'm working on a project that uses Postgres schemas to isolate integration tests by creating a new schema for each one, which means that I have a lot of types with the same name in different schemas.

CREATE TYPE status AS ENUM('open', 'closed');

CREATE SCHEMA test123;
CREATE TYPE test123.status AS ENUM('open', 'closed');

As an example, if we open a new connection and set the search_path to test123, INSERTs referencing the enum status start failing:

#[derive(sqlx::Type)]
#[sqlx(type_name = "status", rename_all = "lowercase")]
enum Status {
    Open,
    Closed
}

// Open a connection and set `search_path` to `test123`
let mut conn: PgConnection = todo!();
sqlx::query("SET search_path = 'test123'").execute(&mut conn).await.unwrap();

// Try inserting some data in a table using the `status` enum.
// This query fails with an error saying that the type `status` cannot be converted to `test123.status`.
sqlx::query("INSERT INTO some_table(id, status) VALUES ($1, $2)")
    .bind(123 as i32)
    .bind(Status::Open)
    .execute(&mut conn)
    .await
    .unwrap(); // This panics

The root of the error lies in the resolution of the type name status to OID that sqlx does when encoding the parameters for the INSERT, because the status name has been unexpectedly resolved to the public.status enum, and not to the test123.status enum.

The query used to resolve the name involves a SELECT from the table pg_catalog.pg_type, which is not namespaced, and thus, if we have multiple types with the same name, it will select one of them at random (even if in my tests, the public schema always was first, but I have no idea whether this is granted or not).

Proposed solution

Postgres has a native syntax to resolve type names to OIDs, which is 'some_type':regtype::oid, which correctly takes into account the current search_path. This PR proposes changing the current query with:

SELECT $1::regtype::oid

In order to respect the search_path of the current connection.

Drawbacks

The first drawback I can think of is caching. Connections cache the type name => oid mappings for obvious performance reasons, and this change would make the cached mappings invalid if the search_path changes after a type has been resolved.

We could decide to expose methods on the connections to manually clear the type cache (like we have right now for statements), but I'm not exactly sure it is worth it.

Also, please note that I have no idea whether this is a breaking change or not. I ran all the Postgres tests locally and they were green.

@abonander
Copy link
Collaborator

@95ulisse current development is on the 0.7-dev branch which just had some major refactors merged. Do you mind rebasing?

@95ulisse 95ulisse force-pushed the fix-postgres-oid-resolution branch from 7d2b3e7 to 4d9d138 Compare February 2, 2023 23:58
@95ulisse 95ulisse changed the base branch from main to 0.7-dev February 2, 2023 23:58
@95ulisse
Copy link
Contributor Author

95ulisse commented Feb 2, 2023

@abonander I've just rebased my PR on top of 0.7-dev 👍

@95ulisse 95ulisse force-pushed the fix-postgres-oid-resolution branch from 4d9d138 to 758b3b0 Compare February 5, 2023 01:26
Copy link
Collaborator

@abonander abonander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize it was this simple to get a type's OID, this is probably the way we should have been doing it from the start.

We do have a method already to clear the prepared statement cache, I suppose maybe that should also clear the type mapping cache because it's likely going to be called when major changes are being made anyway.

sqlx-postgres/src/connection/describe.rs Outdated Show resolved Hide resolved
@95ulisse
Copy link
Contributor Author

95ulisse commented Feb 8, 2023

I've modified clear_cached_statements to also clear the cache_type_oid map. I don't know if we also need to clear cache_type_info, but I don't think it's needed probably.

@abonander abonander merged commit 3e611e1 into launchbadge:0.7-dev Feb 15, 2023
@95ulisse 95ulisse deleted the fix-postgres-oid-resolution branch February 15, 2023 09:42
abonander pushed a commit that referenced this pull request Feb 18, 2023
…rch_path` (#2133)

* Fix oid resolution query

* Address review comments
abonander pushed a commit that referenced this pull request Feb 21, 2023
…rch_path` (#2133)

* Fix oid resolution query

* Address review comments
Aandreba pushed a commit to Aandreba/sqlx that referenced this pull request Mar 31, 2023
…rch_path` (launchbadge#2133)

* Fix oid resolution query

* Address review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants