On Mon, Jun 27, 2011 at 10:43 PM, Noah Misch <n...@leadboat.com> wrote: > 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.
That makes sense, but it reads a bit awkwardly to me. Maybe it's just that the sentence itself is so complicated that I have difficulty understanding it. I guess it's the same problem as with the text you added about hash indexes: without thinking about it, it's hard to understand what territory is covered by the new sentence that is not covered by what's already there. In the case of the hash indexes, I think I have it figured out: we already know that we must get compatible hash values out of logically equal values of different datatypes. But it's possible that the inter-type cast changes the value in some arbitrary way and then compensates for it by defining the hash function in such a way as to compensate. Similarly, for btrees, we need the relative ordering of A and B to remain the same after casting within the opfamily, not to be rearranged somehow. Maybe the text you've got is OK to explain this, but I wonder if there's a way to do it more simply. >> 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. Maybe vile is a strong word, but it seems like a modularity violation: we're basically letting DefineIndex() do some stuff we don't really want done, and then backing it out parts of it that we don't really want done after all. It seems like it would be better to provide DefineIndex with enough information not to do the wrong thing in the first place. Could we maybe pass stmt->oldNode to DefineIndex(), and let it work out the details? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
