Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#17785 closed Bug (fixed)

PostgreSQL Introspection: get_relations() broken after drop column

Reported by: Thomas Güttler Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords: psycopg2 introspection
Cc: hv Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no



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 | |        0 |      3
   149923 | |        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/

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

Attachments (1)

pscopg2-introspection.patch (4.4 KB) - added by Thomas Güttler 6 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 years ago by 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?

comment:2 Changed 6 years ago by Thomas Güttler

Type: UncategorizedBug

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)}

comment:3 Changed 6 years ago by Carl Meyer

Needs tests: set
Triage Stage: UnreviewedAccepted

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.

Changed 6 years ago by Thomas Güttler

Attachment: pscopg2-introspection.patch added

comment:4 Changed 6 years ago by Thomas Güttler

Needs tests: unset

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.

comment:5 Changed 6 years ago by 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.

comment:6 Changed 5 years ago by Thomas Güttler

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

What can I do to get this fixed?

comment:7 Changed 5 years ago by Tim Graham

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

comment:8 Changed 4 years ago by Anssi Kääriäinen

Patch needs improvement: set

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().

comment:9 Changed 3 years ago by Claude Paroz

Patch needs improvement: unset

comment:10 Changed 3 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:11 Changed 3 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

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.

comment:12 Changed 3 years ago by Shai Berger <shai@…>

In aa8ee6a5731b37b73635e7605521fb1a54a5c10d:

Fixed test failures in Oracle introspection

Refs #17785

comment:13 Changed 3 years ago by Tim Graham <timograham@…>

In 4b9d063:

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

comment:14 Changed 3 years ago by 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

Note: See TracTickets for help on using tickets.
Back to Top