Thread: Re: [HACKERS] bumping HASH_VERSION to 3
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 +
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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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
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
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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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
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 +
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
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 +