Thread

1. BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

   Casey & Gina <cg@osss.net> — 2014-10-10T18:14:33Z

   The following bug has been logged on the website:
   
   Bug reference:      11638
   Logged by:          Casey Allen Shobe
   Email address:      cg@osss.net
   PostgreSQL version: 9.3.5
   Operating system:   Linux (RHEL 5)
   Description:        
   
   Discovered on 8.4.5, reproducible on 9.3.5.
   
   The paste pretty much explains it all:
   http://pgsql.privatepaste.com/162682d176
   
   When, within a transaction:
   * I drop a foreign key (or any) constraint.
   * Load some data to the table which won't fit the constraint.
   * Analyze the table.
   * Attempt to re-add the constraint which fails and should roll back the
   whole transaction.
   
   The constraint is still missing after rollback.
   
   If I take out the analyze step, it works as expected.
   
   
   

2. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

   Andres Freund <andres@2ndquadrant.com> — 2014-10-10T19:14:01Z

   Hi,
   
   On 2014-10-10 18:14:33 +0000, cg@osss.net wrote:
   > The following bug has been logged on the website:
   > 
   > Bug reference:      11638
   > Logged by:          Casey Allen Shobe
   > Email address:      cg@osss.net
   > PostgreSQL version: 9.3.5
   > Operating system:   Linux (RHEL 5)
   > Description:        
   > 
   > Discovered on 8.4.5, reproducible on 9.3.5.
   > 
   > The paste pretty much explains it all:
   > http://pgsql.privatepaste.com/162682d176
   
   Please attach - the paste will expire in a couple days. That makes
   researching issues later quite annoying.
   
   > When, within a transaction:
   > * I drop a foreign key (or any) constraint.
   > * Load some data to the table which won't fit the constraint.
   > * Analyze the table.
   > * Attempt to re-add the constraint which fails and should roll back the
   > whole transaction.
   > 
   > The constraint is still missing after rollback.
   > 
   > If I take out the analyze step, it works as expected.
   
   Yuck. This is ugly. The problem is this nice bit:
   
   void
   vac_update_relstats(Relation relation,
   					BlockNumber num_pages, double num_tuples,
   					BlockNumber num_all_visible_pages,
   					bool hasindex, TransactionId frozenxid,
   {					MultiXactId minmulti)
   ...
   	if (pgcform->relhastriggers && relation->trigdesc == NULL)
   	{
   		pgcform->relhastriggers = false;
   		dirty = true;
   	}
   ...
   	/* If anything changed, write out the tuple. */
   	if (dirty)
   		heap_inplace_update(rd, ctup);
   }
   
   That's, like, a *seriously* bad idea. The current xact doesn't see a
   trigger, so we remove relhastriggers (and similarly other relhas*
   stuff). The kicker is that we do so *nontransactionally*.
   
   That's fine enough for VACUUM because that doesn't run in a
   transaction. But vac_update_relstats is also run for ANALYZE.
   
   I've no time to think this through right now, but my current thinking is
   that we should use a transactional update for ANALYZE.
   
   Greetings,
   
   Andres Freund
   
   -- 
    Andres Freund	                   http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

3. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

   Casey & Gina <cg@osss.net> — 2014-10-10T19:22:57Z

   On Oct 10, 2014, at 3:14 PM, Andres Freund <andres@2ndquadrant.com> wrote:
   > 
   > Please attach - the paste will expire in a couple days. That makes
   > researching issues later quite annoying.
   
   Well I set it to a month, but in any case, scripts are attached.
   
   I have updated this to include two different tests - in the first I expanded the test to show that primary keys and indexes are also lost.  In the second I show that by adding their re-create statements before the re-create for the foreign key, they somehow are there after the transaction rollback.
   
   For the first test case, run the following with the attached scripts:
   psql -f create.sql
   psql -f test.sql
   
   Output:
   
          Table "test.table2"
     Column   |  Type   | Modifiers 
   -----------+---------+-----------
    table1_id | integer | not null
    value     | text    | not null
   Indexes:
       "table2_pkey" PRIMARY KEY, btree (table1_id)
       "testindex" btree (value)
   Foreign-key constraints:
       "table2_table1_id_fkey" FOREIGN KEY (table1_id) REFERENCES test.table1(id)
   
   BEGIN
   Time: 0.155 ms
   ALTER TABLE
   Time: 1.194 ms
   ALTER TABLE
   Time: 0.368 ms
   DROP INDEX
   Time: 0.294 ms
   Time: 0.347 ms
   ANALYZE
   Time: 0.427 ms
   psql:test.sql:17: ERROR:  insert or update on table "table2" violates foreign key constraint "table2_table1_id_fkey"
   DETAIL:  Key (table1_id)=(1) is not present in table "table1".
   psql:test.sql:19: ERROR:  current transaction is aborted, commands ignored until end of transaction block
   psql:test.sql:21: ERROR:  current transaction is aborted, commands ignored until end of transaction block
   ROLLBACK
   Time: 0.088 ms
          Table "test.table2"
     Column   |  Type   | Modifiers 
   -----------+---------+-----------
    table1_id | integer | not null
    value     | text    | not null
   
   
   For the second test case, run the following:
   psql -f create.sql
   psql -f test2.sql
   
   Output:
   
          Table "test.table2"
     Column   |  Type   | Modifiers 
   -----------+---------+-----------
    table1_id | integer | not null
    value     | text    | not null
   Indexes:
       "table2_pkey" PRIMARY KEY, btree (table1_id)
       "testindex" btree (value)
   Foreign-key constraints:
       "table2_table1_id_fkey" FOREIGN KEY (table1_id) REFERENCES test.table1(id)
   
   BEGIN
   Time: 0.255 ms
   ALTER TABLE
   Time: 1.164 ms
   ALTER TABLE
   Time: 0.329 ms
   DROP INDEX
   Time: 0.378 ms
   Time: 0.296 ms
   ANALYZE
   Time: 0.533 ms
   ALTER TABLE
   Time: 11.655 ms
   CREATE INDEX
   Time: 5.871 ms
   psql:test2.sql:21: ERROR:  insert or update on table "table2" violates foreign key constraint "table2_table1_id_fkey"
   DETAIL:  Key (table1_id)=(1) is not present in table "table1".
   ROLLBACK
   Time: 0.150 ms
          Table "test.table2"
     Column   |  Type   | Modifiers 
   -----------+---------+-----------
    table1_id | integer | not null
    value     | text    | not null
   Indexes:
       "table2_pkey" PRIMARY KEY, btree (table1_id)
       "testindex" btree (value)
   
   As a workaround to this bug, does it make sense to delay all analyzes until the end of the transaction?
   
   Thanks,
   -- 
   Casey Allen Shobe
   
   

4. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

   Michael Paquier <michael.paquier@gmail.com> — 2014-10-15T04:15:51Z

   On Sat, Oct 11, 2014 at 4:14 AM, Andres Freund <andres@2ndquadrant.com>
   wrote:
   >> When, within a transaction:
   >> * I drop a foreign key (or any) constraint.
   >> * Load some data to the table which won't fit the constraint.
   >> * Analyze the table.
   >> * Attempt to re-add the constraint which fails and should roll back the
   >> whole transaction.
   >>
   >> The constraint is still missing after rollback.
   > That's, like, a *seriously* bad idea. The current xact doesn't see a
   > trigger, so we remove relhastriggers (and similarly other relhas*
   > stuff). The kicker is that we do so *nontransactionally*.
   >
   > That's fine enough for VACUUM because that doesn't run in a
   > transaction. But vac_update_relstats is also run for ANALYZE.
   >
   > I've no time to think this through right now, but my current thinking is
   > that we should use a transactional update for ANALYZE.
   
   The comments on top of vac_update_relstats rely on heap_inplace_update:
    *              Note another assumption: that two VACUUMs/ANALYZEs on a
   table can't
    *              run in parallel, nor can VACUUM/ANALYZE run in parallel
   with a
    *              schema alteration such as adding an index, rule, or
   trigger.  Otherwise
    *              our updates of relhasindex etc might overwrite uncommitted
   updates.
   I am not sure what would be the side effects of such a change, but it seems
   dangerous to add any control mechanism able to choose if ctup is updated
   with either heap_inplace_update or simple_heap_update, especially something
   like (GetTopTransactionIdIfAny() == InvalidTransactionId) to determine if
   this code path is taken in an xact that has already done a transactional
   update. Perhaps a solution would be to document properly that analyze
   should not be run in the same transaction as schema changes.
   Regards,
   -- 
   Michael
   

5. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

   Michael Paquier <michael.paquier@gmail.com> — 2014-10-15T04:17:13Z

   On Sat, Oct 11, 2014 at 4:22 AM, Casey & Gina <cg@osss.net> wrote:
   
   > On Oct 10, 2014, at 3:14 PM, Andres Freund <andres@2ndquadrant.com> wrote:
   > >
   > > Please attach - the paste will expire in a couple days. That makes
   > > researching issues later quite annoying.
   >
   > Well I set it to a month, but in any case, scripts are attached.
   >
   Actually they were not attached. Here they are.
   -- 
   Michael
   

6. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

   Andres Freund <andres@2ndquadrant.com> — 2014-10-15T05:12:54Z

   On 2014-10-15 13:15:51 +0900, Michael Paquier wrote:
   > On Sat, Oct 11, 2014 at 4:14 AM, Andres Freund <andres@2ndquadrant.com>
   > wrote:
   > >> When, within a transaction:
   > >> * I drop a foreign key (or any) constraint.
   > >> * Load some data to the table which won't fit the constraint.
   > >> * Analyze the table.
   > >> * Attempt to re-add the constraint which fails and should roll back the
   > >> whole transaction.
   > >>
   > >> The constraint is still missing after rollback.
   > > That's, like, a *seriously* bad idea. The current xact doesn't see a
   > > trigger, so we remove relhastriggers (and similarly other relhas*
   > > stuff). The kicker is that we do so *nontransactionally*.
   > >
   > > That's fine enough for VACUUM because that doesn't run in a
   > > transaction. But vac_update_relstats is also run for ANALYZE.
   > >
   > > I've no time to think this through right now, but my current thinking is
   > > that we should use a transactional update for ANALYZE.
   > 
   > The comments on top of vac_update_relstats rely on heap_inplace_update:
   >  *              Note another assumption: that two VACUUMs/ANALYZEs on a
   > table can't
   >  *              run in parallel, nor can VACUUM/ANALYZE run in parallel
   > with a
   >  *              schema alteration such as adding an index, rule, or
   > trigger.  Otherwise
   >  *              our updates of relhasindex etc might overwrite uncommitted
   > updates.
   > I am not sure what would be the side effects of such a change, but it seems
   > dangerous to add any control mechanism able to choose if ctup is updated
   > with either heap_inplace_update or simple_heap_update, especially something
   > like (GetTopTransactionIdIfAny() == InvalidTransactionId) to determine if
   > this code path is taken in an xact that has already done a transactional
   > update.
   
   I'm not sure which danger you're seeing here. Imo we need to choose
   between heap_inplace/heap_update for VACUUM/ANALYZE because one is
   allowed to run in a transaction, and the other is not. It simply *can't*
   be safe for ANALYZE to set things like relhastriggers = false using
   heap_inplace().
   There's problems with both it rolling back and thus undoing the action
   that allowed relhastriggers = false to be set and scenarios where it's
   not ok that other backends can see that value before the transaction
   committed.
   
   > Perhaps a solution would be to document properly that analyze
   > should not be run in the same transaction as schema changes.
   
   Surely corrupting the database (yes, this is what's happening here),
   isn't something that can just be documented away.
   
   Greetings,
   
   Andres Freund
   
   -- 
    Andres Freund	                   http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

7. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

   Michael Paquier <michael.paquier@gmail.com> — 2014-10-15T06:02:43Z

   On Wed, Oct 15, 2014 at 2:12 PM, Andres Freund <andres@2ndquadrant.com>
   wrote:
   
   > I'm not sure which danger you're seeing here. Imo we need to choose
   > between heap_inplace/heap_update for VACUUM/ANALYZE because one is
   > allowed to run in a transaction, and the other is not. It simply *can't*
   > be safe for ANALYZE to set things like relhastriggers = false using
   > heap_inplace().
   > There's problems with both it rolling back and thus undoing the action
   > that allowed relhastriggers = false to be set and scenarios where it's
   > not ok that other backends can see that value before the transaction
   > committed.
   
   Hm, I was wondering about the potential effects of VACUUM FULL or VACUUM
   ANALYZE, but as they cannot run in a tx block... Btw, I have just put my
   hands on this code and made the attached to make vac_update_relstats able
   to do a transactional update. It looks to work fine with only a check on
   the flags of vacuum statement.
   Regards,
   -- 
   Michael
   

8. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

   Andres Freund <andres@2ndquadrant.com> — 2014-10-15T06:18:15Z

   On 2014-10-15 15:02:43 +0900, Michael Paquier wrote:
   > On Wed, Oct 15, 2014 at 2:12 PM, Andres Freund <andres@2ndquadrant.com>
   > wrote:
   > 
   > > I'm not sure which danger you're seeing here. Imo we need to choose
   > > between heap_inplace/heap_update for VACUUM/ANALYZE because one is
   > > allowed to run in a transaction, and the other is not. It simply *can't*
   > > be safe for ANALYZE to set things like relhastriggers = false using
   > > heap_inplace().
   > > There's problems with both it rolling back and thus undoing the action
   > > that allowed relhastriggers = false to be set and scenarios where it's
   > > not ok that other backends can see that value before the transaction
   > > committed.
   > 
   > Hm, I was wondering about the potential effects of VACUUM FULL or VACUUM
   > ANALYZE, but as they cannot run in a tx block...
   
   Also they all take ShareUpdateExclusive locks preventing them from
   running concurrently.
   
   > Btw, I have just put my hands on this code and made the attached to
   > make vac_update_relstats able to do a transactional update. It looks
   > to work fine with only a check on the flags of vacuum statement.
   
   Have you tested that the problem's now fixed?
   
   Imo this is complex enough to deserve a regression test. Can you add
   one?
   
   Greetings,
   
   Andres Freund
   
   -- 
    Andres Freund	                   http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

9. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

   Michael Paquier <michael.paquier@gmail.com> — 2014-10-15T08:31:54Z

   On Wed, Oct 15, 2014 at 3:18 PM, Andres Freund <andres@2ndquadrant.com>
   wrote:
   
   > On 2014-10-15 15:02:43 +0900, Michael Paquier wrote:
   > > Btw, I have just put my hands on this code and made the attached to
   > > make vac_update_relstats able to do a transactional update. It looks
   > > to work fine with only a check on the flags of vacuum statement.
   >
   > Have you tested that the problem's now fixed?
   >
   Yep. In the test case given by Casey the foreign key on the second table is
   visible after the rollback.
   
   
   > Imo this is complex enough to deserve a regression test. Can you add
   > one?
   >
   Definitely makes sense. Here is an updated version.
   Regards,
   -- 
   Michael
   

10. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

    Casey & Gina <cg@osss.net> — 2014-10-15T13:18:02Z

    Sorry, here are the files I was actually referencing in that mail.
    
    

11. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

    Casey & Gina <cg@osss.net> — 2014-10-15T13:29:01Z

    On Oct 15, 2014, at 12:15 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
    > 
    > Perhaps a solution would be to document properly that analyze should not be run in the same transaction as schema changes.
    
    If at all possible, we really need to be able to run it transactionally.  The use case where we encountered this is when we deploy upgrades.  We develop numerous changes for the next version of some of our software products (which share a single database), and our deploy process consolidates the database changes required for each change into a single transaction.  If this transaction fails for any reason, the database needs to go back to the state it was originally with no changes whatsoever, and it should not complete until everything including the analyzes are done.  One common type of change is to reload the data of a table by truncating it and then COPYing in the new data.  In the past analyzes did not happen upon table data reloads, and this would result in terrible application behavior after deploys for several hours until auto-analyze would finally catch up.  Adding analyze after each data reload within the deploy process is what eliminated this problem and kept us from having unpredictable performance.
    
    In theory we could make everything but the analyzes run in a transaction and then execute the analyzes afterwards, and not consider the deploy done until they finish, but there is value in having every deploy being exactly one all-or-nothing transaction.
    
    Best wishes,
    -- 
    Casey Allen Shobe
    
    

12. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

    Tom Lane <tgl@sss.pgh.pa.us> — 2014-10-22T02:12:29Z

    Michael Paquier <michael.paquier@gmail.com> writes:
    > On Wed, Oct 15, 2014 at 3:18 PM, Andres Freund <andres@2ndquadrant.com>
    >> On 2014-10-15 15:02:43 +0900, Michael Paquier wrote:
    >>> Btw, I have just put my hands on this code and made the attached to
    >>> make vac_update_relstats able to do a transactional update. It looks
    >>> to work fine with only a check on the flags of vacuum statement.
    
    I think the original reasoning why this would be okay even for ANALYZE
    was that if we are doing an ANALYZE inside a transaction that has done
    DDL on the table, it would be okay to modify the pg_class row in-place
    because it would be a tuple that the transaction itself has written
    and so it would be invalidated if the transaction rolled back.  The flaw
    in this reasoning is that the DDL might not actually have changed the
    pg_class row, and so it might still be the original pre-transaction
    version that's visible to other transactions.  Thus the risk is confined
    to cases like DROP INDEX that don't modify pg_class.  I'm not entirely
    sure, but this reasoning might have been perfectly correct at the time
    and have been invalidated by later optimizations to suppress "unnecessary"
    physical updates of pg_class in favor of just sending sinval events.
    (Note that DROP INDEX certainly must send a relcache inval to ensure that
    other sessions stop using the index, and at one time the only trigger for
    such an inval was to physically update pg_class and/or pg_attribute.)
    
    >> Imo this is complex enough to deserve a regression test. Can you add
    >> one?
    
    > Definitely makes sense. Here is an updated version.
    
    I don't much care for this patch.  Aside from cosmetic issues like having
    named the new argument backwards and failed to update the function header
    comment that the patch largely invalidates, it seems to me to be likely
    to have unforeseen side effects, in that there may now be assumptions
    elsewhere that we don't force a pg_class update for this type of change.
    The implications are particularly ticklish for pg_class itself...
    
    I think that a better answer is to continue to do this update
    nontransactionally, but to not let the code clear relhasindex etc
    if we're inside a transaction block.  It is certainly safe to put
    off clearing those flags if we're not sure that we're seeing a
    committed state of the table's schema.
    
    An interesting question is whether it is ever possible for this function
    to be told to *set* relhasindex when it was clear (or likewise for the
    other flags).  Offhand I would say that that should never happen, because
    certainly neither VACUUM nor ANALYZE should be creating indexes etc.
    Should we make it throw an error if that happens, or just go ahead and
    apply the update, assuming that it's correcting somehow-corrupted data?
    
    			regards, tom lane
    
    
    

13. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

    Andres Freund <andres@2ndquadrant.com> — 2014-10-22T13:21:44Z

    On 2014-10-21 22:12:29 -0400, Tom Lane wrote:
    > I don't much care for this patch.  Aside from cosmetic issues like having
    > named the new argument backwards and failed to update the function header
    > comment that the patch largely invalidates, it seems to me to be likely
    > to have unforeseen side effects, in that there may now be assumptions
    > elsewhere that we don't force a pg_class update for this type of change.
    > The implications are particularly ticklish for pg_class itself...
    
    I'm unconvinced that that's a problem. We already do transactional
    updates of pg_class for stuff like CREATE INDEX - while the relation is
    *not* locked exclusively.  And e.g. CLUSTER pg_class does transactional
    updates of pg_class itself, without problems afaics. The latter is under
    an exclusive lock admittedly.
    
    What's the problem you're suspecting?
    
    Even if it doesn't arrise to the level of data corruption, I suspect in
    many cases updating the stats nontransactionally in an later aborted
    transaction will surprise some users. The normal reason for doing a
    ANALYZE in a transaction is that you changed the data dramatically.
    
    > I think that a better answer is to continue to do this update
    > nontransactionally, but to not let the code clear relhasindex etc
    > if we're inside a transaction block.  It is certainly safe to put
    > off clearing those flags if we're not sure that we're seeing a
    > committed state of the table's schema.
    
    That'd fix the corruption, so it'd certainly be an improvement. But I'm
    unconvinced that it's the best way forward.
    
    > An interesting question is whether it is ever possible for this function
    > to be told to *set* relhasindex when it was clear (or likewise for the
    > other flags).  Offhand I would say that that should never happen, because
    > certainly neither VACUUM nor ANALYZE should be creating indexes etc.
    > Should we make it throw an error if that happens, or just go ahead and
    > apply the update, assuming that it's correcting somehow-corrupted data?
    
    It probably should at least LOG/WARN, to make it clear that things were
    in a bad shape. It might be helpful to allow VACUUM/ANALYZE to fix
    existing corrupted relhas* flags. Although there could already be
    existing corruption, in which case users might want to get alerted of
    that more aggressively.
    
    Greetings,
    
    Andres Freund
    
    -- 
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

14. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

    Casey & Gina <cg@osss.net> — 2014-10-22T13:38:59Z

    On Oct 22, 2014, at 9:21 AM, Andres Freund <andres@2ndquadrant.com> wrote:
    > Even if it doesn't arrise to the level of data corruption, I suspect in
    > many cases updating the stats nontransactionally in an later aborted
    > transaction will surprise some users. The normal reason for doing a
    > ANALYZE in a transaction is that you changed the data dramatically.
    
    I agree.  If stats were updated after a transaction were rolled back, this would be bad.  If the purpose of the ANALYZE is that otherwise queries are slow after a reload, and the transaction fails and we go back to old data, the expectation should be that old performance and output resumes, not bad performance with old output due to corrupted stats.
    
    > It probably should at least LOG/WARN, to make it clear that things were
    > in a bad shape. It might be helpful to allow VACUUM/ANALYZE to fix
    > existing corrupted relhas* flags. Although there could already be
    > existing corruption, in which case users might want to get alerted of
    > that more aggressively.
    
    As a user, I would prefer to see an error - assuming there were a way to deal with it manually in separate steps somehow - if there were pre-existing corruption that needed to be dealt with.  A warning could be sufficient, but I would definitely not want it to be silent, because I think that would potentially hide corruption issues...a problem could exist undetected for longer with something else silently cleaning up after it.
    
    Best wishes,
    -- 
    Casey
    
    

15. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

    Tom Lane <tgl@sss.pgh.pa.us> — 2014-10-22T14:35:24Z

    Andres Freund <andres@2ndquadrant.com> writes:
    > On 2014-10-21 22:12:29 -0400, Tom Lane wrote:
    >> I don't much care for this patch.  Aside from cosmetic issues like having
    >> named the new argument backwards and failed to update the function header
    >> comment that the patch largely invalidates, it seems to me to be likely
    >> to have unforeseen side effects, in that there may now be assumptions
    >> elsewhere that we don't force a pg_class update for this type of change.
    
    > I'm unconvinced that that's a problem.
    
    [ shrug... ]  The fact that you haven't thought of a problem doesn't mean
    there is not one.  We hadn't thought of the current problem either.
    
    > What's the problem you're suspecting?
    
    If I could put my finger on something, I'd have pointed it out rather
    than just handwaving :-(.  But the (theorized) sequence of decisions
    that got us into this mess should convince you that switching between
    transactional and nontransactional updates is not something to be
    done lightly.
    
    > Even if it doesn't arrise to the level of data corruption, I suspect in
    > many cases updating the stats nontransactionally in an later aborted
    > transaction will surprise some users. The normal reason for doing a
    > ANALYZE in a transaction is that you changed the data dramatically.
    
    Well, the pg_statistic updates *are* transactional.  What we're discussing
    here is the reltuples/relpages fields, and it's worth thinking twice
    before claiming that you want to roll such an update back.  If the
    transaction rolls back that's not going to make physically added pages go
    away, so its relpages value is unconditionally better than the old one.
    In the simplest case of a transaction that's UPDATEd all/most rows of the
    table, it'd be best to keep the reltuples/relpages updates because that
    will correctly reflect the fact that the live tuple density is about half
    what it used to be.  (A rollback would mean that a different half of the
    tuples are live, but the ratio is still correct.)  If the transaction was
    mostly deletes or mostly inserts, then its reltuples number will be
    inaccurate if the transaction rolls back ... but the old relpages number
    will be inaccurate too, so it's pretty hard to argue that one state is
    better than the other.  Given the complete lack of user complaints in
    this area, I'm disinclined to change the behavior more than we have to.
    
    			regards, tom lane
    
    
    

16. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

    Tom Lane <tgl@sss.pgh.pa.us> — 2014-10-28T23:28:05Z

    I wrote:
    > I think that a better answer is to continue to do this update
    > nontransactionally, but to not let the code clear relhasindex etc
    > if we're inside a transaction block.  It is certainly safe to put
    > off clearing those flags if we're not sure that we're seeing a
    > committed state of the table's schema.
    
    Attached is a proposed patch to do it that way.  I borrowed Michael's
    test case.
    
    > An interesting question is whether it is ever possible for this function
    > to be told to *set* relhasindex when it was clear (or likewise for the
    > other flags).  Offhand I would say that that should never happen, because
    > certainly neither VACUUM nor ANALYZE should be creating indexes etc.
    > Should we make it throw an error if that happens, or just go ahead and
    > apply the update, assuming that it's correcting somehow-corrupted data?
    
    After looking more closely, the existing precedent for the other similar
    fields is just to make sure the code only clears the flags, never sets
    them, so I think relhasindex should be treated the same.
    
    			regards, tom lane
    
    

17. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

    Andres Freund <andres@2ndquadrant.com> — 2014-10-28T23:52:56Z

    On 2014-10-28 19:28:05 -0400, Tom Lane wrote:
    > I wrote:
    > > I think that a better answer is to continue to do this update
    > > nontransactionally, but to not let the code clear relhasindex etc
    > > if we're inside a transaction block.  It is certainly safe to put
    > > off clearing those flags if we're not sure that we're seeing a
    > > committed state of the table's schema.
    > 
    > Attached is a proposed patch to do it that way.  I borrowed Michael's
    > test case.
    
    I still think it'd be better to use a transactional update. But I also
    *do* agree that this is the safer way forward for now. So +1 from me.
    
    > +  *		In addition to fundamentally nontransactional statistics such as
    > +  *		relpages and relallvisible, we try to maintain certain lazily-updated
    > +  *		DDL flags such as relhasindex, by clearing them if no longer correct.
    > +  *		It's safe to do this in VACUUM, which can't run in parallel with
    > +  *		CREATE INDEX/RULE/TRIGGER and can't be part of a transaction block.
    
    > +  *		However, it's *not* safe to do it in an ANALYZE that's within a
    > +  *		transaction block, because the current transaction might've dropped
    > +  *		the last index; we'd think relhasindex should be cleared, but if the
    > +  *		transaction later rolls back this would be wrong.  So we refrain from
    > +  *		updating the DDL flags if we're inside a transaction block.  This is
    > +  *		OK since postponing the flag maintenance is always allowable.
    > +  *
    
    Absolutely minor nitpick: It nearly sounds like this is only a concern
    for relhasindex - but it's just as much a problem for other relhas*
    stuff. Not sure if it's worth complicating the text for that. Maybe a
    'e.g.'.
    
    Greetings,
    
    Andres Freund
    
    
    

18. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

    Michael Paquier <michael.paquier@gmail.com> — 2014-10-29T02:52:53Z

    On Wed, Oct 29, 2014 at 8:52 AM, Andres Freund <andres@2ndquadrant.com> wrote:
    > On 2014-10-28 19:28:05 -0400, Tom Lane wrote:
    >> I wrote:
    >> > I think that a better answer is to continue to do this update
    >> > nontransactionally, but to not let the code clear relhasindex etc
    >> > if we're inside a transaction block.  It is certainly safe to put
    >> > off clearing those flags if we're not sure that we're seeing a
    >> > committed state of the table's schema.
    >>
    >> Attached is a proposed patch to do it that way.  I borrowed Michael's
    >> test case.
    >
    > I still think it'd be better to use a transactional update. But I also
    > *do* agree that this is the safer way forward for now. So +1 from me.
    A transactional update would be better thinking long-term (ANALYZE is
    still transactional), but well this fix makes it as well. So no loud
    complains here and let's go with what is proposed.
    Regards,
    -- 
    Michael
    
    
    

19. Re: BUG #11638: Transaction safety fails when constraints are dropped and analyze is done

    Tom Lane <tgl@sss.pgh.pa.us> — 2014-10-29T22:15:59Z

    cg@osss.net writes:
    > When, within a transaction:
    > * I drop a foreign key (or any) constraint.
    > * Load some data to the table which won't fit the constraint.
    > * Analyze the table.
    > * Attempt to re-add the constraint which fails and should roll back the
    > whole transaction.
    > The constraint is still missing after rollback.
    > If I take out the analyze step, it works as expected.
    
    Fix committed, many thanks for chasing this down to the point of having a
    reproducible case!  I imagine this was a real bear to pinpoint.
    
    			regards, tom lane