sql: fix has_column_privilege builtin

[yuzefovich]

Previously, has_column_privilege would treat its second argument
(when it is an integer) that specifies the column to check the
privileges on as an ordinal index of the column, but when a column
is dropped from the table, ordinal indices are adjusted, so
ordinal_position of information_schema.columns would contain a
different from attnum of pg_catalog.pg_attribute number. Now
this is fixed.

Fixes: #39703.

Release note: None

[cockroach-teamcity]

This change is 

[yuzefovich]

Here is an excerpt from the PostgreSQL docs for the function:

has_column_privilege checks whether a user can access a column in a particular way. Its argument possibilities are analogous to has_table_privilege, with the addition that the column can be specified either by name or attribute number.

[nvanbenschoten]

Thanks for the quick fix @yuzefovich!

What you have here works, but I think it's actually a misdiagnosis of the problem. In #39703, we see that the ordinal_position from information_schema.columns is not equivalent to the attnum from pg_catalog.pg_attribute. I think these columns should be equivalent, but we're treating dropped columns differently in the two tables.

In Postgres:

nathan=# drop table if exists hcp_test;
DROP TABLE
nathan=# create table hcp_test(a int, b int, c int);
CREATE TABLE
nathan=# select attname, attnum from pg_attribute where attrelid = 'hcp_test'::regclass;
 attname  | attnum
----------+--------
 tableoid |     -7
 cmax     |     -6
 xmax     |     -5
 cmin     |     -4
 xmin     |     -3
 ctid     |     -1
 a        |      1
 b        |      2
 c        |      3
(9 rows)

nathan=# SELECT column_name, ordinal_position from information_schema.columns where table_name = 'hcp_test';
 column_name | ordinal_position
-------------+------------------
 a           |                1
 b           |                2
 c           |                3
(3 rows)

nathan=# alter table hcp_test drop column b;
ALTER TABLE
nathan=# select attname, attnum from pg_attribute where attrelid = 'hcp_test'::regclass;
           attname            | attnum
------------------------------+--------
 tableoid                     |     -7
 cmax                         |     -6
 xmax                         |     -5
 cmin                         |     -4
 xmin                         |     -3
 ctid                         |     -1
 a                            |      1
 ........pg.dropped.2........ |      2
 c                            |      3
(9 rows)

nathan=# SELECT column_name, ordinal_position from information_schema.columns where table_name = 'hcp_test';
 column_name | ordinal_position
-------------+------------------
 a           |                1
 c           |                3
(2 rows)

In CockroachDB:

[email protected]:58308/movr> drop table if exists hcp_test;
DROP TABLE
[email protected]:58308/movr> create table hcp_test(a int, b int, c int);
CREATE TABLE
[email protected]:58308/movr> select attname, attnum from pg_attribute where attrelid = 'hcp_test'::regclass;
  attname | attnum
+---------+--------+
  a       |      1
  b       |      2
  c       |      3
  rowid   |      4
(4 rows)
[email protected]:58308/movr> SELECT column_name, ordinal_position from information_schema.columns where table_name = 'hcp_test';
  column_name | ordinal_position
+-------------+------------------+
  a           |                1
  b           |                2
  c           |                3
  rowid       |                4
(4 rows)
[email protected]:58308/movr> alter table hcp_test drop column b;
ALTER TABLE
[email protected]:58308/movr> select attname, attnum from pg_attribute where attrelid = 'hcp_test'::regclass;
  attname | attnum
+---------+--------+
  a       |      1
  c       |      3
  rowid   |      4
(3 rows)
[email protected]:58308/movr> SELECT column_name, ordinal_position from information_schema.columns where table_name = 'hcp_test';
  column_name | ordinal_position
+-------------+------------------+
  a           |                1
  c           |                2
  rowid       |                3
(3 rows)

I think the correct fix here is to maintain stable ordinal_position values in the information_schema.columns table across dropped columns.

[jordanlewis]

I think the fix here is actually to stop using information_schema stuff to answer a question that's fundamentally about postgres, pg_catalog concepts :) I think if you rephrased the inner query to use pg_catalog everywhere instead of information_schema you'd have an easier time.

[nvanbenschoten]

I think the fix here is actually to stop using information_schema stuff to answer a question that's fundamentally about postgres, pg_catalog concepts

Have you looked into what that entails? has_column_privilege and the other ACL-related pg builtins aren't views on top of pg_catalog. I'm not even sure that pg_catalog exposes enough information to fully bootstrap these functions on top of it, and at one point I did spend some time exploring this option. If we could find a way to do that then I agree that it's the right approach, but I don't see any clear way to do so.

Also, I don't think it's accurate to say that pg_catalog.pg_attribute.attnum is "fundamentally about postgres, pg_catalog concepts". The column is defined as The number of the column. Ordinary columns are numbered from 1 up. System columns, such as oid, have (arbitrary) negative numbers.. Ignoring system columns (which we don't make an effort to replicate), this is almost identical to information_schema.columns.ordinal_position, which is defined as Ordinal position of the column within the table (count starts at 1).

[yuzefovich]

I briefly looked into Nathan's suggestion in keeping ordinal_position values the same when dropping a column, and it seems to me that we would need to modify table descriptors so that the dropped columns are present but not visible which might break some assumptions. Granted, I do not have any experience in that area of the codebase, but it feels like quite a big refactor.

I've just rewritten the query in question using pg_catalog which solves the bug but doesn't address the bigger picture.

[nvanbenschoten]

it seems to me that we would need to modify table descriptors so that the dropped columns are present but not visible which might break some assumptions

I don't think we'd have to go this far. We'd just need to use the ColumnDescriptor.ID as the ordinal position like we already do for pg_catalog.pg_attribute. It's strange to me that these differ in the first place. Maybe there's a reason that's not immediately obvious, but you can see in the SQL I posted above that we're doing the wrong (non-Postgres) thing with information_schema.columns.ordinal_position.

[nvanbenschoten]

This approach does LGTM as-is though! We're still mixing information_schema and pg_catalog, but for this initial attname question, that seems fine. I think I misinterpreted the discussion above as the more general topic of using information_schema.table_privileges to implement these pg acl builtins.

Before merging though, could you open an issue about the ordinal position semantic mismatch between the two tables?

[yuzefovich]

#39787 is filed. Thanks for the review!

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)