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: column indnkeyatts in pg_index does not report key columns properly in primary key #88106

Closed
knz opened this issue Sep 18, 2022 · 6 comments · Fixed by #90287
Closed

sql: column indnkeyatts in pg_index does not report key columns properly in primary key #88106

knz opened this issue Sep 18, 2022 · 6 comments · Fixed by #90287
Assignees
Labels
A-sql-pgcatalog A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-vtables Virtual tables - pg_catalog, information_schema etc C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@knz
Copy link
Contributor

knz commented Sep 18, 2022

Needed for #88061.

Describe the problem

CockroachDB does not include primary key columns in the indnkeyatts attribute of pg_index.

To Reproduce

Initialize with:

create table mytable(x int, y int);

Then use the following query (replace the 3 occurrences of XXXX by mytables' primary index OID):

WITH obj AS (
   SELECT c.oid, c.relchecks, c.relkind, c.relhasindex, c.relhasrules,
          c.relhastriggers, c.relrowsecurity, c.relforcerowsecurity,
          false AS relhasoids, c.relispartition, c.reltablespace,
          CASE
          WHEN c.reloftype = 0 THEN ''
          ELSE c.reloftype::pg_catalog.regtype::pg_catalog.text END AS reloftype,
          c.relpersistence, c.relreplident, am.amname
     FROM pg_catalog.pg_class c
LEFT JOIN pg_catalog.pg_class tc ON (c.reltoastrelid = tc.oid)
LEFT JOIN pg_catalog.pg_am am    ON (c.relam = am.oid)
    WHERE c.oid = XXXXXXX), cols AS (
 SELECT a.attname,
        pg_catalog.format_type(a.atttypid, a.atttypmod) AS typname,
 CASE WHEN a.attnum <= (
  SELECT i.indnkeyatts
    FROM pg_catalog.pg_index i
   WHERE i.indexrelid = XXXXXXX) THEN 'yes' ELSE 'no' END AS is_key,
         pg_catalog.pg_get_indexdef(a.attrelid, a.attnum, TRUE) AS indexdef
    FROM pg_catalog.pg_attribute a
   WHERE a.attrelid = XXXXXXX AND a.attnum > 0 AND NOT a.attisdropped
ORDER BY a.attnum)
SELECT attname AS "Column",
       typname AS "Type",
       is_key AS "Key?",
       indexdef AS "Definition"
  FROM cols

(observe, the column rowid is not marked as Key because indnkeyatts is zero)

cc @rafiss for triage.

Jira issue: CRDB-19690

Epic: CRDB-23454

@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-vtables Virtual tables - pg_catalog, information_schema etc A-sql-pgcatalog labels Sep 18, 2022
@knz knz changed the title sql: column indnkeyatts in pg_index does not report key/storing columns properly sql: column indnkeyatts in pg_index does not report key columns properly in primary key Sep 18, 2022
@rafiss
Copy link
Collaborator

rafiss commented Sep 21, 2022

Is something missing from the the description? The steps show \d myidx yet myidx was not created.

My guess here is that this problem is specific to the hidden rowid column. If the table is created with an explicit PK, does it still have issues?

@knz
Copy link
Contributor Author

knz commented Sep 22, 2022

yes, this description was bogus. My apologies. Corrected.

@rafiss rafiss added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Oct 3, 2022
@rafiss
Copy link
Collaborator

rafiss commented Oct 18, 2022

I think the statement "indnkeyatts is zero" is not accurate.

root@localhost:26257/defaultdb> create table mytable(x int, y int);
CREATE TABLE

root@localhost:26257/defaultdb> select i.indexrelid, i.indrelid, c.relname from pg_index i join pg_class c on c.oid = i.indrelid where c.relname = 'mytable';
  indexrelid | indrelid | relname
-------------+----------+----------
  2083071188 |      104 | mytable
(1 row)

root@localhost:26257/defaultdb> SELECT i.indnkeyatts                                                                                                                FROM pg_catalog.pg_index i                                                                                                                                     WHERE i.indexrelid = 2083071188;
  indnkeyatts
---------------
            1
(1 row)

The problem seems to actually be that the query has some assumptions on a.attnum that do not hold. Specifically this part:

 CASE WHEN a.attnum <= (
  SELECT i.indnkeyatts
    FROM pg_catalog.pg_index i
   WHERE i.indexrelid = 2083071188) THEN 'yes' ELSE 'no' END AS is_key,

If we change that a bit, we can see:

root@localhost:26257/defaultdb> SELECT
  a.attname,
  a.attnum,
  pg_catalog.format_type(a.atttypid, a.atttypmod) AS typname,
  CASE
  WHEN a.attnum
  <= (
      SELECT
        i.indnkeyatts
      FROM
        pg_catalog.pg_index AS i
      WHERE
        i.indexrelid = 2083071188
    )
  THEN 'yes'
  ELSE 'no'
  END
    AS is_key,
  pg_catalog.pg_get_indexdef(a.attrelid, a.attnum, true)
    AS indexdef
FROM
  pg_catalog.pg_attribute AS a
WHERE
  a.attrelid = 2083071188 AND a.attnum > 0 AND NOT a.attisdropped
ORDER BY
  a.attnum;
  attname | attnum | typname | is_key | indexdef
----------+--------+---------+--------+-----------
  rowid   |      3 | bigint  | no     | y
(1 row)

The attnum of rowid is 3, but the query only marks it as a key if attnum <= indnkeyatts

@rafiss
Copy link
Collaborator

rafiss commented Oct 18, 2022

A more reliable query would use pg_index.indkey

indkey int2vector (references pg_attribute.attnum)

This is an array of indnatts values that indicate which table columns this index indexes. For example, a value of 1 3 would mean that the first and the third table columns make up the index entries. Key columns come before non-key (included) columns. A zero in this array indicates that the corresponding index attribute is an expression over the table columns, rather than a simple column reference.

@rafiss
Copy link
Collaborator

rafiss commented Oct 18, 2022

What I said above is true, but now I do think it's a bug that in the primary index the attnum of rowid is 3. Specifically:

root@localhost:26257/defaultdb> create table mytable(x int, y int);
CREATE TABLE

root@localhost:26257/defaultdb> select i.indexrelid, i.indrelid, c.relname from pg_index i join pg_class c on c.oid = i.indrelid where c.relname = 'mytable';
  indexrelid | indrelid | relname
-------------+----------+----------
  2083071188 |      104 | mytable
(1 row)

# This is fine.
root@localhost:26257/defaultdb> select attname, attnum from pg_attribute where attrelid = 104;
  attname | attnum
----------+---------
  x       |      1
  y       |      2
  rowid   |      3
(3 rows)

# This is buggy.
root@localhost:26257/defaultdb> select attname, attnum from pg_attribute where attrelid = 2083071188;
  attname | attnum
----------+---------
  rowid   |      3
(1 row)

@rafiss
Copy link
Collaborator

rafiss commented Oct 18, 2022

Here is a repro that we can compare directly to Postgres to see the bug:

Postgres behavior:

postgres=# create table mytable(x int, y int primary key);
CREATE TABLE

postgres=# select i.indexrelid, c.relname from pg_index i join pg_class c on c.oid = i.indrelid where c.relname = 'mytable';;
 indexrelid | relname
------------+---------
      42597 | mytable
(1 row)

postgres=# select attname, attnum from pg_attribute where attrelid = 42597; -- this ID is from the previous query.
 attname | attnum
---------+--------
 y       |      1
(1 row)

CockroachDB behavior (buggy):

root@localhost:26257/defaultdb> create table mytable(x int, y int primary key);
CREATE TABLE

root@localhost:26257/defaultdb> select i.indexrelid,  c.relname from pg_index i join pg_class c on c.oid = i.indrelid where c.relname = 'mytable';
  indexrelid | relname
-------------+----------
   478257910 | mytable
(1 row)

root@localhost:26257/defaultdb> select attname, attnum from pg_attribute where attrelid = 478257910; -- this ID is from the previous query.
  attname | attnum
----------+---------
  y       |      2
(1 row)

@craig craig bot closed this as completed in 1ef7748 Oct 21, 2022
craig bot pushed a commit that referenced this issue Feb 24, 2023
88061: clisqlshell: new infrastructure for describe commands r=rafiss,ZhouXing19 a=knz

Fixes #95320.
Epic: CRDB-23454

The SQL shell (`cockroach sql`, `demo`) now
supports the client-side commands `\l`, `\dn`, `\d`, `\di`, `\dm`,
`\ds`, `\dt`, `\dv`, `\dC`, `\dT`, `\dd`, `\dg`, `\du`, `\df` and `\dd` in a
way similar to `psql`, including the modifier flags `S` and `+`, for
convenience for users migrating from PostgreSQL.

A notable difference is that when a pattern argument is specified, it
should use the SQL "LIKE" syntax (with `%` representing the wildcard
character) instead of PostgreSQL's glob-like syntax (with `*`
representing wildcards).

