Re: Demo patch for DROP COLUMN - Mailing list pgsql-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