Re: Demo patch for DROP COLUMN - Mailing list pgsql-patches
From | Christopher Kings-Lynne |
---|---|
Subject | Re: Demo patch for DROP COLUMN |
Date | |
Msg-id | GNELIHDDFBOCMGBFGEFOKEDOCDAA.chriskl@familyhealth.com.au Whole thread Raw |
In response to | Re: Demo patch for DROP COLUMN (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: Demo patch for DROP COLUMN
|
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
pgsql-patches by date: