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

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

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