Re: ALTER TYPE 1: recheck index-based constraints

On Wed, Jan 12, 2011 at 8:56 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jan 12, 2011 at 8:14 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> Something like the attached?
>
> I haven't really analyzed in this detail, but yes, approximately
> something sorta like that.

I looked this over some more tonight. The name "tuples_changed" is
giving me some angst, because if we rewrote the heap... the tuples
changed. I think what you intend this to indicate whether the tuples
have changed in a semantic sense, ignoring CTIDs and so on. But it's
not even quite that, either, because ExecuteTruncate() is passing
false, and the set of tuples has probably changed in that case. It
seems that the cases here are:

1. When ALTER TABLE causes a rewrite, we set heap_rebuilt = true and
tuples_changed = true. This causes constraints to be revalidated and
suppresses use of indexes while the rebuild is in progress.
2. When VACUUM FULL or CLUSTER causes a rewrite, we set heap_rebuilt =
true and tuples_changed = false. This avoids constraint revalidation
but still suppresses use of indexes while the rebuild is in progress.
3. When TRUNCATE or REINDEX is invoked, we set heap_rebuilt = false
and tuples_changed = false. Here we neither revalidate constraints
nor suppress use of indexes while the rebuild is in progress.

The first question I asked myself is whether the above is actually
correct, and whether it's possible to simplify back to two cases. So:
The REINDEX case should definitely avoid suppressing the use of the
old index while the new one is rebuilt; I'm not really sure it matters
what TRUNCATE does, since we're going to be operating on a non-system
catalog on which we hold AccessExclusiveLock; the VACUUM FULL/CLUSTER
case certainly needs to suppress uses of indexes, because it can be
used on system catalogs, which may need to be used during the rebuild
itself. Thus #2 and #3 must be distinct. #1 must be distinct from #2
both for performance reasons and to prevent deadlocks when using
VACUUM FULL or CLUSTER on a system catalog (ALTER TABLE can't be so
used, so it's safe for it to not worry about this problem). So I
think the logic is correct and not overly complex.

I think what might make sense is - instead of adding another Boolean
argument, change the heap_rebuilt argument to int flags, and define
REINDEX_CHECK_CONSTRAINTS and REINDEX_SUPPRESS_INDEX_USE as OR-able
flags. I think that's more clear about what's actually going on than
heap_rebuit/tuples_changed.

Thoughts?

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