On Mon, Jun 27, 2011 at 03:45:43PM -0400, Robert Haas wrote: > On Wed, Jun 15, 2011 at 1:03 AM, Noah Misch <n...@leadboat.com> wrote: > > [patch to avoid index rebuilds] > > With respect to the documentation hunks, it seems to me that the first > hunk might be made clearer by leaving the paragraph of which it is a > part as-is, and adding another paragraph afterwards beginning with the > words "In addition".
The added restriction elaborates on the transitivity requirement, so I wanted to keep the new language adjacent to that. > I am not sure whether the second hunk is > necessary at all. Doesn't the existing language cover the same > territory as what you've added? The first hunk updates the contract for btree families, and the second updates the contract for hash families. I kept the second instance a bit terse since it follows soon after the similar text for B-tree. > I think that the variables in ATPostAlterTypeCleanup() could be better > named. They appear to be values, when in fact they are ListCells. > Honestly I'd probably just use l1 and l2, but if you want to insist on > some more mnemonic naming it should probably be something that sounds > vaguely list-ish. Okay; I'll do that in the next version. Either l1/l2 or maybe oid_item/def_item like we use in postgres.c. > As you no doubt expected, my eyes was immediately drawn to the > index-resurrection hack. Reviewing the thread, I see that you asked > about that in January and never got feedback. I have to say that what > you've done here looks like a pretty vile hack, but it's hard to say > for sure without knowing what to compare it against. You made > reference to this being smaller and simpler than updating the index > definition in place - can you give a sketch of what would need to be > done if we went that route instead? In "at7-index-opfamily.patch" attached to http://archives.postgresql.org/message-id/20110113230124.ga18...@tornado.gateway.2wire.net check out the code following the comment "/* The old index is compatible. Update catalogs. */" until the end of the function. That code would need updates for per-column collations, and it incorrectly reuses values/nulls/replace arrays. It probably does not belong in tablecmds.c, either. However, it gives the right general outline. It would be valuable to avoid introducing a second chunk of code that knows everything about the catalog entries behind an index. That's what led me to the put forward the most recent version as best. What do you find vile about that approach? I wasn't comfortable with it at first, because I suspected the checks in RelationPreserveStorage() might be important for correctness. Having studied it some more, though, I think they just reflect the narrower needs of its current sole user. Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
