Thread: Re: [HACKERS] DROP COLUMN round 4

> "Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> > 1. It cascade deletes objects, but it _always_ cascades, no matter what
> > behaviour I specify.  Also, it doesn't give me indications that
> it's cascade
> > deleted an object.
>
> Would you give a specific example?

test=# create table test (a int4, b int4);
CREATE TABLE
test=# create index temp on test (a);
CREATE INDEX
test=# \dt
        List of relations
 Name | Schema | Type  |  Owner
------+--------+-------+---------
 test | public | table | chriskl
(1 row)

test=# \di
            List of relations
 Name | Schema | Type  |  Owner  | Table
------+--------+-------+---------+-------
 temp | public | index | chriskl | test
(1 row)

test=# alter table test drop a restrict;
ALTER TABLE
test=# \di
No relations found.
test=#

> > + drop table child;
> > + ERROR:  RelationForgetRelation: relation 143905 is still open
>
> > What's with the RelationForgetRelation error???  Am I not closing some
> > handle somewhere?
>
> AlterTableDropColumn neglects to heap_close the relation, but I'm
> surprised that error isn't reported sooner.

Fixed.  New diff attached - fixes regression tests as well, plus re-merged
against HEAD.

Note that the check against the parent attribute when adding a foreign key
probably should be improved.  ie. It relies on the fact that the parent
column(s) should not have a unique index on them (thanks to dependencies),
rather than actually checking the attisdropped attribute.

Chris

"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> 1. It cascade deletes objects, but it _always_ cascades, no matter what
> behaviour I specify.  Also, it doesn't give me indications that
> it's cascade deleted an object.
>>
>> Would you give a specific example?

> [ dropping a column silently drops an index on the column ]

This happens because indexes are marked DEPENDENCY_AUTO on their
columns.  The drop will cascade to the index even under RESTRICT;
if you have message level set to DEBUG1 or higher you'll be told
about it, but otherwise the behavior is to zap the index quietly.

An example of a case where RESTRICT should make a difference is
where you have a foreign key reference from another table, or
a view that uses the column you're trying to delete.

We can debate whether indexes ought to be AUTO dependencies or not.
It seems reasonable to me though.  Indexes aren't really independent
objects to my mind, only auxiliary thingummies...

>> AlterTableDropColumn neglects to heap_close the relation, but I'm
>> surprised that error isn't reported sooner.

> Fixed.  New diff attached - fixes regression tests as well, plus re-merged
> against HEAD.

Thanks, will work from this.

> Note that the check against the parent attribute when adding a foreign key
> probably should be improved.  ie. It relies on the fact that the parent
> column(s) should not have a unique index on them (thanks to dependencies),
> rather than actually checking the attisdropped attribute.

Uh ... maybe it's too late at night here, but I didn't follow.

            regards, tom lane

> This happens because indexes are marked DEPENDENCY_AUTO on their
> columns.  The drop will cascade to the index even under RESTRICT;
> if you have message level set to DEBUG1 or higher you'll be told
> about it, but otherwise the behavior is to zap the index quietly.

Ah doh.  I knew that as well.  I'll try a view or something then...

> > Note that the check against the parent attribute when adding a
> foreign key
> > probably should be improved.  ie. It relies on the fact that the parent
> > column(s) should not have a unique index on them (thanks to
> dependencies),
> > rather than actually checking the attisdropped attribute.

OK, in this statement:

ALTER TABLE child ADD FOREIGN KEY (a) REFERENCES parent(b);

It does not check that column b is not dropped (it does check "a").  It just
relies on the UNIQUE constraint not being present on b.  This should
probably be fixed...

Chris

"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> Fixed.  New diff attached - fixes regression tests as well, plus re-merged
> against HEAD.

I've applied this patch after some editorializing.  Notably, I went
ahead and created syscache lookup convenience routines

extern HeapTuple SearchSysCacheAttName(Oid relid, const char *attname);
extern HeapTuple SearchSysCacheCopyAttName(Oid relid, const char *attname);
extern bool SearchSysCacheExistsAttName(Oid relid, const char *attname);

These are equivalent to the corresponding basic syscache routines for
the ATTNAME cache, except that they will release the cache entry and
return NULL (or false, etc) if the entry exists but has attisdropped =
true.

Essentially all uses of the ATTNAME cache should now go through these
routines instead.  This cleans up a number of cases in which the patch
as-submitted produced rather bogus error messages.

I did not adopt the approach of inserting NULLs in eref lists, because
I'm afraid that will break code elsewhere that relies on eref lists.
Instead I added a post-facto check to scanRTEforColumn, which is simple
and reliable.

Also, I'm still unconvinced that there's any percentage in modifying the
name of a deleted column to avoid collisions, since it does nothing for
after-the-fact collisions; AFAICT the net result is just to *enlarge*
the set of column names that users have to avoid.  So I took that code
out.  A possible solution that would actually work is to replace
pg_attribute's unique index on (attrelid, attname) with one on
(attrelid, attname, attisdropped).  However, I doubt that the problem is
worth the overhead that that would add.  I'm inclined to leave the code
as it stands and just document that column names like
"........pg.dropped.N........" are best avoided.


There are still issues about functions that accept or return tuple
datatypes --- a DROP COLUMN is likely to break any functions that use
the table's tuple datatype.  Related problems occur already with ADD
COLUMN, though, so I did not think this was a reason to reject the
patch.

            regards, tom lane

"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> OK, in this statement:

> ALTER TABLE child ADD FOREIGN KEY (a) REFERENCES parent(b);

> It does not check that column b is not dropped (it does check "a").  It just
> relies on the UNIQUE constraint not being present on b.  This should
> probably be fixed...

This is not really a case of it "relies on" that, it's just that the
check for a key is done before trying to resolve the column names to
numbers, so you get that error first.  I'm not sure it's worth trying to
rearrange the code in analyze.c to avoid that.  In the self-referential
case,

    create table foo (id int primary key,
              parent int references foo);

you more or less have to work with column names not numbers.

            regards, tom lane