Re: bumping HASH_VERSION to 3

On Mon, May 15, 2017 at 8:57 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 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.
>

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