Re: [HACKERS] Ignore invalid indexes in pg_dump.
On Tue, Mar 26, 2013 at 09:43:54PM +, Tom Lane wrote: Ignore invalid indexes in pg_dump. Dumping invalid indexes can cause problems at restore time, for example if the reason the index creation failed was because it tried to enforce a uniqueness condition not satisfied by the table's data. Also, if the index creation is in fact still in progress, it seems reasonable to consider it to be an uncommitted DDL change, which pg_dump wouldn't be expected to dump anyway. Back-patch to all active versions, and teach them to ignore invalid indexes in servers back to 8.2, where the concept was introduced. This commit affects pg_upgrade. You might remember we had to patch pg_upgrade to prevent it from migrating clusters with invalid indexes in December, 2012: http://momjian.us/main/blogs/pgblog/2012.html#December_14_2012 This was released on February 7, 2013 in 9.2.3 and other back branches: http://www.postgresql.org/docs/9.2/static/release-9-2-3.html This git commit means that pg_upgrade can again migrate systems with invalid indexes as pg_upgrade can just skip migrating them because pg_dump will dump the SQL commands to create them --- previously pg_upgrade threw an error. Should I just patch pg_upgrade to remove the indisvalid, skip indisvalid indexes, and backpatch it? Users should be using the version of pg_upgrade to match new pg_dump. Is there any case where they don't match? Do I still need to check for indisready? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump.
Bruce Momjian br...@momjian.us writes: Should I just patch pg_upgrade to remove the indisvalid, skip indisvalid indexes, and backpatch it? Users should be using the version of pg_upgrade to match new pg_dump. Is there any case where they don't match? Do I still need to check for indisready? Yeah, if you can just ignore !indisvalid indexes that should work fine. I see no need to look at indisready if you're doing that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump.
Tom Lane t...@sss.pgh.pa.us schrieb: Bruce Momjian br...@momjian.us writes: Should I just patch pg_upgrade to remove the indisvalid, skip indisvalid indexes, and backpatch it? Users should be using the version of pg_upgrade to match new pg_dump. Is there any case where they don't match? Do I still need to check for indisready? Yeah, if you can just ignore !indisvalid indexes that should work fine. I see no need to look at indisready if you're doing that. You need to look at inisready in 9.2 since thats used for about to be dropped indexes. No? Andres --- Please excuse brevity and formatting - I am writing this on my mobile phone. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump.
On Thu, Mar 28, 2013 at 10:31:51PM +0100, anara...@anarazel.de wrote: Tom Lane t...@sss.pgh.pa.us schrieb: Bruce Momjian br...@momjian.us writes: Should I just patch pg_upgrade to remove the indisvalid, skip indisvalid indexes, and backpatch it? Users should be using the version of pg_upgrade to match new pg_dump. Is there any case where they don't match? Do I still need to check for indisready? Yeah, if you can just ignore !indisvalid indexes that should work fine. I see no need to look at indisready if you're doing that. You need to look at inisready in 9.2 since thats used for about to be dropped indexes. No? Well, if it is dropped, pg_dump will not dump it. At this point though, pg_upgrade is either running in check mode, or it is the only user. I think we are OK. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump.
anara...@anarazel.de and...@anarazel.de writes: Tom Lane t...@sss.pgh.pa.us schrieb: Yeah, if you can just ignore !indisvalid indexes that should work fine. I see no need to look at indisready if you're doing that. You need to look at inisready in 9.2 since thats used for about to be dropped indexes. No? No, he doesn't need to look at indisready/indislive; if either of those flags are off then indisvalid should certainly be off too. (If it isn't, queries against the table are already in trouble.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump.
Tom Lane t...@sss.pgh.pa.us schrieb: anara...@anarazel.de and...@anarazel.de writes: Tom Lane t...@sss.pgh.pa.us schrieb: Yeah, if you can just ignore !indisvalid indexes that should work fine. I see no need to look at indisready if you're doing that. You need to look at inisready in 9.2 since thats used for about to be dropped indexes. No? No, he doesn't need to look at indisready/indislive; if either of those flags are off then indisvalid should certainly be off too. (If it isn't, queries against the table are already in trouble.) 9.2 represents inisdead as live !ready, doesn't it? So just looking at indislive will include about to be dropped or partially dropped indexes? Andres --- Please excuse brevity and formatting - I am writing this on my mobile phone. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump.
On Thu, Mar 28, 2013 at 10:47:55PM +0100, anara...@anarazel.de wrote: Tom Lane t...@sss.pgh.pa.us schrieb: anara...@anarazel.de and...@anarazel.de writes: Tom Lane t...@sss.pgh.pa.us schrieb: Yeah, if you can just ignore !indisvalid indexes that should work fine. I see no need to look at indisready if you're doing that. You need to look at inisready in 9.2 since thats used for about to be dropped indexes. No? No, he doesn't need to look at indisready/indislive; if either of those flags are off then indisvalid should certainly be off too. (If it isn't, queries against the table are already in trouble.) 9.2 represents inisdead as live !ready, doesn't it? So just looking at indislive will include about to be dropped or partially dropped indexes? Where do you see 'inisdead' defined? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump.
On 2013-03-28 17:54:08 -0400, Bruce Momjian wrote: On Thu, Mar 28, 2013 at 10:47:55PM +0100, anara...@anarazel.de wrote: Tom Lane t...@sss.pgh.pa.us schrieb: anara...@anarazel.de and...@anarazel.de writes: Tom Lane t...@sss.pgh.pa.us schrieb: Yeah, if you can just ignore !indisvalid indexes that should work fine. I see no need to look at indisready if you're doing that. You need to look at inisready in 9.2 since thats used for about to be dropped indexes. No? No, he doesn't need to look at indisready/indislive; if either of those flags are off then indisvalid should certainly be off too. (If it isn't, queries against the table are already in trouble.) 9.2 represents inisdead as live !ready, doesn't it? So just looking at indislive will include about to be dropped or partially dropped indexes? Where do you see 'inisdead' defined? Sorry, its named the reverse, indislive. And its only there in 9.3+... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump.
anara...@anarazel.de and...@anarazel.de writes: 9.2 represents inisdead as live !ready, doesn't it? So just looking at indislive will include about to be dropped or partially dropped indexes? Ooooh ... you're right, in 9.2 (only) we need to check both indisvalid and indisready (cf IndexIsValid() macro in the different branches). So that's actually a bug in the committed pg_dump patch, too. I'll fix that shortly. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump.
On 2013-03-28 17:35:08 -0400, Bruce Momjian wrote: On Thu, Mar 28, 2013 at 10:31:51PM +0100, anara...@anarazel.de wrote: Tom Lane t...@sss.pgh.pa.us schrieb: Bruce Momjian br...@momjian.us writes: Should I just patch pg_upgrade to remove the indisvalid, skip indisvalid indexes, and backpatch it? Users should be using the version of pg_upgrade to match new pg_dump. Is there any case where they don't match? Do I still need to check for indisready? Yeah, if you can just ignore !indisvalid indexes that should work fine. I see no need to look at indisready if you're doing that. You need to look at inisready in 9.2 since thats used for about to be dropped indexes. No? Well, if it is dropped, pg_dump will not dump it. At this point though, pg_upgrade is either running in check mode, or it is the only user. I think we are OK. Its about DROP INDEX CONCURRENTLY which can leave indexes in a partial state visible to the outside. Either just transiently while a DROP CONCURRENLTY is going on or even permanently if the server crashed during such a drop or similar. c.f. index.c:index_drop Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump
Michael Paquier michael.paqu...@gmail.com writes: On top of checking indisvalid, I think that some additional checks on indislive and indisready are also necessary. Those are not necessary, as an index that is marked indisvalid should certainly also have those flags set. If it didn't require making two new version distinctions in getIndexes(), I'd be okay with the extra checks; but as-is I think the maintenance pain this would add greatly outweighs any likely value. I've committed this in the simpler form that just adds indisvalid checks to the appropriate version cases. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump
On Wed, Mar 27, 2013 at 6:47 AM, Tom Lane t...@sss.pgh.pa.us wrote: Michael Paquier michael.paqu...@gmail.com writes: On top of checking indisvalid, I think that some additional checks on indislive and indisready are also necessary. Those are not necessary, as an index that is marked indisvalid should certainly also have those flags set. If it didn't require making two new version distinctions in getIndexes(), I'd be okay with the extra checks; but as-is I think the maintenance pain this would add greatly outweighs any likely value. I've committed this in the simpler form that just adds indisvalid checks to the appropriate version cases. Thanks. -- Michael
Re: [HACKERS] Ignore invalid indexes in pg_dump
On 20 March 2013 02:51, Michael Paquier michael.paqu...@gmail.com wrote: If failures happen with CREATE INDEX CONCURRENTLY, the system will be let with invalid indexes. I don't think that the user would like to see invalid indexes of an existing system being recreated as valid after a restore. So why not removing from a dump invalid indexes with something like the patch attached? This should perhaps be applied in pg_dump for versions down to 8.2 where CREATE INDEX CONCURRENTLY has been implemented? Invalid also means currently-in-progress, so it would be better to keep them in. I noticed some recent discussions about that: http://www.postgresql.org/message-id/20121207141236.gb4...@alvh.no-ip.org In this case the problem has been fixed in pg_upgrade directly. That is valid because the index build is clearly not in progress. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump
On Wed, Mar 20, 2013 at 2:00 AM, Simon Riggs si...@2ndquadrant.com wrote: On 20 March 2013 02:51, Michael Paquier michael.paqu...@gmail.com wrote: If failures happen with CREATE INDEX CONCURRENTLY, the system will be let with invalid indexes. I don't think that the user would like to see invalid indexes of an existing system being recreated as valid after a restore. So why not removing from a dump invalid indexes with something like the patch attached? This should perhaps be applied in pg_dump for versions down to 8.2 where CREATE INDEX CONCURRENTLY has been implemented? Invalid also means currently-in-progress, so it would be better to keep them in. For invalid indexes which are left hanging around in the database, if the index definition is included by pg_dump, it will likely cause pain during the restore. If the index build failed the first time and hasn't been manually dropped and recreated since then, it's a good bet it will fail the next time. Errors during restore can be more than just a nuisance; consider restores with --single-transaction. And if the index is simply currently-in-progress, it seems like the expected behavior would be for pg_dump to ignore it anyway. We don't include other DDL objects which are not yet committed while pg_dump is running. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump
Josh Kupershmidt schmi...@gmail.com writes: On Wed, Mar 20, 2013 at 2:00 AM, Simon Riggs si...@2ndquadrant.com wrote: Invalid also means currently-in-progress, so it would be better to keep them in. For invalid indexes which are left hanging around in the database, if the index definition is included by pg_dump, it will likely cause pain during the restore. If the index build failed the first time and hasn't been manually dropped and recreated since then, it's a good bet it will fail the next time. Errors during restore can be more than just a nuisance; consider restores with --single-transaction. And if the index is simply currently-in-progress, it seems like the expected behavior would be for pg_dump to ignore it anyway. We don't include other DDL objects which are not yet committed while pg_dump is running. I had been on the fence about what to do here, but I find Josh's arguments persuasive, particularly the second one. Why shouldn't we consider an in-progress index to be an uncommitted DDL change? (Now admittedly, there won't *be* any uncommitted ordinary DDL on tables while pg_dump is running, because it takes AccessShareLock on all tables. But there could easily be uncommitted DDL against other types of database objects, which pg_dump won't even see.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Ignore invalid indexes in pg_dump
On Thu, Mar 21, 2013 at 12:58 AM, Tom Lane t...@sss.pgh.pa.us wrote: I had been on the fence about what to do here, but I find Josh's arguments persuasive, particularly the second one. Why shouldn't we consider an in-progress index to be an uncommitted DDL change? (Now admittedly, there won't *be* any uncommitted ordinary DDL on tables while pg_dump is running, because it takes AccessShareLock on all tables. But there could easily be uncommitted DDL against other types of database objects, which pg_dump won't even see.) +1. Playing it safe is a better thing to do for sure, especially if a restore would fail. I didn't think about that first... On top of checking indisvalid, I think that some additional checks on indislive and indisready are also necessary. As indisready has been introduced in 8.3 and indislive has been added in 9.3, the attached patch is good I think. I also added a note in the documentation about invalid indexes not being dumped. Perhaps this patch should be backpatched to previous versions in order to have the same consistent behavior. Regards, -- Michael 20130321_no_dump_indisvalid.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
