Re: [HACKERS] Ignore invalid indexes in pg_dump.

2013-03-28 Thread Bruce Momjian
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.

2013-03-28 Thread Tom Lane
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.

2013-03-28 Thread anara...@anarazel.de


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.

2013-03-28 Thread Bruce Momjian
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.

2013-03-28 Thread Tom Lane
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.

2013-03-28 Thread anara...@anarazel.de


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.

2013-03-28 Thread Bruce Momjian
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.

2013-03-28 Thread Andres Freund
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.

2013-03-28 Thread Tom Lane
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.

2013-03-28 Thread Andres Freund
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

2013-03-26 Thread Tom Lane
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

2013-03-26 Thread Michael Paquier
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

2013-03-20 Thread Simon Riggs
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

2013-03-20 Thread Josh Kupershmidt
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

2013-03-20 Thread Tom Lane
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

2013-03-20 Thread Michael Paquier
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


[HACKERS] Ignore invalid indexes in pg_dump

2013-03-19 Thread Michael Paquier
Hi,

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?

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.

-- 
Michael


20130317_dump_only_valid_index.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