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

sql: introduce PG-compatible access privilege inquiry functions #23334

Merged

Conversation

Projects
None yet
4 participants
@nvanbenschoten
Copy link
Member

commented Mar 3, 2018

Fixes #22734.
Fixes #20784.
Relates to #15441.

This change introduces a series of Postgres-compatible privilege-related
builtin functions:

  • has_any_column_privilege
  • has_column_privilege
  • has_database_privilege
  • has_foreign_data_wrapper_privilege
  • has_function_privilege
  • has_language_privilege
  • has_schema_privilege
  • has_sequence_privilege
  • has_server_privilege
  • has_table_privilege
  • has_tablespace_privilege
  • has_type_privilege
  • pg_has_role (coming soon!)

These all follow the specification documented by Postgres in:
https://www.postgresql.org/docs/8.4/static/functions-info.html#FUNCTIONS-INFO-ACCESS-TABLE

These Access Privilege Inquiry Functions allow users to query object
access privileges programmatically. Each function has a number of
variants, which differ based on their function signatures. These
signatures have the following structure:

- optional "user" argument
  - if used, can be a STRING or an OID type
  - if not used, current_user is assumed
- series of one or more object specifier arguments
  - each can accept multiple types
- a "privilege" argument
  - must be a STRING
  - parsed as a comma-separated list of privilege

This means that in total, each function has at least 6 variants.

The main reason for adding these builtins in is because they were the
last remaining issue that was blocking full compatibility with pgweb.
Pgweb is a web-based database browser written in Go, which means that
can run on OSX, Linux and Windows machines! Here's what TPC-C looks
like in pgweb:

screenshot-2018-3-2 pgweb

Release note (sql change): Introduces a series of Postgres-compatible
privilege-related builtin functions.

@nvanbenschoten nvanbenschoten added this to the 2.0 milestone Mar 3, 2018

@nvanbenschoten nvanbenschoten requested review from jordanlewis and mberhault Mar 3, 2018

@nvanbenschoten nvanbenschoten requested review from cockroachdb/sql-execution-prs as code owners Mar 3, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Mar 3, 2018

This change is Reviewable

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten:nvanbenschoten/pgSysFunc branch from 31a1352 to ae97d2f Mar 3, 2018

@fire fire referenced this pull request Mar 8, 2018

Closed

Support CockroachDB #235

@sosedoff

This comment has been minimized.

Copy link

commented Mar 8, 2018

Pgweb author here. Whats the main reasoning to get cockroachdb compatible with pgweb?

@nvanbenschoten

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2018

@sosedoff CockroachDB aims to be Postgres compatible both in the wire protocol it speaks and the SQL semantics it exposes. As such, people expect that the same tools they use with Postgres will also work with CockroachDB.

There were a few reasons to focus on pgweb compatibility specifically here:

  • pgweb looks nice and is highly functional! You've clearly done a great job with the project
  • pgweb is cross-platform
  • pgweb is written in Go and so is cockroach, for whatever that's worth
  • pgweb was already very close to working because you have avoided relying on any obscure features of Postgres
  • pgweb compatibility required improving pg-compatibility in general, so the changes made here are progress towards a more general goal
@mberhault

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

LGTM, just a few nits. I also find it odd that we're allowing queries for privileges we have no concept of, but I suppose it makes sense given that this is for compatibility.


Review status: 0 of 9 files reviewed at latest revision, all discussions resolved, some commit checks failed.


pkg/sql/logictest/testdata/logic_test/privilege_builtins, line 1 at r3 (raw file):

# LogicTest: default distsql

It would be good to run a few of the builtins with user testuser to ensure that we allow everyone to query everything (postgres does, so as odd as it seems, we need to ensure we do the same).


pkg/sql/sem/tree/eval.go, line 2905 at r3 (raw file):

			return NewDName(s), nil
		default:
			panic(fmt.Sprintf("missing case for cast to string: %T", c))

If I'm reading this function correctly, this panic will be covered by the CodeCannotCoerceError at the end, correct?


Comments from Reviewable

nvanbenschoten added some commits Mar 2, 2018

sql: clean up pg_catalog.go
Release note: None
sql: Add pg_description table to pg_catalog
The `pg_language` table registers languages in which users can
write functions or stored procedures.

Release note (sql change): Added pg_language table to the pg_catalog.
sql: introduce PG-compatible access privilege inquiry functions
Fixes #22734.
Fixes #20784.
Relates to #15441.

This change introduces a series of Postgres-compatible privilege-related
builtin functions:
- `has_any_column_privilege`
- `has_column_privilege`
- `has_database_privilege`
- `has_foreign_data_wrapper_privilege`
- `has_function_privilege`
- `has_language_privilege`
- `has_schema_privilege`
- `has_sequence_privilege`
- `has_server_privilege`
- `has_table_privilege`
- `has_tablespace_privilege`
- `has_type_privilege`
- `pg_has_role` (_coming soon!_)

These all follow the specification documented by Postgres in:
https://www.postgresql.org/docs/8.4/static/functions-info.html#FUNCTIONS-INFO-ACCESS-TABLE

These Access Privilege Inquiry Functions allow users to query object
access privileges programmatically. Each function has a number of
variants, which differ based on their function signatures. These
signatures have the following structure:
```
- optional "user" argument
  - if used, can be a STRING or an OID type
  - if not used, current_user is assumed
- series of one or more object specifier arguments
  - each can accept multiple types
- a "privilege" argument
  - must be a STRING
  - parsed as a comma-separated list of privilege
```

This means that in total, each function has at least 6 variants.

The main reason for adding these builtins in is because they were the
last remaining issue that was blocking full compatibility with pgweb.
Pgweb is a web-based database browser written in Go, which means that
can run on OSX, Linux and Windows machines!

Release note (sql change): Introduces a series of Postgres-compatible
privilege-related builtin functions.

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten:nvanbenschoten/pgSysFunc branch from ae97d2f to 94c25be Mar 14, 2018

@nvanbenschoten

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2018

TFTR!


Review status: 0 of 9 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/sql/logictest/testdata/logic_test/privilege_builtins, line 1 at r3 (raw file):

Previously, mberhault (marc) wrote…

It would be good to run a few of the builtins with user testuser to ensure that we allow everyone to query everything (postgres does, so as odd as it seems, we need to ensure we do the same).

Done.


pkg/sql/sem/tree/eval.go, line 2905 at r3 (raw file):

Previously, mberhault (marc) wrote…

If I'm reading this function correctly, this panic will be covered by the CodeCannotCoerceError at the end, correct?

Yep, exactly. I'm not sure why we had this panic here as well.


Comments from Reviewable

@nvanbenschoten nvanbenschoten merged commit efb6111 into cockroachdb:master Mar 14, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
code-review/reviewable Review complete: 0 of 0 LGTMs obtained
Details
license/cla Contributor License Agreement is signed.
Details

@nvanbenschoten nvanbenschoten deleted the nvanbenschoten:nvanbenschoten/pgSysFunc branch Mar 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.