Re: Avoid index rebuilds for no-rewrite ALTER TABLE ALTER TYPE

On Mon, Jun 27, 2011 at 10:43 PM, Noah Misch <noah(at)leadboat(dot)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 <noah(at)leadboat(dot)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.GA18733@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