Opened 13 years ago

Closed 10 years ago

Last modified 9 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

Description

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.

Attachments (1)

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

Download all attachments as: .zip

Change History (15)

comment:1 by Claude Paroz, 13 years ago

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 by Thomas Güttler, 13 years ago

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 by Carl Meyer, 13 years ago

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.

by Thomas Güttler, 13 years ago

Attachment: pscopg2-introspection.patch added

comment:4 by Thomas Güttler, 13 years ago

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 by Thomas Güttler, 12 years ago

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 by Thomas Güttler, 11 years ago

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

What can I do to get this fixed?

comment:7 by Tim Graham, 11 years ago

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

comment:8 by Anssi Kääriäinen, 10 years ago

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 by Claude Paroz, 10 years ago

Patch needs improvement: unset

comment:10 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:11 by Claude Paroz <claude@…>, 10 years ago

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 by Shai Berger <shai@…>, 10 years ago

In aa8ee6a5731b37b73635e7605521fb1a54a5c10d:

Fixed test failures in Oracle introspection

Refs #17785

comment:13 by Tim Graham <timograham@…>, 9 years ago

In 4b9d063:

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

comment:14 by Tim Graham <timograham@…>, 9 years ago

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