PostgreSQL Introspection: get_relations() broken after drop column

[Thomas Güttler]

Hi,

if a column gets dropped postgreSQL does not update the attnum of columns which have a higher attnum:

# oid: select oid from pg_class where relname='modwork_buchungskreis';

modwork_eins_d=> select attrelid, attname, atttypid,  attnum from pg_attribute where attrelid = 149923 order by attnum; attrelid |           attname            | atttypid | attnum 
----------+------------------------------+----------+--------
   149923 | tableoid                     |       26 |     -7
   149923 | cmax                         |       29 |     -6
   149923 | xmax                         |       28 |     -5
   149923 | cmin                         |       29 |     -4
   149923 | xmin                         |       28 |     -3
   149923 | ctid                         |       27 |     -1
   149923 | id                           |     1043 |      1
   149923 | name                         |     1043 |      2
   149923 | ........pg.dropped.3........ |        0 |      3
   149923 | ........pg.dropped.4........ |        0 |      4
   149923 | default                      |       16 |      5
   149923 | beschreibung                 |     1043 |      6
   149923 | archiv                       |     1043 |      7
   149923 | email_sender_id              |     1043 |      8

BTW, the same bug is in django/db/backends/postgresql/introspection.py

Patch attached. I think it is overkill to write a unittest for this. Please let me know if you want a test.

[Claude Paroz]

Did you explore the possibility to use the column name as the relations dict key instead of an index, which seems very fragile to me?

[Thomas Güttler]

I updated the patch. A dictionary was not needed. A list is enough. I would prefere to return column names from get_relations(), but of course I need to fit to the current API: Returns a dictionary of

{field_index: (field_index_other_table, other_table)}

[Carl Meyer]

Off-hand I don't see any reason why this fix deserves to not be tested. I realize that the test will require creating and modifying a table, but that's not a problem, the existing introspection tests already use raw SQL for this sort of thing, it's not difficult.

[Thomas Güttler]

I updated the patch. It has a unittest now. I tested it with sqlite (test get skipped, since it does not support DROP COLUMN) and psycopg2.

[Thomas Güttler]

PostgreSQL 9.0.6 contains a bug fix in information_schema.referential_constraints. This does not change the way postgresql handles deleted columns. This patch is still needed. I tested it with version 9.0.10.

[Thomas Güttler]

The bug still exists. Tested with Django1.5 and PostgreSQL 9.0.10.

What can I do to get this fixed?

[Tim Graham]

@guettli - you need to get someone to review your patch and mark it "Ready for checkin."

[Anssi Kääriäinen]

The attached tests will not work on MySQL. The tests use fixed quoting (not every database uses " for quoting). In addition, I don't think the table creation belongs into the test, if at all possible it would be better to use normal models and then alter those dynamically. This would give better portability.

Instead of doing selects to the local and remote table in get_relations(), we could likely use information_schema.columns to skip the missing field indexes. The query is something like:

WITH augmented_information_schema_columns AS (
        SELECT table_name, ordinal_position,
               row_number() OVER (PARTITION BY table_name ORDER BY ordinal_position)
          FROM information_schema.columns
)
SELECT aisc1.row_number - 1 as local_field_index, aisc2.row_number - 1 as remote_field_index, c2.relname
  FROM pg_constraint con
  JOIN pg_class c1 ON c1.oid = con.conrelid
  JOIN pg_class c2 ON c2.oid = con.confrelid
  JOIN augmented_information_schema_columns aisc1 ON con.conkey[1] = aisc1.ordinal_position
       AND aisc1.table_name = c1.relname
  JOIN augmented_information_schema_columns aisc2 ON con.confkey[1] = aisc2.ordinal_position
       AND aisc2.table_name = c2.relname
  WHERE c1.relname = 'bar'
       AND con.contype = 'f';

(only very quickly tested).

Maybe a better approach would be to dump usage of column numbers altogether, instead use just column names in get_relations().

[Claude Paroz]

​https://github.com/django/django/pull/3884

[Claude Paroz <claude@…>]

In 4c413e231cfe788de6e371567f395c8ccbd26103:

Fixed #17785 -- Preferred column names in get_relations introspection

Thanks Thomas Güttler for the report and the initial patch, and
Tim Graham for the review.

[Shai Berger <shai@…>]

In aa8ee6a5731b37b73635e7605521fb1a54a5c10d:

Fixed test failures in Oracle introspection

Refs #17785

[Tim Graham <timograham@…>]

In 4b9d063:

Refs #17785 -- Made docstring for sqlite3's get_relations() consistent with other backends.

[Tim Graham <timograham@…>]

In eb0bbb8:

[1.8.x] Refs #17785 -- Made docstring for sqlite3's get_relations() consistent with other backends.

Backport of 4b9d063da05faa112577a4e3cefd020850a25e9e from master