Re: ALTER TYPE 1: recheck index-based constraints

On Thu, Jan 20, 2011 at 12:57 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> There are two distinct questions here:

Agreed.

> (1) Should reindex_relation receive boolean facts from its callers by way of two
> boolean arguments, or by way of one flags vector?
>
> The former seems best when you want every caller to definitely think about which
> answer is right, and the latter when that's not so necessary.  (For a very long
> list of options, the flags might be chosen on other grounds.)  As framed, I'd
> lean toward keeping distinct arguments, as these are important questions.

My main beef with the Boolean flags is that this kind of thing is not too clear:

reindex_relation(myrel, false, false, true, true, false, true,
false, false, true);

Unless you have an excellent memory, you can't tell what the heck
that's doing without flipping back and forth between the function
definition and the call site. With a bit-field, it's a lot easier to
glance at the call site and have a clue what's going on. We're of
course not quite to the point of that exaggerated example yet.

> However, suppose we inverted both flags, say REINDEX_SKIP_CONSTRAINT_CHECKS and
> REINDEX_ALLOW_OLD_INDEX_USE.  Then, flags = 0 can hurt performance but not
> correctness.  That's looking like a win.

I prefer the positive sense for those flags because I think it's more
clear. There aren't so many call sites or so many people using this
that we have to worry about what people are going to do in new calling
locations; getting it right in any new code shouldn't be a
consideration.

> (2) Should reindex_relation frame its boolean arguments in terms of what the
> caller did (heap_rebuilt, tuples_changed), or in terms of what reindex_relation
> will be doing (check_constraints, suppress_index_use)?

Yeah, I know we're doing the former now, but I think it's just getting
confusing for exactly the reasons you state:

> I agree that both heap_rebuilt and tuples_changed are bad abstractions.
> TRUNCATE is lying about the former, and the latter, as you say, is never really
> correct.  column_values_changed, perhaps.  tuples_could_now_violate_constraints
> would be correct, but that's just a bad spelling for REINDEX_CHECK_CONSTRAINTS.
> I guess the equivalent long-winded improvement for heap_rebuilt would be
> indexes_still_valid_for_snapshotnow.  Eh.

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