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 (bug): adnum in pg_attrdef and attnum in pg_attribute do not match. #46799

Closed
RichardJCai opened this issue Mar 31, 2020 · 7 comments
Closed
Assignees
Labels
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.

Comments

@RichardJCai
Copy link
Contributor

RichardJCai commented Mar 31, 2020

adnum in pg_attrdef should match attnum in pg_attribute.
In CRDB, the numbers can mismatch - this is because pg_attrdef currently uses the colNum (position in Columns slice in TableDescriptor) to generate adnum instead of ordinal_position / ColumnID.

In PG - adnum / attnum should match.

Here's an example where it differs from PG.

Postgres:

richard=# create table t(x INT DEFAULT 1, y INT DEFAULT 1);
richard=# alter table t drop column y; 
richard=# alter table t add column y int default 1;
richard=# select adrelid, adnum from pg_attrdef;
 adrelid | adnum 
---------+-------
   16399 |     1
   16399 |     3
(2 rows)

richard=# select attrelid, attname, attnum from pg_attribute where attrelid = 16399;
 attrelid |           attname            | attnum 
----------+------------------------------+--------
    16399 | tableoid                     |     -6
    16399 | cmax                         |     -5
    16399 | xmax                         |     -4
    16399 | cmin                         |     -3
    16399 | xmin                         |     -2
    16399 | ctid                         |     -1
    16399 | x                            |      1
    16399 | ........pg.dropped.2........ |      2
    16399 | y                            |      3
(9 rows)

CRDB:

root@127.0.0.1:58838/movr> create table t(x INT DEFAULT 1, y INT DEFAULT 1);
root@127.0.0.1:58838/movr> SET sql_safe_updates = false;
root@127.0.0.1:58838/movr> alter table t drop column y;
root@127.0.0.1:58838/movr> alter table t add column y int default 1;
root@127.0.0.1:58838/movr> select adrelid, adnum from pg_attrdef;
  adrelid | adnum
----------+--------
       59 |     1
       59 |     2 # This column corresponds to y, should be 4.
       59 |     3

root@127.0.0.1:58838/movr> select attrelid, attname, attnum from pg_attribute where attrelid = 59;
  attrelid | attname | attnum
-----------+---------+---------
        59 | x       |      1
        59 | rowid   |      3
        59 | y       |      4
(3 rows)
@RichardJCai RichardJCai added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-vtables Virtual tables - pg_catalog, information_schema etc labels Mar 31, 2020
@RichardJCai RichardJCai self-assigned this Mar 31, 2020
@nvanbenschoten
Copy link
Member

How does this issue relate to #39787?

Running the example you gave against Postgres does seem to produce the same results:

nathan=# create table t1 (id int PRIMARY KEY, id2 int, id3 int default 0);
CREATE TABLE
nathan=# select table_name, column_name, ordinal_position from information_schema.columns where table_name='t1';
 table_name | column_name | ordinal_position
------------+-------------+------------------
 t1         | id          |                1
 t1         | id2         |                2
 t1         | id3         |                3
(3 rows)

nathan=# alter table t1 drop column id3;
ALTER TABLE
nathan=# select table_name, column_name, ordinal_position from information_schema.columns where table_name='t1';
 table_name | column_name | ordinal_position
------------+-------------+------------------
 t1         | id          |                1
 t1         | id2         |                2
(2 rows)

nathan=# alter table t1 add column id3 int;
ALTER TABLE
nathan=# select table_name, column_name, ordinal_position from information_schema.columns where table_name='t1';
 table_name | column_name | ordinal_position
------------+-------------+------------------
 t1         | id          |                1
 t1         | id2         |                2
 t1         | id3         |                4

@RichardJCai
Copy link
Contributor Author

@nvanbenschoten Ah if that's the case then this looks correct.
However I think there still might be an inconsistency with regards to adnum in (pg_catalog line 413).
adnum seems to be using the colNum in the table instead of the ColumnID, I believe it should be consistent with attnum.

adnum and attnum both are defined as The number of the column
https://www.postgresql.org/docs/9.5/catalog-pg-attrdef.html
https://www.postgresql.org/docs/9.5/catalog-pg-attribute.html

@nvanbenschoten
Copy link
Member

Interesting. For these kinds of issues, it's usually best to present CRDB's output vs. Postgres'. PGs docs aren't always the easiest to understand and often have some ambiguity, but showing a difference in behavior that everyone can reproduce is irrefutable.

Is there a sequence of queries that cause adnum and attnum to differ between CRDB and PG?

@RichardJCai
Copy link
Contributor Author

RichardJCai commented Apr 21, 2020

Oops forgot to include this example - but heres one where adnum and attnum differ on CRDB but not in Postgres.

Postgres:

richard=# create table t(x INT DEFAULT 1, y INT DEFAULT 1);
richard=# alter table t drop column y; 
richard=# alter table t add column y int default 1;
richard=# select adrelid, adnum from pg_attrdef;
 adrelid | adnum 
---------+-------
   16399 |     1
   16399 |     3
(2 rows)

richard=# select attrelid, attname, attnum from pg_attribute where attrelid = 16399;
 attrelid |           attname            | attnum 
----------+------------------------------+--------
    16399 | tableoid                     |     -6
    16399 | cmax                         |     -5
    16399 | xmax                         |     -4
    16399 | cmin                         |     -3
    16399 | xmin                         |     -2
    16399 | ctid                         |     -1
    16399 | x                            |      1
    16399 | ........pg.dropped.2........ |      2
    16399 | y                            |      3
(9 rows)

CRDB:

root@127.0.0.1:58838/movr> create table t(x INT DEFAULT 1, y INT DEFAULT 1);
root@127.0.0.1:58838/movr> SET sql_safe_updates = false;
root@127.0.0.1:58838/movr> alter table t drop column y;
root@127.0.0.1:58838/movr> alter table t add column y int default 1;
root@127.0.0.1:58838/movr> select adrelid, adnum from pg_attrdef;
  adrelid | adnum
----------+--------
       59 |     1
       59 |     2 # This column corresponds to y, should be 4.
       59 |     3

root@127.0.0.1:58838/movr> select attrelid, attname, attnum from pg_attribute where attrelid = 59;
  attrelid | attname | attnum
-----------+---------+---------
        59 | x       |      1
        59 | rowid   |      3
        59 | y       |      4
(3 rows)

@RichardJCai RichardJCai changed the title sql (bug): Virtual tables assume ColumnID = ordinal position sql (bug): adnum in pg_attrdef and attnum in pg_attribute do not match. Apr 21, 2020
@nvanbenschoten
Copy link
Member

Got it, so to confirm, the bug is that pg_attrdef.adnum is implemented using the column number instead of column.ID here, right?

tree.NewDInt(tree.DInt(colNum)), // adnum

@RichardJCai
Copy link
Contributor Author

RichardJCai commented Apr 21, 2020

Got it, so to confirm, the bug is that pg_attrdef.adnum is implemented using the column number instead of column.ID here, right?

tree.NewDInt(tree.DInt(colNum)), // adnum

Yep - planning to fix this in a separate PR for adding LogicalColumnIDs to ColumnDescriptors. LogicalColumnID is added to support swapping the order of columns to allow ALTER COLUMN TYPE (uses a column swap after creating and backfilling a column with the new type)

@RichardJCai
Copy link
Contributor Author

Fixed by
#46992

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

No branches or pull requests

2 participants