Re: [HACKERS] bumping HASH_VERSION to 3 - Mailing list pgsql-hackers

On Mon, May 15, 2017 at 12:08 AM, Noah Misch <noah@leadboat.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