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

On Tue, Jun 28, 2011 at 3:40 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Tue, Jun 28, 2011 at 02:11:11PM -0400, Robert Haas wrote:
>> 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.
>
> That's basically right, I think.  Presently, there is no connection between
> casts and operator family notions of equality.  For example, a cast can change
> the hash value.  In general, that's not wrong.  However, I wish to forbid it
> when some hash operator family covers both the source and destination types of
> the cast.  Note that, I don't especially care whether the stored bits changed or
> not.  I just want casts to preserve equality when an operator family defines
> equality across the types involved in the cast.  The specific way of
> articulating that is probably vulnerable to improvement.
>
>> > 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?
>
> True.  I initially shied away from that, because we assume somewhat deep into
> the stack that the new relation will have pg_class.oid = pg_class.relfilenode.
> Here's the call stack in question:
>
>        RelationBuildLocalRelation
>        heap_create
>        index_create
>        DefineIndex
>        ATExecAddIndex
>
> Looking at it again, it wouldn't bring the end of the world to add a relfilenode
> argument to each. None of those have more than four callers.

Yeah. Those functions take an awful lot of arguments, which suggests
that some refactoring might be in order, but I still think it's
cleaner to add another argument than to change the state around
after-the-fact.

> ATExecAddIndex()
> would then call RelationPreserveStorage() before calling DefineIndex(), which
> would in turn put things in a correct state from the start.  Does that seem
> appropriate?  Offhand, I do like it better than what I had.

I wish we could avoid the whole death-and-resurrection thing
altogether, but off-hand I'm not seeing a real clean way to do that.
At the very least we had better comment it to death.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company