Thread

Commits

  1. pg_upgrade: Handle hash index upgrades more smoothly.

  2. Expand hash indexes more gradually.

  3. Cache hash index's metapage in rel->rd_amcache.

  1. bumping HASH_VERSION to 3

    Robert Haas <robertmhaas@gmail.com> — 2017-03-31T18:17:26Z

    Starting a new thread about this to get more visibility.
    
    Despite the extensive work that has been done on hash indexes this
    release, we have thus far not made any change to the on-disk format
    that is not nominally backward-compatible.  Commit
    293e24e507838733aba4748b514536af2d39d7f2 did make a change for new
    hash indexes, but included backward-compatibility code so that old
    indexes would continue to work.  However, I'd like to also commit
    Mithun Cy's patch to expand hash indexes more gradually -- latest
    version in http://postgr.es/m/CAD__OujD-iBxm91ZcqziaYftWqJxnFqgMv361V9zke83s6ifBg@mail.gmail.com
    -- and that's not backward-compatible.
    
    It would be possible to write code to convert the old metapage format
    to the new metapage format introduced by that patch, and it wouldn't
    be very hard, but I think it would be better to NOT do that, and
    instead force everybody upgrading to v10 to rebuild all of their hash
    indexes.   If we don't do that, then we'll never know whether
    instances of hash index corruption reported against v10 or higher are
    caused by defects in the new code, because there's always the chance
    that the hash index could have been built on a pre-v10 version, got
    corrupted because of the lack of WAL-logging, and then been brought up
    to v10+ via pg_upgrade.  Forcing a reindex in v10 kills three birds
    with one stone:
    
    - No old, not logged, possibly corrupt hash indexes floating around
    after an upgrade to v10.
    - Can remove the backward-compatibility code added by
    293e24e507838733aba4748b514536af2d39d7f2 instead of keeping it around
    forever.
    - No need to worry about doing an in-place upgrade of the metapage for
    the above-mentioned patch.
    
    Thoughts?
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    
    
  2. Re: bumping HASH_VERSION to 3

    Magnus Hagander <magnus@hagander.net> — 2017-03-31T18:19:53Z

    On Fri, Mar 31, 2017 at 8:17 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    
    > Starting a new thread about this to get more visibility.
    >
    > Despite the extensive work that has been done on hash indexes this
    > release, we have thus far not made any change to the on-disk format
    > that is not nominally backward-compatible.  Commit
    > 293e24e507838733aba4748b514536af2d39d7f2 did make a change for new
    > hash indexes, but included backward-compatibility code so that old
    > indexes would continue to work.  However, I'd like to also commit
    > Mithun Cy's patch to expand hash indexes more gradually -- latest
    > version in http://postgr.es/m/CAD__OujD-iBxm91ZcqziaYftWqJxnFqgMv361V9
    > zke83s6ifBg@mail.gmail.com
    > -- and that's not backward-compatible.
    >
    > It would be possible to write code to convert the old metapage format
    > to the new metapage format introduced by that patch, and it wouldn't
    > be very hard, but I think it would be better to NOT do that, and
    > instead force everybody upgrading to v10 to rebuild all of their hash
    > indexes.   If we don't do that, then we'll never know whether
    > instances of hash index corruption reported against v10 or higher are
    > caused by defects in the new code, because there's always the chance
    > that the hash index could have been built on a pre-v10 version, got
    > corrupted because of the lack of WAL-logging, and then been brought up
    > to v10+ via pg_upgrade.  Forcing a reindex in v10 kills three birds
    > with one stone:
    >
    > - No old, not logged, possibly corrupt hash indexes floating around
    > after an upgrade to v10.
    > - Can remove the backward-compatibility code added by
    > 293e24e507838733aba4748b514536af2d39d7f2 instead of keeping it around
    > forever.
    > - No need to worry about doing an in-place upgrade of the metapage for
    > the above-mentioned patch.
    >
    > Thoughts?
    >
    
    Given the state of hash indexes in <= 9.6, I think this is a reasonable
    tradeoff. Most people won't be using them at all today. Those that do will
    have to "pay" with a REINDEX on upgrade. I think the benefits definitely
    outweigh the cost.
    
    So +1 for doing it.
    
    -- 
     Magnus Hagander
     Me: http://www.hagander.net/
     Work: http://www.redpill-linpro.com/
    
  3. Re: bumping HASH_VERSION to 3

    Jesper Pedersen <jesper.pedersen@redhat.com> — 2017-03-31T18:27:26Z

    On 03/31/2017 02:17 PM, Robert Haas wrote:
    > Starting a new thread about this to get more visibility.
    >
    > Despite the extensive work that has been done on hash indexes this
    > release, we have thus far not made any change to the on-disk format
    > that is not nominally backward-compatible.  Commit
    > 293e24e507838733aba4748b514536af2d39d7f2 did make a change for new
    > hash indexes, but included backward-compatibility code so that old
    > indexes would continue to work.  However, I'd like to also commit
    > Mithun Cy's patch to expand hash indexes more gradually -- latest
    > version in http://postgr.es/m/CAD__OujD-iBxm91ZcqziaYftWqJxnFqgMv361V9zke83s6ifBg@mail.gmail.com
    > -- and that's not backward-compatible.
    >
    > It would be possible to write code to convert the old metapage format
    > to the new metapage format introduced by that patch, and it wouldn't
    > be very hard, but I think it would be better to NOT do that, and
    > instead force everybody upgrading to v10 to rebuild all of their hash
    > indexes.   If we don't do that, then we'll never know whether
    > instances of hash index corruption reported against v10 or higher are
    > caused by defects in the new code, because there's always the chance
    > that the hash index could have been built on a pre-v10 version, got
    > corrupted because of the lack of WAL-logging, and then been brought up
    > to v10+ via pg_upgrade.  Forcing a reindex in v10 kills three birds
    > with one stone:
    >
    > - No old, not logged, possibly corrupt hash indexes floating around
    > after an upgrade to v10.
    > - Can remove the backward-compatibility code added by
    > 293e24e507838733aba4748b514536af2d39d7f2 instead of keeping it around
    > forever.
    > - No need to worry about doing an in-place upgrade of the metapage for
    > the above-mentioned patch.
    >
    > Thoughts?
    >
    
    +1
    
    Best regards,
      Jesper
    
    
    
    
  4. Re: bumping HASH_VERSION to 3

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-31T18:48:05Z

    Robert Haas <robertmhaas@gmail.com> writes:
    > Forcing a reindex in v10 kills three birds
    > with one stone:
    
    > - No old, not logged, possibly corrupt hash indexes floating around
    > after an upgrade to v10.
    > - Can remove the backward-compatibility code added by
    > 293e24e507838733aba4748b514536af2d39d7f2 instead of keeping it around
    > forever.
    > - No need to worry about doing an in-place upgrade of the metapage for
    > the above-mentioned patch.
    
    > Thoughts?
    
    +1, as long as we're clear on what will happen when pg_upgrade'ing
    an installation containing hash indexes.  I think a minimum requirement is
    that it succeed and be able to start up, and allow the user to manually
    REINDEX such indexes afterwards.  Bonus points for:
    
    1. teaching pg_upgrade to create a script containing the required REINDEX
    commands.  (I think it's produced scripts for similar requirements in the
    past.)
    
    2. marking the index invalid so that the system would silently ignore it
    until it's been reindexed.  I think there might be adequate infrastructure
    for that already thanks to REINDEX CONCURRENTLY, and it'd just be a matter
    of getting pg_upgrade to hack the indexes' catalog state.  (If not, it's
    probably not worth the trouble.)
    
    A variant on that might just be to not transfer over hash indexes,
    leaving the user to CREATE them rather than REINDEX them.
    
    			regards, tom lane
    
    
    
  5. Re: bumping HASH_VERSION to 3

    Joe Conway <mail@joeconway.com> — 2017-04-03T16:29:23Z

    On 03/31/2017 11:19 AM, Magnus Hagander wrote:
    > On Fri, Mar 31, 2017 at 8:17 PM, Robert Haas <robertmhaas@gmail.com
    > <mailto:robertmhaas@gmail.com>> wrote:
    > 
    >     Starting a new thread about this to get more visibility.
    > 
    >     Despite the extensive work that has been done on hash indexes this
    >     release, we have thus far not made any change to the on-disk format
    >     that is not nominally backward-compatible.  Commit
    >     293e24e507838733aba4748b514536af2d39d7f2 did make a change for new
    >     hash indexes, but included backward-compatibility code so that old
    >     indexes would continue to work.  However, I'd like to also commit
    >     Mithun Cy's patch to expand hash indexes more gradually -- latest
    >     version in
    >     http://postgr.es/m/CAD__OujD-iBxm91ZcqziaYftWqJxnFqgMv361V9zke83s6ifBg@mail.gmail.com
    >     <http://postgr.es/m/CAD__OujD-iBxm91ZcqziaYftWqJxnFqgMv361V9zke83s6ifBg@mail.gmail.com>
    >     -- and that's not backward-compatible.
    > 
    >     It would be possible to write code to convert the old metapage format
    >     to the new metapage format introduced by that patch, and it wouldn't
    >     be very hard, but I think it would be better to NOT do that, and
    >     instead force everybody upgrading to v10 to rebuild all of their hash
    >     indexes.   If we don't do that, then we'll never know whether
    >     instances of hash index corruption reported against v10 or higher are
    >     caused by defects in the new code, because there's always the chance
    >     that the hash index could have been built on a pre-v10 version, got
    >     corrupted because of the lack of WAL-logging, and then been brought up
    >     to v10+ via pg_upgrade.  Forcing a reindex in v10 kills three birds
    >     with one stone:
    > 
    >     - No old, not logged, possibly corrupt hash indexes floating around
    >     after an upgrade to v10.
    >     - Can remove the backward-compatibility code added by
    >     293e24e507838733aba4748b514536af2d39d7f2 instead of keeping it around
    >     forever.
    >     - No need to worry about doing an in-place upgrade of the metapage for
    >     the above-mentioned patch.
    > 
    >     Thoughts?
    > 
    > 
    > Given the state of hash indexes in <= 9.6, I think this is a reasonable
    > tradeoff. Most people won't be using them at all today. Those that do
    > will have to "pay" with a REINDEX on upgrade. I think the benefits
    > definitely outweigh the cost.
    > 
    > So +1 for doing it. 
    
    +1
    
    
    -- 
    Crunchy Data - http://crunchydata.com
    PostgreSQL Support for Secure Enterprises
    Consulting, Training, & Open Source Development
    
    
  6. Re: bumping HASH_VERSION to 3

    Bruce Momjian <bruce@momjian.us> — 2017-04-11T18:23:46Z

    On Fri, Mar 31, 2017 at 02:48:05PM -0400, Tom Lane wrote:
    > +1, as long as we're clear on what will happen when pg_upgrade'ing
    > an installation containing hash indexes.  I think a minimum requirement is
    > that it succeed and be able to start up, and allow the user to manually
    > REINDEX such indexes afterwards.  Bonus points for:
    > 
    > 1. teaching pg_upgrade to create a script containing the required REINDEX
    > commands.  (I think it's produced scripts for similar requirements in the
    > past.)
    > 
    > 2. marking the index invalid so that the system would silently ignore it
    > until it's been reindexed.  I think there might be adequate infrastructure
    > for that already thanks to REINDEX CONCURRENTLY, and it'd just be a matter
    > of getting pg_upgrade to hack the indexes' catalog state.  (If not, it's
    > probably not worth the trouble.)
    
    We already have code to do all of that, but it was removed from
    pg_upgrade in 9.5.  You can still see it in 9.4:
    
    	contrib/pg_upgrade/version_old_8_3.c::old_8_3_invalidate_hash_gin_indexes()
    
    I would be happy to restore that code and make it work for PG 10.
    
    -- 
      Bruce Momjian  <bruce@momjian.us>        http://momjian.us
      EnterpriseDB                             http://enterprisedb.com
    
    + As you are, so once was I.  As I am, so you will be. +
    +                      Ancient Roman grave inscription +
    
    
    
  7. Re: bumping HASH_VERSION to 3

    Robert Haas <robertmhaas@gmail.com> — 2017-04-11T18:53:54Z

    On Tue, Apr 11, 2017 at 2:23 PM, Bruce Momjian <bruce@momjian.us> wrote:
    > On Fri, Mar 31, 2017 at 02:48:05PM -0400, Tom Lane wrote:
    >> +1, as long as we're clear on what will happen when pg_upgrade'ing
    >> an installation containing hash indexes.  I think a minimum requirement is
    >> that it succeed and be able to start up, and allow the user to manually
    >> REINDEX such indexes afterwards.  Bonus points for:
    >>
    >> 1. teaching pg_upgrade to create a script containing the required REINDEX
    >> commands.  (I think it's produced scripts for similar requirements in the
    >> past.)
    >>
    >> 2. marking the index invalid so that the system would silently ignore it
    >> until it's been reindexed.  I think there might be adequate infrastructure
    >> for that already thanks to REINDEX CONCURRENTLY, and it'd just be a matter
    >> of getting pg_upgrade to hack the indexes' catalog state.  (If not, it's
    >> probably not worth the trouble.)
    >
    > We already have code to do all of that, but it was removed from
    > pg_upgrade in 9.5.  You can still see it in 9.4:
    >
    >         contrib/pg_upgrade/version_old_8_3.c::old_8_3_invalidate_hash_gin_indexes()
    >
    > I would be happy to restore that code and make it work for PG 10.
    
    Cool!
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    
    
  8. Re: bumping HASH_VERSION to 3

    Amit Kapila <amit.kapila16@gmail.com> — 2017-05-10T05:55:22Z

    On Tue, Apr 11, 2017 at 11:53 PM, Bruce Momjian <bruce@momjian.us> wrote:
    > On Fri, Mar 31, 2017 at 02:48:05PM -0400, Tom Lane wrote:
    >> +1, as long as we're clear on what will happen when pg_upgrade'ing
    >> an installation containing hash indexes.  I think a minimum requirement is
    >> that it succeed and be able to start up, and allow the user to manually
    >> REINDEX such indexes afterwards.  Bonus points for:
    >>
    >> 1. teaching pg_upgrade to create a script containing the required REINDEX
    >> commands.  (I think it's produced scripts for similar requirements in the
    >> past.)
    >>
    >> 2. marking the index invalid so that the system would silently ignore it
    >> until it's been reindexed.  I think there might be adequate infrastructure
    >> for that already thanks to REINDEX CONCURRENTLY, and it'd just be a matter
    >> of getting pg_upgrade to hack the indexes' catalog state.  (If not, it's
    >> probably not worth the trouble.)
    >
    > We already have code to do all of that, but it was removed from
    > pg_upgrade in 9.5.  You can still see it in 9.4:
    >
    >         contrib/pg_upgrade/version_old_8_3.c::old_8_3_invalidate_hash_gin_indexes()
    >
    
    Thanks for the pointer.
    
    > I would be happy to restore that code and make it work for PG 10.
    >
    
    Attached patch implements the two points suggested by Tom. I will add
    this to PG-10 open issues list so that we don't forget about this
    work, let me know if you think otherwise.
    
    -- 
    With Regards,
    Amit Kapila.
    EnterpriseDB: http://www.enterprisedb.com
    
  9. Re: bumping HASH_VERSION to 3

    Noah Misch <noah@leadboat.com> — 2017-05-15T04:08:37Z

    On Wed, May 10, 2017 at 11:25:22AM +0530, Amit Kapila wrote:
    > On Tue, Apr 11, 2017 at 11:53 PM, Bruce Momjian <bruce@momjian.us> wrote:
    > > On Fri, Mar 31, 2017 at 02:48:05PM -0400, Tom Lane wrote:
    > >> +1, as long as we're clear on what will happen when pg_upgrade'ing
    > >> an installation containing hash indexes.  I think a minimum requirement is
    > >> that it succeed and be able to start up, and allow the user to manually
    > >> REINDEX such indexes afterwards.  Bonus points for:
    > >>
    > >> 1. teaching pg_upgrade to create a script containing the required REINDEX
    > >> commands.  (I think it's produced scripts for similar requirements in the
    > >> past.)
    > >>
    > >> 2. marking the index invalid so that the system would silently ignore it
    > >> until it's been reindexed.  I think there might be adequate infrastructure
    > >> for that already thanks to REINDEX CONCURRENTLY, and it'd just be a matter
    > >> of getting pg_upgrade to hack the indexes' catalog state.  (If not, it's
    > >> probably not worth the trouble.)
    > >
    > > We already have code to do all of that, but it was removed from
    > > pg_upgrade in 9.5.  You can still see it in 9.4:
    > >
    > >         contrib/pg_upgrade/version_old_8_3.c::old_8_3_invalidate_hash_gin_indexes()
    > >
    > 
    > Thanks for the pointer.
    > 
    > > I would be happy to restore that code and make it work for PG 10.
    > >
    > 
    > Attached patch implements the two points suggested by Tom. I will add
    > this to PG-10 open issues list so that we don't forget about this
    > work, let me know if you think otherwise.
    
    [Action required within three days.  This is a generic notification.]
    
    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.
    
    [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
    
    
    
  10. Re: bumping HASH_VERSION to 3

    Robert Haas <robertmhaas@gmail.com> — 2017-05-15T15:27:03Z

    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
    
    
    
  11. Re: bumping HASH_VERSION to 3

    Amit Kapila <amit.kapila16@gmail.com> — 2017-05-16T11:31:11Z

    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
    
    
    
  12. Re: bumping HASH_VERSION to 3

    Robert Haas <robertmhaas@gmail.com> — 2017-05-16T11:44:57Z

    On Tue, May 16, 2017 at 7:31 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
    >> +    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?
    
    Yes.  It's silly to copy a constant string into a new buffer just for
    the heck of it.  Perhaps the old instances should also be cleaned up
    at some point, but even if we don't bother, copying absolutely
    pointless coding into new places isn't getting us anywhere.
    
    > Hmm, "./" is required for non-windows, but as mentioned above, this is
    > not required for our case.
    
    OK.
    
    > I will send an updated patch once we agree on above points.
    
    Sounds good.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    
    
  13. Re: bumping HASH_VERSION to 3

    Amit Kapila <amit.kapila16@gmail.com> — 2017-05-16T13:25:30Z

    On Tue, May 16, 2017 at 5:14 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    > On Tue, May 16, 2017 at 7:31 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
    >
    >> I will send an updated patch once we agree on above points.
    >
    > Sounds good.
    >
    
    Attached patch addresses all the comments as discussed.
    
    -- 
    With Regards,
    Amit Kapila.
    EnterpriseDB: http://www.enterprisedb.com
    
  14. Re: bumping HASH_VERSION to 3

    Robert Haas <robertmhaas@gmail.com> — 2017-05-19T20:54:27Z

    On Tue, May 16, 2017 at 9:25 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
    > On Tue, May 16, 2017 at 5:14 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    >> On Tue, May 16, 2017 at 7:31 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
    >>> I will send an updated patch once we agree on above points.
    >>
    >> Sounds good.
    >
    > Attached patch addresses all the comments as discussed.
    
    Committed.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    
    
  15. Re: bumping HASH_VERSION to 3

    Amit Kapila <amit.kapila16@gmail.com> — 2017-05-20T02:40:34Z

    On Sat, May 20, 2017 at 2:24 AM, Robert Haas <robertmhaas@gmail.com> wrote:
    > On Tue, May 16, 2017 at 9:25 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
    >> On Tue, May 16, 2017 at 5:14 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    >>> On Tue, May 16, 2017 at 7:31 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
    >>>> I will send an updated patch once we agree on above points.
    >>>
    >>> Sounds good.
    >>
    >> Attached patch addresses all the comments as discussed.
    >
    > Committed.
    >
    
    Thanks.
    
    
    
    -- 
    With Regards,
    Amit Kapila.
    EnterpriseDB: http://www.enterprisedb.com
    
    
    
  16. Re: bumping HASH_VERSION to 3

    Bruce Momjian <bruce@momjian.us> — 2017-05-20T23:56:32Z

    On Fri, May 19, 2017 at 04:54:27PM -0400, Robert Haas wrote:
    > On Tue, May 16, 2017 at 9:25 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
    > > On Tue, May 16, 2017 at 5:14 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    > >> On Tue, May 16, 2017 at 7:31 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
    > >>> I will send an updated patch once we agree on above points.
    > >>
    > >> Sounds good.
    > >
    > > Attached patch addresses all the comments as discussed.
    > 
    > Committed.
    
    Just checking, but you reused some of my code from the 8.3-8.4 migration
    in pg_upgrade that was removed a while back, rather than writing it from
    scratch, right?
    
    -- 
      Bruce Momjian  <bruce@momjian.us>        http://momjian.us
      EnterpriseDB                             http://enterprisedb.com
    
    + As you are, so once was I.  As I am, so you will be. +
    +                      Ancient Roman grave inscription +
    
    
    
  17. Re: bumping HASH_VERSION to 3

    Amit Kapila <amit.kapila16@gmail.com> — 2017-05-21T03:41:29Z

    On Sun, May 21, 2017 at 5:26 AM, Bruce Momjian <bruce@momjian.us> wrote:
    > On Fri, May 19, 2017 at 04:54:27PM -0400, Robert Haas wrote:
    >> On Tue, May 16, 2017 at 9:25 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
    >> > On Tue, May 16, 2017 at 5:14 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    >> >> On Tue, May 16, 2017 at 7:31 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
    >> >>> I will send an updated patch once we agree on above points.
    >> >>
    >> >> Sounds good.
    >> >
    >> > Attached patch addresses all the comments as discussed.
    >>
    >> Committed.
    >
    > Just checking, but you reused some of my code from the 8.3-8.4 migration
    > in pg_upgrade that was removed a while back, rather than writing it from
    > scratch, right?
    >
    
    Yes, I have adapted it to what is required for hash indexes, adjusted
    the code for v10 and there are some other minor changes in code.
    
    -- 
    With Regards,
    Amit Kapila.
    EnterpriseDB: http://www.enterprisedb.com
    
    
    
  18. Re: bumping HASH_VERSION to 3

    Bruce Momjian <bruce@momjian.us> — 2017-05-21T03:43:51Z

    On Sun, May 21, 2017 at 09:11:29AM +0530, Amit Kapila wrote:
    > On Sun, May 21, 2017 at 5:26 AM, Bruce Momjian <bruce@momjian.us> wrote:
    > > On Fri, May 19, 2017 at 04:54:27PM -0400, Robert Haas wrote:
    > >> On Tue, May 16, 2017 at 9:25 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
    > >> > On Tue, May 16, 2017 at 5:14 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    > >> >> On Tue, May 16, 2017 at 7:31 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
    > >> >>> I will send an updated patch once we agree on above points.
    > >> >>
    > >> >> Sounds good.
    > >> >
    > >> > Attached patch addresses all the comments as discussed.
    > >>
    > >> Committed.
    > >
    > > Just checking, but you reused some of my code from the 8.3-8.4 migration
    > > in pg_upgrade that was removed a while back, rather than writing it from
    > > scratch, right?
    > >
    > 
    > Yes, I have adapted it to what is required for hash indexes, adjusted
    > the code for v10 and there are some other minor changes in code.
    
    Great, thanks.  It was hard to do originally so I am glad you could
    reuse it.
    
    -- 
      Bruce Momjian  <bruce@momjian.us>        http://momjian.us
      EnterpriseDB                             http://enterprisedb.com
    
    + As you are, so once was I.  As I am, so you will be. +
    +                      Ancient Roman grave inscription +