On Thu, Jan 20, 2011 at 12:57 AM, Noah Misch <n...@leadboat.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
