On Mon, May 15, 2017 at 8:57 PM, Robert Haas <robertmh...@gmail.com> wrote: > On Mon, May 15, 2017 at 12:08 AM, Noah Misch <n...@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. >
Okay, will change. > + 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. > Sure, but the same code pattern is used in all other similar functions in version.c, refer new_9_0_populate_pg_largeobject_metadata. Both the ways will serve the purpose, do you think it makes sense to write this differently? > Compare > this to the logic in create_script_for_cluster_analyze, which prepends > SCRIPT_PREFIX. That is required for .sh or .bat files not for .sql files. I think we need to compare it with logic in new_9_0_populate_pg_largeobject_metadata. > (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.) > Hmm, "./" is required for non-windows, but as mentioned above, this is not required for our case. > + 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? > oops. copy-paste. It passed in my testing because I have not used any different options (like port number) for old or new server. > This might need a visit from pgindent in one or two places, too. > I have run pgindent before sending the previous version, but will again verify the same. I will send an updated patch once we agree on above points. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers