Re: bumping HASH_VERSION to 3

On Mon, May 15, 2017 at 12:08 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> The above-described topic is currently a PostgreSQL 10 open item. Robert,
> since you committed the patch believed to have created it, you own this open
> item. If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know. Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message. Include a date for your subsequent status update. Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10. Consequently, I will appreciate your efforts
> toward speedy resolution. Thanks.

I don't believe this patch can be committed to beta1 which wraps in
just a few hours; it seems to need a bit of work. I'll update again
by Friday based on how review unfolds this week. I anticipate
committing something on Wednesday or Thursday unless Bruce picks this
one up.

+ /* find hash and gin indexes */
+ res = executeQueryOrDie(conn,
+ "SELECT n.nspname, c.relname "
+ "FROM pg_catalog.pg_class c, "
+ " pg_catalog.pg_index i, "
+ " pg_catalog.pg_am a, "
+ " pg_catalog.pg_namespace n "
+ "WHERE i.indexrelid = c.oid AND "
+ " c.relam = a.oid AND "
+ " c.relnamespace = n.oid AND "
+ " a.amname = 'hash'"

Comment doesn't match code.

+ if (!check_mode && db_used)
+ /* mark hash indexes as invalid */
+ PQclear(executeQueryOrDie(conn,

Given that we have a comment here, I'd put curly braces around this block.

+ snprintf(output_path, sizeof(output_path), "reindex_hash.sql");

This looks suspiciously pointless. The contents of output_path will
always be precisely "reindex_hash.sql"; you could just char
*output_path = "reindex_hash.sql" and get the same effect. Compare
this to the logic in create_script_for_cluster_analyze, which prepends
SCRIPT_PREFIX. (I am not sure why it's necessary to prepend "./" on
Windows, but if it's needed in that case, maybe it's needed in this
case, too.)

+ start_postmaster(&new_cluster, true);
+ old_9_6_invalidate_hash_indexes(&old_cluster, false);
+ stop_postmaster(false);

Won't this cause the hash indexes to be invalided in the old cluster
rather than the new one?

This might need a visit from pgindent in one or two places, too.

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