Issues discovered:

- [x] join bug:  #88096
- [x] semi-join exec error #91012
- [x] `pg_table_is_visible` should return true when given a valid index OID and the index is valid.  #88097
- [x] missing pkey column in pg_index:  #88106
- [x] missing stored columns in pg_index: #88107 
- [x] pg_statistic_ext has problems #88108
- [x] missing view def on materialized views  #88109
- [x] missing schema comments: #88098
- [x] missing pronamespace for functions #94952
- [x] broken pg_function_is_visible for UDFs #94953
- [x] generated columns #92545
- [x] indnullsnotdistinct #92583
- [x] missing prokind #95288
- [x] missing function comments in obj_description #95292
- [x] planning regression #95633

96397: builtins: mark some pg_.* builtins as strict r=DrewKimball a=mgartner

Builtins defined using the UDF `Body` field will be wrapped in a `CASE`
expression if they are strict, i.e., `CalledOnNullInput=false`. When the
builtin is inlined, the `CASE` expression prevents decorrelation,
leaving a slow apply-join in the query plan. This caused a significant
regression of some ORM introspection queries.

Some of these builtins have filters that cause the SQL body to return no rows
if any of the arguments is NULL. In this case, the builtin will have the same
behavior whether or not it is defined as being strict. We can safely optimize
these builtins by setting `CalledOnNullInput=true`.

