Thread

1. Bugs in CREATE/DROP INDEX CONCURRENTLY

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-05T23:56:40Z

   1.  These operations think they can use ordinary heap_update operations
   to change pg_index entries when they don't have exclusive lock on the
   parent table.  The lack of ex-lock means that another backend could be
   currently loading up its list of index OIDs for the table --- and since
   it scans pg_index with SnapshotNow to do that, the heap_update could
   result in the other backend failing to see this index *at all*.  That's
   okay if it causes the other backend to not use the index for scanning...
   but not okay if it causes the other backend to fail to make index
   entries it is supposed to make.
   
   I think this could possibly be fixed by using nontransactional
   update-in-place when we're trying to change indisvalid and/or
   indisready, but I've not really thought through the details.
   
   2.  DROP INDEX CONCURRENTLY doesn't bother to do
   TransferPredicateLocksToHeapRelation until long after it's invalidated
   the index.  Surely that's no good?  Is it even possible to do that
   correctly, when we don't have a lock that will prevent new predicate
   locks from being taken out meanwhile?
   
   			regards, tom lane
   
   
   

2. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

   Simon Riggs <simon@2ndquadrant.com> — 2012-10-06T19:35:47Z

   On 6 October 2012 00:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > 1.  These operations think they can use ordinary heap_update operations
   > to change pg_index entries when they don't have exclusive lock on the
   > parent table.  The lack of ex-lock means that another backend could be
   > currently loading up its list of index OIDs for the table --- and since
   > it scans pg_index with SnapshotNow to do that, the heap_update could
   > result in the other backend failing to see this index *at all*.  That's
   > okay if it causes the other backend to not use the index for scanning...
   > but not okay if it causes the other backend to fail to make index
   > entries it is supposed to make.
   >
   > I think this could possibly be fixed by using nontransactional
   > update-in-place when we're trying to change indisvalid and/or
   > indisready, but I've not really thought through the details.
   >
   > 2.  DROP INDEX CONCURRENTLY doesn't bother to do
   > TransferPredicateLocksToHeapRelation until long after it's invalidated
   > the index.  Surely that's no good?  Is it even possible to do that
   > correctly, when we don't have a lock that will prevent new predicate
   > locks from being taken out meanwhile?
   
   I'm in the middle of reviewing other fixes there, so will comment
   soon, just not right now.
   
   -- 
    Simon Riggs                   http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

3. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

   Greg Stark <stark@mit.edu> — 2012-10-07T21:54:00Z

   On Sat, Oct 6, 2012 at 12:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > 1.  These operations think they can use ordinary heap_update operations
   > to change pg_index entries when they don't have exclusive lock on the
   > parent table.
   
   I wonder if we need a manual that lists exhaustively what operations
   can be taken with locks on various objects. Relying on understanding
   every other possible operation in the system and knowing which
   operations might or might not happen is clearly getting us into
   trouble.
   
   This would be a lot less worrying if we had a regression test suite
   that could reliably test race conditions like this.
   
   > The lack of ex-lock means that another backend could be
   > currently loading up its list of index OIDs for the table --- and since
   > it scans pg_index with SnapshotNow to do that, the heap_update could
   > result in the other backend failing to see this index *at all*.  That's
   > okay if it causes the other backend to not use the index for scanning...
   > but not okay if it causes the other backend to fail to make index
   > entries it is supposed to make.
   
   I marked this email to read later but now I'm reading it and I realize
   I'm not sure I can really help. Using a nontransactional update to
   flag indisready makes sense to me since the whole point of the wait is
   that we've waited long enough that anyone should see this record. I
   guess that's what I had in mind was happening when I wrote the update.
   But I'm not sure what other requirements nontransactional updates have
   to work properly.
   
   
   -- 
   greg
   
   
   

4. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

   Simon Riggs <simon@2ndquadrant.com> — 2012-10-09T09:11:13Z

   On 6 October 2012 00:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > 1.  These operations think they can use ordinary heap_update operations
   > to change pg_index entries when they don't have exclusive lock on the
   > parent table.  The lack of ex-lock means that another backend could be
   > currently loading up its list of index OIDs for the table --- and since
   > it scans pg_index with SnapshotNow to do that, the heap_update could
   > result in the other backend failing to see this index *at all*.  That's
   > okay if it causes the other backend to not use the index for scanning...
   > but not okay if it causes the other backend to fail to make index
   > entries it is supposed to make.
   >
   > I think this could possibly be fixed by using nontransactional
   > update-in-place when we're trying to change indisvalid and/or
   > indisready, but I've not really thought through the details.
   
   Only solution there is to fix SnapshotNow, as we discussed last
   Christmas. I'll dig out my patch for that, which IIRC I was nervous of
   committing at last minute into 9.2.
   
   > 2.  DROP INDEX CONCURRENTLY doesn't bother to do
   > TransferPredicateLocksToHeapRelation until long after it's invalidated
   > the index.  Surely that's no good?  Is it even possible to do that
   > correctly, when we don't have a lock that will prevent new predicate
   > locks from being taken out meanwhile?
   
   No idea there. Input appreciated.
   
   -- 
    Simon Riggs                   http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

5. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

   Michael Paquier <michael.paquier@gmail.com> — 2012-10-09T23:09:20Z

   On Tue, Oct 9, 2012 at 6:11 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
   
   > On 6 October 2012 00:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > > 1.  These operations think they can use ordinary heap_update operations
   > > to change pg_index entries when they don't have exclusive lock on the
   > > parent table.  The lack of ex-lock means that another backend could be
   > > currently loading up its list of index OIDs for the table --- and since
   > > it scans pg_index with SnapshotNow to do that, the heap_update could
   > > result in the other backend failing to see this index *at all*.  That's
   > > okay if it causes the other backend to not use the index for scanning...
   > > but not okay if it causes the other backend to fail to make index
   > > entries it is supposed to make.
   > >
   > > I think this could possibly be fixed by using nontransactional
   > > update-in-place when we're trying to change indisvalid and/or
   > > indisready, but I've not really thought through the details.
   >
   > Only solution there is to fix SnapshotNow, as we discussed last
   > Christmas. I'll dig out my patch for that, which IIRC I was nervous of
   > committing at last minute into 9.2.
   >
   Hi Simon,
   Do you have an URL to this patch?
   -- 
   Michael Paquier
   http://michael.otacoo.com
   

6. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

   Andres Freund <andres@2ndquadrant.com> — 2012-11-24T17:38:18Z

   On 2012-10-05 19:56:40 -0400, Tom Lane wrote:
   > 1.  These operations think they can use ordinary heap_update operations
   > to change pg_index entries when they don't have exclusive lock on the
   > parent table.  The lack of ex-lock means that another backend could be
   > currently loading up its list of index OIDs for the table --- and since
   > it scans pg_index with SnapshotNow to do that, the heap_update could
   > result in the other backend failing to see this index *at all*.  That's
   > okay if it causes the other backend to not use the index for scanning...
   > but not okay if it causes the other backend to fail to make index
   > entries it is supposed to make.
   >
   > I think this could possibly be fixed by using nontransactional
   > update-in-place when we're trying to change indisvalid and/or
   > indisready, but I've not really thought through the details.
   
   I couldn't really think of any realistic method to fix this other than
   update in place. I thought about it for a while and I think it should
   work, but I have to say it makes me slightly uneasy.
   
   If we could could ensure both land on the same page it would be possible
   to fix in a nicer way, but thats not really possible. Especially not in
   any way thats backpatchable.
   
   Unless somebody has a better idea I am going to write a patch for that.
   
   Greetings,
   
   Andres Freund
   
   -- 
    Andres Freund	                   http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

7. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-27T03:44:49Z

   Andres Freund <andres@2ndquadrant.com> writes:
   > On 2012-10-05 19:56:40 -0400, Tom Lane wrote:
   >> I think this could possibly be fixed by using nontransactional
   >> update-in-place when we're trying to change indisvalid and/or
   >> indisready, but I've not really thought through the details.
   
   > I couldn't really think of any realistic method to fix this other than
   > update in place. I thought about it for a while and I think it should
   > work, but I have to say it makes me slightly uneasy.
   
   I looked through the code a bit, and I think we should be able to make
   this work, but note there are also places that update indcheckxmin using
   heap_update, and that's just as dangerous as changing the other two
   flags via heap_update, if you don't have exclusive lock on the table.
   This is going to need some careful thought, because we currently expect
   that such an update will set the pg_index row's xmin to the current XID,
   which is something an in-place update can *not* do.  I think this is a
   non-problem during construction of a new index, since the xmin ought to
   be the current XID already anyway, but it's less clear what to do in
   REINDEX.  In the short term there may be no problem since REINDEX takes
   exclusive lock on the parent table anyway (and hence could safely do
   a transactional update) but if we ever want REINDEX CONCURRENTLY we'll
   need a better answer.
   
   			regards, tom lane
   
   
   

8. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

   Andres Freund <andres@2ndquadrant.com> — 2012-11-27T11:18:06Z

   On 2012-11-26 22:44:49 -0500, Tom Lane wrote:
   > Andres Freund <andres@2ndquadrant.com> writes:
   > > On 2012-10-05 19:56:40 -0400, Tom Lane wrote:
   > >> I think this could possibly be fixed by using nontransactional
   > >> update-in-place when we're trying to change indisvalid and/or
   > >> indisready, but I've not really thought through the details.
   >
   > > I couldn't really think of any realistic method to fix this other than
   > > update in place. I thought about it for a while and I think it should
   > > work, but I have to say it makes me slightly uneasy.
   >
   > I looked through the code a bit, and I think we should be able to make
   > this work, but note there are also places that update indcheckxmin using
   > heap_update, and that's just as dangerous as changing the other two
   > flags via heap_update, if you don't have exclusive lock on the table.
   > This is going to need some careful thought, because we currently expect
   > that such an update will set the pg_index row's xmin to the current XID,
   > which is something an in-place update can *not* do.  I think this is a
   > non-problem during construction of a new index, since the xmin ought to
   > be the current XID already anyway, but it's less clear what to do in
   > REINDEX.  In the short term there may be no problem since REINDEX takes
   > exclusive lock on the parent table anyway (and hence could safely do
   > a transactional update) but if we ever want REINDEX CONCURRENTLY we'll
   > need a better answer.
   
   Isn't setting indexcheckxmin already problematic in the case of CREATE
   INDEX CONCURRENTLY? index_build already runs in a separate transaction
   there.
   
   I am really surprised that we haven't seen any evidence of a problem
   with this. On a database with a busy & bigger catalog that ought to be
   a real problem.
   
   I wonder whether we couldn't fix it by introducing a variant/wrapper of
   heap_fetch et al. that follows t_ctid if the tuple became invisible
   "recently". That ought to be able to fix most of these issues in a
   pretty general fashion.
   That would make a nice implementation of REINDEX CONCURRENTLY easier as
   well...
   
   Btw, even if we manage to find a sensible fix for this I would suggest
   postponing it after the next back branch release.
   
   Greetings,
   
   Andres Freund
   
   --
    Andres Freund	                   http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

9. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-27T18:45:08Z

   Andres Freund <andres@2ndquadrant.com> writes:
   > On 2012-11-26 22:44:49 -0500, Tom Lane wrote:
   >> I looked through the code a bit, and I think we should be able to make
   >> this work, but note there are also places that update indcheckxmin using
   >> heap_update, and that's just as dangerous as changing the other two
   >> flags via heap_update, if you don't have exclusive lock on the table.
   
   > Isn't setting indexcheckxmin already problematic in the case of CREATE
   > INDEX CONCURRENTLY? index_build already runs in a separate transaction
   > there.
   
   Yeah, you are right, except that AFAICS indcheckxmin is really only
   needed for regular non-concurrent CREATE INDEX, which needs it because
   it commits without waiting for readers that might be bothered by broken
   HOT chains.  In a concurrent CREATE INDEX, we handle that problem by
   waiting out all such readers before setting indisvalid.  So the
   concurrent code path should not be bothering to set indcheckxmin at all,
   I think.  (This is underdocumented.)
   
   Looking closer at the comment in reindex_index, what it's really full of
   angst about is that simple_heap_update will update the tuple's xmin
   *when we would rather that it didn't*.  So switching to update-in-place
   there will solve a problem, not create one.
   
   In short, all flag changes in pg_index should be done by
   update-in-place, and the tuple's xmin will never change from the
   original creating transaction (until frozen).
   
   What we want the xmin to do, for indcheckxmin purposes, is reflect the
   time at which the index started to be included in HOT-safety decisions.
   Since we're never going to move the xmin, that means that *we cannot
   allow REINDEX to mark a dead index live again*.  Once DROP INDEX
   CONCURRENTLY has reached the final state, you can't revalidate the index
   by reindexing it, you'll have to drop it and then make a brand new one.
   That seems like a pretty minor compromise.
   
   What I propose to do next is create a patch for HEAD that includes a
   new pg_index flag column, since I think the logic will be clearer
   with that.  Once we're happy with that, we can back-port the patch
   into a form that uses the existing flag columns.
   
   Anybody feel like bikeshedding the flag column name?  I'm thinking
   "indislive" but maybe there's a better name.
   
   Note: I'm not impressed by the proposal to replace these columns with
   a single integer flag column.  Aside from any possible incompatibility
   with existing client code, it just isn't going to be easy to read the
   index's state manually if we do that.  We could maybe dodge that
   complaint with a char (pseudo-enum) status column but I don't think
   that will simplify the code at all, and it's got the same or worse
   compatibility issues.
   
   > Btw, even if we manage to find a sensible fix for this I would suggest
   > postponing it after the next back branch release.
   
   AFAICS this is a data loss/corruption problem, and as such a "must fix".
   If we can't have it done by next week, I'd rather postpone the releases
   until it is done.
   
   			regards, tom lane
   
   
   

10. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Andres Freund <andres@2ndquadrant.com> — 2012-11-27T19:07:46Z

    On 2012-11-27 13:45:08 -0500, Tom Lane wrote:
    > Andres Freund <andres@2ndquadrant.com> writes:
    > > On 2012-11-26 22:44:49 -0500, Tom Lane wrote:
    > >> I looked through the code a bit, and I think we should be able to make
    > >> this work, but note there are also places that update indcheckxmin using
    > >> heap_update, and that's just as dangerous as changing the other two
    > >> flags via heap_update, if you don't have exclusive lock on the table.
    >
    > > Isn't setting indexcheckxmin already problematic in the case of CREATE
    > > INDEX CONCURRENTLY? index_build already runs in a separate transaction
    > > there.
    >
    > Yeah, you are right, except that AFAICS indcheckxmin is really only
    > needed for regular non-concurrent CREATE INDEX, which needs it because
    > it commits without waiting for readers that might be bothered by broken
    > HOT chains.  In a concurrent CREATE INDEX, we handle that problem by
    > waiting out all such readers before setting indisvalid.  So the
    > concurrent code path should not be bothering to set indcheckxmin at all,
    > I think.  (This is underdocumented.)
    
    Seems to be correct to me.
    
    > Looking closer at the comment in reindex_index, what it's really full of
    > angst about is that simple_heap_update will update the tuple's xmin
    > *when we would rather that it didn't*.  So switching to update-in-place
    > there will solve a problem, not create one.
    
    It strikes me that the whole idea of reusing xmin when indexcheckxmin is
    set is overly clever and storing the xid itself would have be been
    better... Too late though.
    
    > In short, all flag changes in pg_index should be done by
    > update-in-place, and the tuple's xmin will never change from the
    > original creating transaction (until frozen).
    
    Hm. That doesn't sound that easy to guarantee. Several ALTER TABLE and
    ALTER INDEX operations are expected to work transactionally right
    now. As far as I see there is nothing that prohibits a indexcheckxmin
    requiring index to be altered while indexcheckxmin is still required?
    
    > What we want the xmin to do, for indcheckxmin purposes, is reflect the
    > time at which the index started to be included in HOT-safety decisions.
    > Since we're never going to move the xmin, that means that *we cannot
    > allow REINDEX to mark a dead index live again*.  Once DROP INDEX
    > CONCURRENTLY has reached the final state, you can't revalidate the index
    > by reindexing it, you'll have to drop it and then make a brand new one.
    > That seems like a pretty minor compromise.
    
    That would be a regression compared with the current state though. We
    have officially documented REINDEX as a way out of INVALID indexes...
    
    If we store the xid of the reindexing transaction there that might be
    pessimal (because there should be not HOT safety problems) but should
    always be correct, or am I missing something?
    
    > What I propose to do next is create a patch for HEAD that includes a
    > new pg_index flag column, since I think the logic will be clearer
    > with that.  Once we're happy with that, we can back-port the patch
    > into a form that uses the existing flag columns.
    >
    > Anybody feel like bikeshedding the flag column name?  I'm thinking
    > "indislive" but maybe there's a better name.
    
    I personally would slightly favor indisdead instead...
    
    > > Btw, even if we manage to find a sensible fix for this I would suggest
    > > postponing it after the next back branch release.
    >
    > AFAICS this is a data loss/corruption problem, and as such a "must fix".
    > If we can't have it done by next week, I'd rather postpone the releases
    > until it is done.
    
    Ok, just seemed like a rather complex fix in a short time for something
    that seemingly hasn't been noticed since 8.3. I am a bit worried about
    introducing something worse while fixing this.
    
    Greetings,
    
    Andres Freund
    
    --
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

11. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-27T19:41:34Z

    Andres Freund <andres@2ndquadrant.com> writes:
    > On 2012-11-27 13:45:08 -0500, Tom Lane wrote:
    >> In short, all flag changes in pg_index should be done by
    >> update-in-place, and the tuple's xmin will never change from the
    >> original creating transaction (until frozen).
    
    > Hm. That doesn't sound that easy to guarantee. Several ALTER TABLE and
    > ALTER INDEX operations are expected to work transactionally right
    > now. As far as I see there is nothing that prohibits a indexcheckxmin
    > requiring index to be altered while indexcheckxmin is still required?
    
    I said "in pg_index".  There is no reason to ever alter an index's
    pg_index entry transactionally, because we don't support redefining
    the index columns.  The stuff you are allowed to ALTER is pretty much
    irrelevant to the index's life as an index.
    
    >> What we want the xmin to do, for indcheckxmin purposes, is reflect the
    >> time at which the index started to be included in HOT-safety decisions.
    >> Since we're never going to move the xmin, that means that *we cannot
    >> allow REINDEX to mark a dead index live again*.
    
    > That would be a regression compared with the current state though. We
    > have officially documented REINDEX as a way out of INVALID indexes...
    
    It's a way out of failed CREATE operations.  If DROP fails at the last
    step, you won't be able to go back, but why would you want to?  Just
    do the DROP again.
    
    >> Anybody feel like bikeshedding the flag column name?  I'm thinking
    >> "indislive" but maybe there's a better name.
    
    > I personally would slightly favor indisdead instead...
    
    Meh ... the other two flags are positive, in the sense of
    true-is-the-normal-state, so I thought this one should be too.
    
    I had also toyed with "indishot", to reflect the idea that this controls
    whether the index is included in HOT-safety decisions, but that seems
    maybe a little too cute.
    
    >>> Btw, even if we manage to find a sensible fix for this I would suggest
    >>> postponing it after the next back branch release.
    
    >> AFAICS this is a data loss/corruption problem, and as such a "must fix".
    >> If we can't have it done by next week, I'd rather postpone the releases
    >> until it is done.
    
    > Ok, just seemed like a rather complex fix in a short time for something
    > that seemingly hasn't been noticed since 8.3. I am a bit worried about
    > introducing something worse while fixing this.
    
    Hm?  The fact that the DROP patch broke CREATE INDEX CONCURRENTLY is a
    new and very nasty bug in 9.2.  I would agree with you if we were
    considering the unsafe-row-update problem alone, but it seems like we
    might as well fix both aspects while we're looking at this code.
    
    There is a question of whether we should risk trying to back-patch the
    in-place-update changes further than 9.2.  Given the lack of complaints
    I'm inclined not to, but could be persuaded differently.
    
    			regards, tom lane
    
    
    

12. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Andres Freund <andres@2ndquadrant.com> — 2012-11-27T19:50:41Z

    On 2012-11-27 14:41:34 -0500, Tom Lane wrote:
    > Andres Freund <andres@2ndquadrant.com> writes:
    > > On 2012-11-27 13:45:08 -0500, Tom Lane wrote:
    > >> In short, all flag changes in pg_index should be done by
    > >> update-in-place, and the tuple's xmin will never change from the
    > >> original creating transaction (until frozen).
    >
    > > Hm. That doesn't sound that easy to guarantee. Several ALTER TABLE and
    > > ALTER INDEX operations are expected to work transactionally right
    > > now. As far as I see there is nothing that prohibits a indexcheckxmin
    > > requiring index to be altered while indexcheckxmin is still required?
    >
    > I said "in pg_index".  There is no reason to ever alter an index's
    > pg_index entry transactionally, because we don't support redefining
    > the index columns.  The stuff you are allowed to ALTER is pretty much
    > irrelevant to the index's life as an index.
    
    Isn't inisprimary updated when an ALTER TABLE ... ADD PRIMARY KEY
    ... USING someindex ; is done? Also I think indoption might be written
    to as well.
    
    > >> What we want the xmin to do, for indcheckxmin purposes, is reflect the
    > >> time at which the index started to be included in HOT-safety decisions.
    > >> Since we're never going to move the xmin, that means that *we cannot
    > >> allow REINDEX to mark a dead index live again*.
    >
    > > That would be a regression compared with the current state though. We
    > > have officially documented REINDEX as a way out of INVALID indexes...
    >
    > It's a way out of failed CREATE operations.  If DROP fails at the last
    > step, you won't be able to go back, but why would you want to?  Just
    > do the DROP again.
    
    Oh, sorry, misunderstood you.
    
    >
    > >> Anybody feel like bikeshedding the flag column name?  I'm thinking
    > >> "indislive" but maybe there's a better name.
    >
    > > I personally would slightly favor indisdead instead...
    >
    > Meh ... the other two flags are positive, in the sense of
    > true-is-the-normal-state, so I thought this one should be too.
    
    Good point.
    
    > I had also toyed with "indishot", to reflect the idea that this controls
    > whether the index is included in HOT-safety decisions, but that seems
    > maybe a little too cute.
    
    indislive seems better than that, yes.
    
    > >>> Btw, even if we manage to find a sensible fix for this I would suggest
    > >>> postponing it after the next back branch release.
    >
    > >> AFAICS this is a data loss/corruption problem, and as such a "must fix".
    > >> If we can't have it done by next week, I'd rather postpone the releases
    > >> until it is done.
    >
    > > Ok, just seemed like a rather complex fix in a short time for something
    > > that seemingly hasn't been noticed since 8.3. I am a bit worried about
    > > introducing something worse while fixing this.
    >
    > Hm?  The fact that the DROP patch broke CREATE INDEX CONCURRENTLY is a
    > new and very nasty bug in 9.2.  I would agree with you if we were
    > considering the unsafe-row-update problem alone, but it seems like we
    > might as well fix both aspects while we're looking at this code.
    
    > There is a question of whether we should risk trying to back-patch the
    > in-place-update changes further than 9.2.  Given the lack of complaints
    > I'm inclined not to, but could be persuaded differently.
    
    Oh, I only was talking about the inplace changes. The DROP INDEX
    CONCURRENTLY breakage definitely needs to get backpatched.
    
    Greetings,
    
    Andres Freund
    
    --
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

13. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-27T21:31:15Z

    Andres Freund <andres@2ndquadrant.com> writes:
    > On 2012-11-27 14:41:34 -0500, Tom Lane wrote:
    >> The stuff you are allowed to ALTER is pretty much
    >> irrelevant to the index's life as an index.
    
    > Isn't inisprimary updated when an ALTER TABLE ... ADD PRIMARY KEY
    > ... USING someindex ; is done? Also I think indoption might be written
    > to as well.
    
    Ugh, you're right.  Somebody wasn't paying attention when those ALTER
    commands were added.
    
    We could probably alleviate the consequences of this by having those
    operations reset indcheckxmin if the tuple's old xmin is below the
    GlobalXmin horizon.  That's something for later though --- it's not
    a data corruption issue, it just means that the index might unexpectedly
    not be used for queries for a little bit after an ALTER.
    
    > It strikes me that the whole idea of reusing xmin when indexcheckxmin is
    > set is overly clever and storing the xid itself would have be been
    > better... Too late though.
    
    Well, the reason for that is documented in README.HOT: if the XID were
    in an ordinary column there'd be no nice way to get it frozen when it
    gets too old.  As-is, VACUUM takes care of that problem automatically.
    
    			regards, tom lane
    
    
    

14. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Andres Freund <andres@2ndquadrant.com> — 2012-11-27T22:13:25Z

    On 2012-11-27 16:31:15 -0500, Tom Lane wrote:
    > Andres Freund <andres@2ndquadrant.com> writes:
    > > On 2012-11-27 14:41:34 -0500, Tom Lane wrote:
    > >> The stuff you are allowed to ALTER is pretty much
    > >> irrelevant to the index's life as an index.
    >
    > > Isn't inisprimary updated when an ALTER TABLE ... ADD PRIMARY KEY
    > > ... USING someindex ; is done? Also I think indoption might be written
    > > to as well.
    >
    > Ugh, you're right.  Somebody wasn't paying attention when those ALTER
    > commands were added.
    >
    > We could probably alleviate the consequences of this by having those
    > operations reset indcheckxmin if the tuple's old xmin is below the
    > GlobalXmin horizon.  That's something for later though --- it's not
    > a data corruption issue, it just means that the index might unexpectedly
    > not be used for queries for a little bit after an ALTER.
    
    mark_index_clustered() does the same btw, its not a problem in the
    CLUSTER ... USING ...; case because that creates a new pg_index entry
    anyway but in the ALTER TABLE one thats not the case.
    
    Greetings,
    
    Andres Freund
    
    --
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

15. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-28T04:46:58Z

    Andres Freund <andres@2ndquadrant.com> writes:
    > On 2012-11-27 16:31:15 -0500, Tom Lane wrote:
    >> Andres Freund <andres@2ndquadrant.com> writes:
    >>> Isn't inisprimary updated when an ALTER TABLE ... ADD PRIMARY KEY
    >>> ... USING someindex ; is done? Also I think indoption might be written
    >>> to as well.
    
    >> Ugh, you're right.  Somebody wasn't paying attention when those ALTER
    >> commands were added.
    
    On closer look, indoption is never updated --- perhaps you were thinking
    about pg_class.reloptions.  indisprimary, indimmediate, and
    indisclustered are all subject to post-creation updates though.
    
    >> We could probably alleviate the consequences of this by having those
    >> operations reset indcheckxmin if the tuple's old xmin is below the
    >> GlobalXmin horizon.  That's something for later though --- it's not
    >> a data corruption issue, it just means that the index might unexpectedly
    >> not be used for queries for a little bit after an ALTER.
    
    > mark_index_clustered() does the same btw, its not a problem in the
    > CLUSTER ... USING ...; case because that creates a new pg_index entry
    > anyway but in the ALTER TABLE one thats not the case.
    
    After further study I think the situation is that
    
    (1) The indisvalid/indisready/indislive updates in CREATE/DROP INDEX
    CONCURRENTLY can, and must, be done in-place since we don't have
    exclusive lock on the parent table.
    
    (2) All the other updates can be transactional because we hold
    sufficient locks to ensure that nothing bad will happen.  The proposed
    reductions in ALTER TABLE lock strength would break this in several
    cases, but that's a problem for another day.
    
    Attached is a very preliminary draft patch for this.  I've not addressed
    the question of whether we can clear indcheckxmin during transactional
    updates of pg_index rows, but I think it covers everything else talked
    about in this thread.
    
    			regards, tom lane
    
    

16. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Andres Freund <andres@2ndquadrant.com> — 2012-11-28T12:59:06Z

    On 2012-11-27 23:46:58 -0500, Tom Lane wrote:
    > Andres Freund <andres@2ndquadrant.com> writes:
    > > On 2012-11-27 16:31:15 -0500, Tom Lane wrote:
    > >> Andres Freund <andres@2ndquadrant.com> writes:
    > >>> Isn't inisprimary updated when an ALTER TABLE ... ADD PRIMARY KEY
    > >>> ... USING someindex ; is done? Also I think indoption might be written
    > >>> to as well.
    >
    > >> Ugh, you're right.  Somebody wasn't paying attention when those ALTER
    > >> commands were added.
    >
    > On closer look, indoption is never updated --- perhaps you were thinking
    > about pg_class.reloptions.  indisprimary, indimmediate, and
    > indisclustered are all subject to post-creation updates though.
    
    Yea, I haven't really checked what inoption actually does.
    
    > >> We could probably alleviate the consequences of this by having those
    > >> operations reset indcheckxmin if the tuple's old xmin is below the
    > >> GlobalXmin horizon.  That's something for later though --- it's not
    > >> a data corruption issue, it just means that the index might unexpectedly
    > >> not be used for queries for a little bit after an ALTER.
    >
    > > mark_index_clustered() does the same btw, its not a problem in the
    > > CLUSTER ... USING ...; case because that creates a new pg_index entry
    > > anyway but in the ALTER TABLE one thats not the case.
    >
    > After further study I think the situation is that
    >
    > (1) The indisvalid/indisready/indislive updates in CREATE/DROP INDEX
    > CONCURRENTLY can, and must, be done in-place since we don't have
    > exclusive lock on the parent table.
    >
    > (2) All the other updates can be transactional because we hold
    > sufficient locks to ensure that nothing bad will happen.  The proposed
    > reductions in ALTER TABLE lock strength would break this in several
    > cases, but that's a problem for another day.
    
    
    > Attached is a very preliminary draft patch for this.  I've not addressed
    > the question of whether we can clear indcheckxmin during transactional
    > updates of pg_index rows, but I think it covers everything else talked
    > about in this thread.
    
    Looks good on a quick lookthrough. Will play a bit more once the
    indexcheckxmin stuff is sorted out.
    
    Some comments:
    - INDEX_DROP_CLEAR_READY clears indislive, perhasp INDEX_DROP_SET_DEAD
    or NOT_ALIVE is more appropriate?
    - I noticed while trying my old isolationtester test that
    heap_update_inplace disregards any locks on the tuple. I don't really
    see a scenario where this is problematic right now, seems a bit
    dangerous for the future though.
    
    Greetings,
    
    Andres Freund
    
    --
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

17. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-28T19:09:11Z

    Andres Freund <andres@2ndquadrant.com> writes:
    > On 2012-11-27 23:46:58 -0500, Tom Lane wrote:
    >> Attached is a very preliminary draft patch for this.  I've not addressed
    >> the question of whether we can clear indcheckxmin during transactional
    >> updates of pg_index rows, but I think it covers everything else talked
    >> about in this thread.
    
    > Looks good on a quick lookthrough. Will play a bit more once the
    > indexcheckxmin stuff is sorted out.
    
    > Some comments:
    > - INDEX_DROP_CLEAR_READY clears indislive, perhasp INDEX_DROP_SET_DEAD
    > or NOT_ALIVE is more appropriate?
    
    I changed it to SET_DEAD.  Also, on further reflection, I took your
    advice to use macros instead of direct tests of the flag columns where
    possible.
    
    > - I noticed while trying my old isolationtester test that
    > heap_update_inplace disregards any locks on the tuple. I don't really
    > see a scenario where this is problematic right now, seems a bit
    > dangerous for the future though.
    
    I think this should be all right --- we have at least
    ShareUpdateExclusiveLock on the table and the index before we do
    anything, so nobody else should be fooling with its pg_index entry.
    
    Attached is an updated patch for HEAD that I think is about ready to go.
    I'll start making a back-patchable version shortly.
    
    			regards, tom lane
    
    

18. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Andres Freund <andres@2ndquadrant.com> — 2012-11-28T20:00:04Z

    On 2012-11-28 14:09:11 -0500, Tom Lane wrote:
    > Andres Freund <andres@2ndquadrant.com> writes:
    > > On 2012-11-27 23:46:58 -0500, Tom Lane wrote:
    > >> Attached is a very preliminary draft patch for this.  I've not addressed
    > >> the question of whether we can clear indcheckxmin during transactional
    > >> updates of pg_index rows, but I think it covers everything else talked
    > >> about in this thread.
    
    > > - I noticed while trying my old isolationtester test that
    > > heap_update_inplace disregards any locks on the tuple. I don't really
    > > see a scenario where this is problematic right now, seems a bit
    > > dangerous for the future though.
    >
    > I think this should be all right --- we have at least
    > ShareUpdateExclusiveLock on the table and the index before we do
    > anything, so nobody else should be fooling with its pg_index entry.
    >
    > Attached is an updated patch for HEAD that I think is about ready to go.
    > I'll start making a back-patchable version shortly.
    
    Looks good!
    
    One minor thing I haven't noticed earlier: Perhaps we should also skip
    over invalid indexes in transformTableLikeClause's
    CREATE_TABLE_LIKE_INDEXES case?
    
    Greetings,
    
    Andres Freund
    
    --
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

19. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-28T22:42:18Z

    Andres Freund <andres@2ndquadrant.com> writes:
    > One minor thing I haven't noticed earlier: Perhaps we should also skip
    > over invalid indexes in transformTableLikeClause's
    > CREATE_TABLE_LIKE_INDEXES case?
    
    I left that as-is intentionally: the fact that an index isn't valid
    doesn't prevent us from cloning it.  A relevant data point is that
    pg_dump doesn't care whether indexes are valid or not --- it'll dump
    their definitions anyway.
    
    I agree it's a judgment call, though.  Anybody want to argue for the
    other position?
    
    			regards, tom lane
    
    
    

20. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Andres Freund <andres@anarazel.de> — 2012-11-28T22:53:11Z

    On 2012-11-28 17:42:18 -0500, Tom Lane wrote:
    > Andres Freund <andres@2ndquadrant.com> writes:
    > > One minor thing I haven't noticed earlier: Perhaps we should also skip
    > > over invalid indexes in transformTableLikeClause's
    > > CREATE_TABLE_LIKE_INDEXES case?
    >
    > I left that as-is intentionally: the fact that an index isn't valid
    > doesn't prevent us from cloning it.  A relevant data point is that
    > pg_dump doesn't care whether indexes are valid or not --- it'll dump
    > their definitions anyway.
    >
    > I agree it's a judgment call, though.  Anybody want to argue for the
    > other position?
    
    Hm. Seems odd to include indexes that are being dropped concurrently at
    that moment. But then, we can't really detect that situation and as you
    say its consistent with pg_dump...
    
    Hm.
    
    Greetings,
    
    Andres Freund
    
    
    

21. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-28T23:41:39Z

    Andres Freund <andres@anarazel.de> writes:
    > On 2012-11-28 17:42:18 -0500, Tom Lane wrote:
    >> I agree it's a judgment call, though.  Anybody want to argue for the
    >> other position?
    
    > Hm. Seems odd to include indexes that are being dropped concurrently at
    > that moment. But then, we can't really detect that situation and as you
    > say its consistent with pg_dump...
    
    [ thinks about that for a bit... ]  We could have that, for about the same
    cost as the currently proposed patch: instead of defining the added flag
    column as "index is live", define it as "drop in progress", and set it
    immediately at the start of the DROP CONCURRENTLY sequence.  Then the
    "dead" condition that RelationGetIndexList must check for is "drop in
    progress and not indisvalid and not indisready".
    
    However, this is more complicated and harder to understand.  So unless
    somebody is really excited about being able to tell the difference
    between create-in-progress and drop-in-progress, I'd rather leave the
    patch as-is.
    
    			regards, tom lane
    
    
    

22. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Andres Freund <andres@2ndquadrant.com> — 2012-11-28T23:52:29Z

    On 2012-11-28 18:41:39 -0500, Tom Lane wrote:
    > Andres Freund <andres@anarazel.de> writes:
    > > On 2012-11-28 17:42:18 -0500, Tom Lane wrote:
    > >> I agree it's a judgment call, though.  Anybody want to argue for the
    > >> other position?
    >
    > > Hm. Seems odd to include indexes that are being dropped concurrently at
    > > that moment. But then, we can't really detect that situation and as you
    > > say its consistent with pg_dump...
    >
    > [ thinks about that for a bit... ]  We could have that, for about the same
    > cost as the currently proposed patch: instead of defining the added flag
    > column as "index is live", define it as "drop in progress", and set it
    > immediately at the start of the DROP CONCURRENTLY sequence.  Then the
    > "dead" condition that RelationGetIndexList must check for is "drop in
    > progress and not indisvalid and not indisready".
    
    You're right.
    
    > However, this is more complicated and harder to understand.  So unless
    > somebody is really excited about being able to tell the difference
    > between create-in-progress and drop-in-progress, I'd rather leave the
    > patch as-is.
    
    The only real argument for doing this that I can see is a potential
    REINDEX CONCURRENTLY.
    
    Greetings,
    
    Andres Freund
    
    --
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

23. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Michael Paquier <michael.paquier@gmail.com> — 2012-11-29T00:10:22Z

    On Thu, Nov 29, 2012 at 8:52 AM, Andres Freund <andres@2ndquadrant.com>wrote:
    
    > On 2012-11-28 18:41:39 -0500, Tom Lane wrote:
    > > Andres Freund <andres@anarazel.de> writes:
    > > > On 2012-11-28 17:42:18 -0500, Tom Lane wrote:
    > > >> I agree it's a judgment call, though.  Anybody want to argue for the
    > > >> other position?
    > >
    > > > Hm. Seems odd to include indexes that are being dropped concurrently at
    > > > that moment. But then, we can't really detect that situation and as you
    > > > say its consistent with pg_dump...
    > >
    > > [ thinks about that for a bit... ]  We could have that, for about the
    > same
    > > cost as the currently proposed patch: instead of defining the added flag
    > > column as "index is live", define it as "drop in progress", and set it
    > > immediately at the start of the DROP CONCURRENTLY sequence.  Then the
    > > "dead" condition that RelationGetIndexList must check for is "drop in
    > > progress and not indisvalid and not indisready".
    >
    > You're right.
    >
    > > However, this is more complicated and harder to understand.  So unless
    > > somebody is really excited about being able to tell the difference
    > > between create-in-progress and drop-in-progress, I'd rather leave the
    > > patch as-is.
    >
    > The only real argument for doing this that I can see is a potential
    > REINDEX CONCURRENTLY.
    >
    Patch that has been submitted to this commit fest, and is going to need a
    lot of rework as well as more infrastructure like a better MVCC-ish
    SnapshotNow.
    -- 
    Michael Paquier
    http://michael.otacoo.com
    

24. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-29T00:11:46Z

    Andres Freund <andres@2ndquadrant.com> writes:
    > On 2012-11-28 18:41:39 -0500, Tom Lane wrote:
    >> However, this is more complicated and harder to understand.  So unless
    >> somebody is really excited about being able to tell the difference
    >> between create-in-progress and drop-in-progress, I'd rather leave the
    >> patch as-is.
    
    > The only real argument for doing this that I can see is a potential
    > REINDEX CONCURRENTLY.
    
    While I was working on this patch, I came to the conclusion that the
    only way REINDEX CONCURRENTLY could possibly work is:
    
    1. Do CREATE INDEX CONCURRENTLY with a temporary index name.
    
    2. Swap index names and any dependencies (eg for unique/pkey
       constraints), in a transaction of its own.
    
    3. Do DROP INDEX CONCURRENTLY on the now-obsolete index.
    
    If you try to do it with just one set of index catalog entries, you'll
    find the pg_class row has to be in two states at once, since there
    certainly have to be two underlying physical files while all this is
    going on.  That being the case, there'll be two different pg_index rows
    as well, and thus my worries upthread about whether REINDEX CONCURRENTLY
    would need to do something special with the pg_index row seem unfounded.
    
    Of course, there's still plenty of magic required to make this happen
    --- I don't see how to do step 2 safely without taking exclusive lock
    for at least a short interval.  But that's mostly about the SnapshotNow
    scan problem, which we at least have some ideas about how to solve.
    
    			regards, tom lane
    
    
    

25. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Andres Freund <andres@2ndquadrant.com> — 2012-11-29T00:15:18Z

    On 2012-11-29 09:10:22 +0900, Michael Paquier wrote:
    > On Thu, Nov 29, 2012 at 8:52 AM, Andres Freund <andres@2ndquadrant.com>wrote:
    > 
    > > On 2012-11-28 18:41:39 -0500, Tom Lane wrote:
    > > > Andres Freund <andres@anarazel.de> writes:
    > > > > On 2012-11-28 17:42:18 -0500, Tom Lane wrote:
    > > > >> I agree it's a judgment call, though.  Anybody want to argue for the
    > > > >> other position?
    > > >
    > > > > Hm. Seems odd to include indexes that are being dropped concurrently at
    > > > > that moment. But then, we can't really detect that situation and as you
    > > > > say its consistent with pg_dump...
    > > >
    > > > [ thinks about that for a bit... ]  We could have that, for about the
    > > same
    > > > cost as the currently proposed patch: instead of defining the added flag
    > > > column as "index is live", define it as "drop in progress", and set it
    > > > immediately at the start of the DROP CONCURRENTLY sequence.  Then the
    > > > "dead" condition that RelationGetIndexList must check for is "drop in
    > > > progress and not indisvalid and not indisready".
    > >
    > > You're right.
    > >
    > > > However, this is more complicated and harder to understand.  So unless
    > > > somebody is really excited about being able to tell the difference
    > > > between create-in-progress and drop-in-progress, I'd rather leave the
    > > > patch as-is.
    > >
    > > The only real argument for doing this that I can see is a potential
    > > REINDEX CONCURRENTLY.
    > >
    > Patch that has been submitted to this commit fest
    
    Yea, I did a first look through it recently... Not really sure where to
    start with the necessary infrastructure yet.
    
    > and is going to need a lot of rework as well as more infrastructure
    > like a better MVCC-ish SnapshotNow.
    
    Which is a major project in itself. I wonder whether my crazy "follow
    updates via t_ctid isn't the actually easier way to get there in the
    short term. On the other hand, a more MVCCish catalog access would be
    awesome.
    
    Greetings,
    
    Andres Freund
    
    -- 
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

26. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Andres Freund <andres@2ndquadrant.com> — 2012-11-29T00:23:25Z

    On 2012-11-28 19:11:46 -0500, Tom Lane wrote:
    > Andres Freund <andres@2ndquadrant.com> writes:
    > > On 2012-11-28 18:41:39 -0500, Tom Lane wrote:
    > >> However, this is more complicated and harder to understand.  So unless
    > >> somebody is really excited about being able to tell the difference
    > >> between create-in-progress and drop-in-progress, I'd rather leave the
    > >> patch as-is.
    >
    > > The only real argument for doing this that I can see is a potential
    > > REINDEX CONCURRENTLY.
    >
    > While I was working on this patch, I came to the conclusion that the
    > only way REINDEX CONCURRENTLY could possibly work is:
    >
    > 1. Do CREATE INDEX CONCURRENTLY with a temporary index name.
    >
    > 2. Swap index names and any dependencies (eg for unique/pkey
    >    constraints), in a transaction of its own.
    >
    > 3. Do DROP INDEX CONCURRENTLY on the now-obsolete index.
    >
    > If you try to do it with just one set of index catalog entries, you'll
    > find the pg_class row has to be in two states at once, since there
    > certainly have to be two underlying physical files while all this is
    > going on.  That being the case, there'll be two different pg_index rows
    > as well, and thus my worries upthread about whether REINDEX CONCURRENTLY
    > would need to do something special with the pg_index row seem unfounded.
    >
    > Of course, there's still plenty of magic required to make this happen
    > --- I don't see how to do step 2 safely without taking exclusive lock
    > for at least a short interval.  But that's mostly about the SnapshotNow
    > scan problem, which we at least have some ideas about how to solve.
    
    That's actually pretty similar to the way Michael has implemented it in
    his submitted patch and what has been discussed in a recent thread. His
    patch doesn't claim to solve the concurrency issues around 2) though...
    
    Greetings,
    
    Andres
    
    --
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

27. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-29T00:30:01Z

    Andres Freund <andres@2ndquadrant.com> writes:
    > On 2012-11-29 09:10:22 +0900, Michael Paquier wrote:
    >> and is going to need a lot of rework as well as more infrastructure
    >> like a better MVCC-ish SnapshotNow.
    
    > Which is a major project in itself. I wonder whether my crazy "follow
    > updates via t_ctid isn't the actually easier way to get there in the
    > short term. On the other hand, a more MVCCish catalog access would be
    > awesome.
    
    Yeah, eliminating the race conditions for SnapshotNow scans would be
    valuable enough to justify a lot of work --- we could get rid of a
    bunch of kluges once we had that, not to mention that Simon's project of
    reducing ALTER TABLE lock strength might stand a chance of working.
    
    			regards, tom lane
    
    
    

28. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Michael Paquier <michael.paquier@gmail.com> — 2012-11-29T01:31:20Z

    On 2012/11/29, at 9:23, Andres Freund <andres@2ndquadrant.com> wrote:
    
    > On 2012-11-28 19:11:46 -0500, Tom Lane wrote:
    >> Andres Freund <andres@2ndquadrant.com> writes:
    >>> On 2012-11-28 18:41:39 -0500, Tom Lane wrote:
    >>>> However, this is more complicated and harder to understand.  So unless
    >>>> somebody is really excited about being able to tell the difference
    >>>> between create-in-progress and drop-in-progress, I'd rather leave the
    >>>> patch as-is.
    >> 
    >>> The only real argument for doing this that I can see is a potential
    >>> REINDEX CONCURRENTLY.
    >> 
    >> While I was working on this patch, I came to the conclusion that the
    >> only way REINDEX CONCURRENTLY could possibly work is:
    >> 
    >> 1. Do CREATE INDEX CONCURRENTLY with a temporary index name.
    >> 
    >> 2. Swap index names and any dependencies (eg for unique/pkey
    >>   constraints), in a transaction of its own.
    >> 
    >> 3. Do DROP INDEX CONCURRENTLY on the now-obsolete index.
    >> 
    >> If you try to do it with just one set of index catalog entries, you'll
    >> find the pg_class row has to be in two states at once, since there
    >> certainly have to be two underlying physical files while all this is
    >> going on.  That being the case, there'll be two different pg_index rows
    >> as well, and thus my worries upthread about whether REINDEX CONCURRENTLY
    >> would need to do something special with the pg_index row seem unfounded.
    >> 
    >> Of course, there's still plenty of magic required to make this happen
    >> --- I don't see how to do step 2 safely without taking exclusive lock
    >> for at least a short interval.  But that's mostly about the SnapshotNow
    >> scan problem, which we at least have some ideas about how to solve.
    > 
    > That's actually pretty similar to the way Michael has implemented it in
    > his submitted patch and what has been discussed in a recent thread. His
    > patch doesn't claim to solve the concurrency issues around 2) though...
    Correct, that is the same approach.
    The patch took as approach to create a completely separate and new index entry which is a clone of the former index. This way all the entries are in catalogs are doubled, and the switch of the names is made while the two indexes are valid, but yes, I am myself wondering about the necessary lock that needs to be taken when switching the 2 index names. By the way, just by knowing that, I would agree to first rework the SnapshotNow mechanisms that would make a far better base for concurrent DDLs, and this is not limited to REINDEX only, but other things like CLUSTER, ALTER TABLE and perhaps others.
    
    Then once this is done PG will have better prospectives with features using CONCURRENTLY, and we could envisage a clean implementation for REINDEX CONCURRENTLY,
    
    Regards,
    
    Michael Paquier
    
    

29. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-29T04:40:27Z

    I wrote:
    > Attached is an updated patch for HEAD that I think is about ready to go.
    > I'll start making a back-patchable version shortly.
    
    Here is an only-lightly-tested version for 9.2.
    
    			regards, tom lane
    
    

30. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Pavan Deolasee <pavan.deolasee@gmail.com> — 2012-11-29T04:50:12Z

    On Thu, Nov 29, 2012 at 10:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
    
    > I wrote:
    > > Attached is an updated patch for HEAD that I think is about ready to go.
    > > I'll start making a back-patchable version shortly.
    >
    > Here is an only-lightly-tested version for 9.2.
    >
    >
    Looks good at a glance. I wonder though if it would have been better to
    have IndexSetValid/IndexClearValid family of macros instead of the
    index_set_state_flags kind of a machinery, purely from consistency
    perspective. I understand that index_set_state_flags also takes care of
    opening the catalogue etc, but there are only two callers to the function
    and we could do that outside that.
    
    May be too late since we already have the patch committed to HEAD.
    
    Thanks,
    Pavan
    

31. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Amit Kapila <amit.kapila@huawei.com> — 2012-11-29T10:48:07Z

    On Thursday, November 29, 2012 12:39 AM Tom Lane wrote.
    > Andres Freund <andres@2ndquadrant.com> writes:
    > > On 2012-11-27 23:46:58 -0500, Tom Lane wrote:
    > >> Attached is a very preliminary draft patch for this.  I've not
    > >> addressed the question of whether we can clear indcheckxmin during
    > >> transactional updates of pg_index rows, but I think it covers
    > >> everything else talked about in this thread.
    > 
     
    > Attached is an updated patch for HEAD that I think is about ready to go.
    > I'll start making a back-patchable version shortly.
    
    I had verified in the Patch committed that the problem is resolved. 
    
    I have a doubt related to RelationGetIndexList() function.
    
    In while loop, if index is not live then it continues, so it can be possible
    that we don't find a valid index after this while loop.
    But still after the loop, it marks relation->rd_indexvalid = 1. I am not
    able to see any problem with it, but why to mark it as valid when actually
    there is no valid index.
    
    With Regards,
    Amit Kapila.
    
    
    
    

32. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Andres Freund <andres@2ndquadrant.com> — 2012-11-29T10:54:05Z

    On 2012-11-29 16:18:07 +0530, Amit Kapila wrote:
    > On Thursday, November 29, 2012 12:39 AM Tom Lane wrote.
    > > Andres Freund <andres@2ndquadrant.com> writes:
    > > > On 2012-11-27 23:46:58 -0500, Tom Lane wrote:
    > > >> Attached is a very preliminary draft patch for this.  I've not
    > > >> addressed the question of whether we can clear indcheckxmin during
    > > >> transactional updates of pg_index rows, but I think it covers
    > > >> everything else talked about in this thread.
    > >
    >
    > > Attached is an updated patch for HEAD that I think is about ready to go.
    > > I'll start making a back-patchable version shortly.
    >
    > I had verified in the Patch committed that the problem is resolved.
    >
    > I have a doubt related to RelationGetIndexList() function.
    >
    > In while loop, if index is not live then it continues, so it can be possible
    > that we don't find a valid index after this while loop.
    > But still after the loop, it marks relation->rd_indexvalid = 1. I am not
    > able to see any problem with it, but why to mark it as valid when actually
    > there is no valid index.
    
    rd_indexvalid is just saying whether rd_indexlist is valid. A zero
    element list seems to be valid to me. If we don't set rd_indexvalid
    pg_index will constantly be rescanned because the result isn't
    considered cached anymore.
    
    Greetings,
    
    Andres Freund
    
    --
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

33. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Amit Kapila <amit.kapila@huawei.com> — 2012-11-29T11:45:35Z

    On Thursday, November 29, 2012 4:24 PM Andres Freund wrote:
    > On 2012-11-29 16:18:07 +0530, Amit Kapila wrote:
    > > On Thursday, November 29, 2012 12:39 AM Tom Lane wrote.
    > > > Andres Freund <andres@2ndquadrant.com> writes:
    > > > > On 2012-11-27 23:46:58 -0500, Tom Lane wrote:
    > > > >> Attached is a very preliminary draft patch for this.  I've not
    > > > >> addressed the question of whether we can clear indcheckxmin
    > during
    > > > >> transactional updates of pg_index rows, but I think it covers
    > > > >> everything else talked about in this thread.
    > > >
    > >
    > > > Attached is an updated patch for HEAD that I think is about ready to
    > go.
    > > > I'll start making a back-patchable version shortly.
    > >
    > > I had verified in the Patch committed that the problem is resolved.
    > >
    > > I have a doubt related to RelationGetIndexList() function.
    > >
    > > In while loop, if index is not live then it continues, so it can be
    > possible
    > > that we don't find a valid index after this while loop.
    > > But still after the loop, it marks relation->rd_indexvalid = 1. I am
    > not
    > > able to see any problem with it, but why to mark it as valid when
    > actually
    > > there is no valid index.
    > 
    > rd_indexvalid is just saying whether rd_indexlist is valid. A zero
    > element list seems to be valid to me. If we don't set rd_indexvalid
    > pg_index will constantly be rescanned because the result isn't
    > considered cached anymore.
    
    Got the point.
    Thanks.
    
    With Regards,
    Amit Kapila.
    
    
    
    

34. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-29T16:53:50Z

    And here is a version for 9.1.  This omits the code changes directly
    relevant to DROP INDEX CONCURRENTLY, but includes the changes to avoid
    transactional updates of the pg_index row during CREATE CONCURRENTLY,
    as well as the changes to prevent use of not-valid or not-ready indexes
    in places where it matters.  I also chose to keep on using the
    IndexIsValid and IndexIsReady macros, so as to avoid unnecessary
    divergences of the branches.
    
    I think this much of the patch needs to go into all supported branches.
    
    			regards, tom lane
    
    

35. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Andres Freund <andres@2ndquadrant.com> — 2012-11-29T19:43:59Z

    On 2012-11-29 11:53:50 -0500, Tom Lane wrote:
    > And here is a version for 9.1.  This omits the code changes directly
    > relevant to DROP INDEX CONCURRENTLY, but includes the changes to avoid
    > transactional updates of the pg_index row during CREATE CONCURRENTLY,
    > as well as the changes to prevent use of not-valid or not-ready indexes
    > in places where it matters.  I also chose to keep on using the
    > IndexIsValid and IndexIsReady macros, so as to avoid unnecessary
    > divergences of the branches.
    
    Looks good me.
    
    > I think this much of the patch needs to go into all supported branches.
    
    Looks like that to me, yes.
    
    Thanks for all that work!
    
    Andres
    
    --
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

36. Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

    Michael Paquier <michael.paquier@gmail.com> — 2012-11-30T00:56:34Z

    On Fri, Nov 30, 2012 at 4:43 AM, Andres Freund <andres@2ndquadrant.com>wrote:
    
    > On 2012-11-29 11:53:50 -0500, Tom Lane wrote:
    > > And here is a version for 9.1.  This omits the code changes directly
    > > relevant to DROP INDEX CONCURRENTLY, but includes the changes to avoid
    > > transactional updates of the pg_index row during CREATE CONCURRENTLY,
    > > as well as the changes to prevent use of not-valid or not-ready indexes
    > > in places where it matters.  I also chose to keep on using the
    > > IndexIsValid and IndexIsReady macros, so as to avoid unnecessary
    > > divergences of the branches.
    >
    > Looks good me.
    >
    > > I think this much of the patch needs to go into all supported branches.
    >
    > Looks like that to me, yes.
    >
    > Thanks for all that work!
    >
    Thanks. Just by looking at the patch it will be necessary to realign the
    patch of REINDEX CONCURRENTLY.
    However, as the discussion regarding the lock taken at phase 2 (index
    swapping) is still not done, I am not sure if it is worth to do that yet.
    Andres, please let me know in case you want a better version for your
    review.
    -- 
    Michael Paquier
    http://michael.otacoo.com