Opened 3 years ago

Closed 6 months ago

Last modified 6 months ago

#17785 closed Bug (fixed)

PostgreSQL Introspection: get_relations() broken after drop column

Reported by: guettli 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 guettli 3 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 3 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 3 years ago by guettli

  • Type changed from Uncategorized to Bug

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 3 years ago by carljm

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

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 3 years ago by guettli

comment:4 Changed 3 years ago by guettli

  • 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 3 years ago by guettli

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 23 months ago by guettli

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

What can I do to get this fixed?

comment:7 Changed 23 months ago by timo

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

comment:8 Changed 11 months ago by akaariai

  • 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 6 months ago by claudep

  • Patch needs improvement unset

comment:10 Changed 6 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

comment:11 Changed 6 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

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 6 months ago by Shai Berger <shai@…>

In aa8ee6a5731b37b73635e7605521fb1a54a5c10d:

Fixed test failures in Oracle introspection

Refs #17785

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