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

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.

> 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.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.

Thanks,
nm