Re: Demo patch for DROP COLUMN

"Christopher Kings-Lynne" <chriskl(at)familyhealth(dot)com(dot)au> writes:
> This is the patch of what I've done so far. I'd value any feedback!

A few comments ---

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.

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.)
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.

! for (;;) {
! if (SearchSysCacheExists(ATTNAME,
! ObjectIdGetDatum(myrelid),
! PointerGetDatum(newattname),
! 0, 0)) {
! snprintf(newattname, NAMEDATALEN, "___pg_dropped%d %d", ++n, attnum);
! }
! else break;
! }

Why not just

while (SearchSysCacheExists(...))
{
snprintf(... ++n ...);
}

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. 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. (Dashes or other
choices would work as well. I'm not in favor of control characters
or whitespace in the name though.)

foreach(c, rte->eref->colnames)
{
attnum++;
! if (strcmp(strVal(lfirst(c)), colname) == 0)
{
if (result)
elog(ERROR, "Column reference \"%s\" is ambiguous", colname);
--- 271,278 ----
foreach(c, rte->eref->colnames)
{
attnum++;
! /* Column name must be non-NULL and be a string match */
! if (lfirst(c) != NULL && strcmp(strVal(lfirst(c)), colname) == 0)
{
if (result)
elog(ERROR, "Column reference \"%s\" is ambiguous", colname);

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.

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.

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

regards, tom lane