The following conditions are sufficient to prove that `CalledOnNullInput` can
be set for a builtin function with a SQL body:

  1. The WHERE clause of the SQL query *null-rejects* every argument of the
     builtin. Operators like `=` and `<` *null-reject* their operands because
     they filter rows for which an operand is NULL.

  2. The arguments are not used elsewhere in the query. This is not strictly
     necessary, but simplifies the proof because it ensures NULL arguments will
     not cause the builtin to error.

Examples of SQL statements that would allow `CalledOnNullInput` to be set:
```
SELECT * FROM tab WHERE $1=1 AND $2='two';

SELECT * FROM tab WHERE $1 > 0;
```

Fixes #96218
Fixes #95569

Epic: None

Release note: None


97585: cli: don't scope TLS client certs to a specific tenant by default r=stevendanna a=knz

Epic: CRDB-23559
Fixes: #97584

This commit changes the default for `--tenant-scope` from "only the system tenant" to "cert valid for all tenants".

Note that the scoping is generally useful for security, and it is used in CockroachCloud. However, CockroachCloud does not use our CLI code to generate certs and sets its cert tenant scopes on its own.

Given that our CLI code is provided for convenience and developer productivity, and we don't expect certs generated here to be used in multi-tenant deployments where tenants are adversarial to each other, defaulting to certs that are valid on every tenant is a good choice.

Release note: None


Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcatalog A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-vtables Virtual tables - pg_catalog, information_schema etc C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants