Skip site navigation (1) Skip section navigation (2)

Re: Demo patch for DROP COLUMN

From: "Christopher Kings-Lynne" <chriskl(at)familyhealth(dot)com(dot)au>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: "Patches" <pgsql-patches(at)postgresql(dot)org>
Subject: Re: Demo patch for DROP COLUMN
Date: 2002-07-22 02:22:40
Message-ID: GNELIHDDFBOCMGBFGEFOKEDOCDAA.chriskl@familyhealth.com.au (view raw or flat)
Thread:
Lists: pgsql-hackerspgsql-patches
> You really really should put in the wrapper routines that were suggested
> for SearchSysCache, as this would avoid a whole lot of duplicated code;
> most of the explicit tests for COLUMN_IS_DROPPED that are in your patch
> could be replaced with that technique, reducing code size and increasing
> readability.

Thing is that not all the commands use SearchSysCache (some use get_attnum
for instance), and you'd still need to go thru all the commands and change
them to use SearchSysCacheNotDropped(blah).  I'll do it tho, if you think
it's a better way.

> The implementation routine for DROP COLUMN is wrong; it needs to be
> split into two parts, one that calls performDeletion and one that's
> called by same.  AlterTableDropConstraint might be a useful model,
> although looking at it I wonder whether it has the permissions checks
> right.  (Should the owner of a table be allowed to drop constraints on
> child tables he doesn't own?  If not, we should recurse to
> AlterTableDropConstraint rather than calling RemoveRelChecks directly.)

Yeah - I hadn't gotten around to researching how to split it up yet.  That's
why I commented out the performDeletion.  I'll do it soon.

> Also, you have
>
> ! 	 * Propagate to children if desired
> ! 	 * @@ THIS BEHAVIOR IS BROKEN, HOWEVER IT MATCHES RENAME COLUMN @@
>
> What's broken about it?  RENAME probably is broken, in that it should
> never allow non-recursion, but DROP doesn't have that issue.

Hrm.  Yeah I guess the real problem is dropping a column that still exists
in an ancestor.  I got mixed up there.  Is there a problem with this,
though:

ALTER TABLE ONLY ancestor DROP a;

Seems a little dodgy to me...

> Why not just
>
> 	while (SearchSysCacheExists(...))
> 	{
> 		snprintf(... ++n ...);
> 	}

Ah yes.

> More to the point, though, this is a waste of time.  Since you're using
> a physical column number there can be no conflict against the names of
> other dropped columns.  There might be a conflict against a user column
> name, but the answer to that is to choose a weird name in the first
> place; the hack above only avoids half of the problem, namely when the
> user column already exists.

Problem was, I couldn't find a generated name that was _impossible_ to enter
as a user.  So, I made sure it would never be a problem.

> You're still wrong if he tries to do ALTER
> TABLE ADD COLUMN ___pg_dropped1 later on.  Therefore what you want to
> do is use a name that is (a) very unlikely to be used in practice, and
> (b) predictable so that we can document it and say "tough, you can't use
> that name".  I'd suggest something like
> 	........pg.dropped.%d........
> Note the use of dots instead of underscores --- we may as well pick a
> name that's not valid as an unquoted identifier.

Not that I already use spaces in my generated names, for this exact reason.
But I'll change it to your example above...

> (Dashes or other
> choices would work as well.  I'm not in favor of control characters
> or whitespace in the name though.)

OK, no whitespace.

> Although I think it was my suggestion to put nulls in the eref lists,
> it sure looks messy.  What happens if you just allow the dropped-column
> names to be used in eref?  After getting a match you'll need to check if
> the column is actually valid, but I think maybe this can be combined
> with lookups that will occur anyway.  Probably it'd net out being fewer
> changes in total.

Well, that change was actually trivial after I thought it was going to be
difficult...  When you say "after getting a match need to check if actually
valid", wasn't that what I proposed originally, but we decided it'd be
faster if the columns never even made it in?  Where would you propose doing
these post hoc checks?

> The proposed pg_dump change will NOT work, since it will cause pg_dump's
> idea of column numbers not to agree with reality, thus breaking
> pg_attrdef lookups and probably other places.  Probably best bet is to
> actually retrieve attisdropped (or constant 'f' for older releases),
> store dropped attrs in the pg_dump datastructures anyway, and then omit
> them at the time of listing out the table definition.

Yes, I'm actually aware of this.  In fact, I was planning to make exactly
the change you have suggested here.

> tab-complete query is wrong (try it)...

OK, hadn't noticed that one.  BTW, how do I make a dropped column disappear
from the column-name-tab-complete-cache thingy in psql?  I guess I'll see
how DROP TABLE works...

Chris


In response to

Responses

pgsql-hackers by date

Next:From: SAKAIDA MasaakiDate: 2002-07-22 03:30:22
Subject: pgbash-2.4a.2 released
Previous:From: Christopher Kings-LynneDate: 2002-07-22 02:13:31
Subject: Re: CREATE/DROP OPERATOR CLASS

pgsql-patches by date

Next:From: Tom LaneDate: 2002-07-22 15:06:38
Subject: Re: More heap tuple header fixes
Previous:From: Manfred KoizarDate: 2002-07-21 21:00:22
Subject: Re: More heap tuple header fixes

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group