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

pg-ext: add more SQL func wrappers #8488

Merged
merged 6 commits into from
Mar 21, 2025
Merged

pg-ext: add more SQL func wrappers #8488

merged 6 commits into from
Mar 21, 2025

Conversation

fantix
Copy link
Member

@fantix fantix commented Mar 19, 2025

Adds wrappers for:

  • has_database_privilege
  • has_any_column_privilege

has_any_column_privilege is needed by Metabase introspecting the schema.

Also added an internal util function to_regclass() that uses parse_ident() to imitate the actual to_regclass(). This may potentially replace the current compiler-backed to_regclass(), but I'd keep this PR small.

The pg_database view was broken, fix that too.

  • Fix namespaced table names
  • Fix broken pg_database view
  • Add test

Refs #5307, #5319
Fixes #8457

* has_database_privilege
* has_any_column_privilege
@fantix fantix added the to-backport-6.x PRs that *should* be backported to 6.x label Mar 19, 2025
@fantix fantix requested review from msullivan and aljazerzen March 19, 2025 20:38
@fantix fantix marked this pull request as ready for review March 19, 2025 20:38
@fantix fantix requested review from elprans and removed request for msullivan March 19, 2025 22:24
(
SELECT oid::regclass
FROM edgedbsql_VER.pg_class
WHERE relname = parts[1]
Copy link
Member

Choose a reason for hiding this comment

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

This is icky. Relation names might not be unique across schemas so a "subquery returned more than one row" is possible. You need to take search_path into account (via current_schemas()?).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we would have to take search_path into account.

But not our internal search path accessible via current_schemas, but the search_path we expose to the user. That is passed to pg_resolver in ResolverOptions and is (currently) not accessible at runtime in SQL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I remember now: there is cast_to_regclass and to_regclass in static.py. They do the "search_path" part statically and generate SQL for runtime introspection.

They also handle all supported ways of expressing a regclass.

Copy link
Member Author

@fantix fantix Mar 20, 2025

Choose a reason for hiding this comment

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

is (currently) not accessible at runtime in SQL.

Does that mean this PR needs to be re-done in the compiler? Or I should just make search_path available in the _edgecon_state temp table?

there is cast_to_regclass and to_regclass in static.py.

Yeah, that's what I meant by:

This may potentially replace the current compiler-backed to_regclass(), but I'd keep this PR small.

Actually, I'd like to move forward with the search_path issue unfixed like it has been for months (but with some strong comments/warnings in code, and a limit 1 mitigation) and solve the namespace and quoting issue first.

Because it's much worse that this doesn't work:

main=# select has_table_privilege('"TypeWithUniqueName"', 'update');
 has_table_privilege
---------------------

(1 row)

(Because it's more likely to have case-sensitive names with Gel schemas, that require quoting in SQL. The current 6.3 server mistakingly supports the unquoted has_table_privilege('TypeWithUniqueName', ..) but not the properly quoted has_table_privilege('"TypeWithUniqueName"', ..), not to mention namespaced names.)

... rather than baring with this being inaccurately correct:

main=# select has_table_privilege('"TypeWithDupNameInDiffModules"', 'update');
 has_table_privilege
---------------------
 t
(1 row)

(This behaves the same as the current 6.3 server, whose implementation SQL is also returning multiple rows, I think the function wrapper implicitly aggregated the bools and took the first row?)

It's still better if we can fix it properly, it's just seemingly a much larger changeset. Thoughts? I'm fine with either 1 PR or 2.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, the compiler implementation of to_regclass is also wrong, because it only takes into account the top search path and it should search the whole stack, including the implicit entries (i.e pg_catalog).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree has_table_privilege is a much higher priority than regclass. I'd suggest moving impl of to_regclass to SQL in a separate PR, because it might get large and complex.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll start a new PR to apply the application-level search_path in the backend as an opaque value, and reimplement sql.to_regclass properly.

Copy link
Contributor

@aljazerzen aljazerzen left a comment

Choose a reason for hiding this comment

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

Looks good apart from to_regclass thing.

@fantix fantix merged commit 9099869 into master Mar 21, 2025
23 checks passed
@fantix fantix deleted the sql-priv-func-wrapper branch March 21, 2025 13:33
@fantix
Copy link
Member Author

fantix commented Mar 21, 2025

Merged and explained in the commit message:

If the same table name exists in more than one schema, the has*_privilege() functions that take an unqualified table name are not yet looking into search_path as they are expected to be, but rather choose a "random" one as the result. This is not a problem yet, because Gel user is "superuser" only, it has access to any of the tables with the same name, so the result is inaccurately correct.

elprans pushed a commit that referenced this pull request Mar 21, 2025
Adds wrappers for:

* `has_database_privilege`
* `has_any_column_privilege` (this fixes Metabase compatibility)

If the same table name exists in more than one schema, the
`has*_privilege()` functions that take an unqualified table
name are not yet looking into `search_path` as they are expected
to be, but rather choose a "random" one as the result. This is
not a problem yet, because Gel user is "superuser" only, it has
access to any of the tables with the same name, so the result
is inaccurately correct.

Also fixes the `pg_database` view.

Refs #5307, #5319
Fixes #8457
@elprans elprans mentioned this pull request Mar 21, 2025
elprans pushed a commit that referenced this pull request Mar 22, 2025
Adds wrappers for:

* `has_database_privilege`
* `has_any_column_privilege` (this fixes Metabase compatibility)

If the same table name exists in more than one schema, the
`has*_privilege()` functions that take an unqualified table
name are not yet looking into `search_path` as they are expected
to be, but rather choose a "random" one as the result. This is
not a problem yet, because Gel user is "superuser" only, it has
access to any of the tables with the same name, so the result
is inaccurately correct.

Also fixes the `pg_database` view.

Refs #5307, #5319
Fixes #8457
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to-backport-6.x PRs that *should* be backported to 6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gel 6.1 Metabase connection - relation "public.XXXX" does not exist
3 participants