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

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.

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

On Oct 10, 2014, at 3:14 PM, Andres Freund <andres@2ndquadrant.com> =
wrote:
>=20
> 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=20
-----------+---------+-----------
 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)=3D(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=20
-----------+---------+-----------
 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=20
-----------+---------+-----------
 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)=3D(1) is not present in table "table1".
ROLLBACK
Time: 0.150 ms
       Table "test.table2"
  Column   |  Type   | Modifiers=20
-----------+---------+-----------
 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,
--=20
Casey Allen Shobe=

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

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

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

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




> On Oct 15, 2014, at 12:17 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
>
>
>
> 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
> <create.sql><test.sql>

On Oct 15, 2014, at 12:15 AM, Michael Paquier =
<michael.paquier@gmail.com> wrote:
>=20
> 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,
--=20
Casey Allen Shobe=

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

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

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

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,
--=20
Casey=

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

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index e5fefa3..cfa4a1d 100644
*** a/src/backend/commands/vacuum.c
--- b/src/backend/commands/vacuum.c
*************** vac_estimate_reltuples(Relation relation
*** 648,670 ****
   *
   *        We violate transaction semantics here by overwriting the rel's
   *        existing pg_class tuple with the new values.  This is reasonably
!  *        safe since the new values are correct whether or not this transaction
!  *        commits.  The reason for this is that if we updated these tuples in
!  *        the usual way, vacuuming pg_class itself wouldn't work very well ---
!  *        by the time we got done with a vacuum cycle, most of the tuples in
!  *        pg_class would've been obsoleted.  Of course, this only works for
!  *        fixed-size never-null columns, but these are.
!  *
!  *        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.
   *
   *        Another reason for doing it this way is that when we are in a lazy
!  *        VACUUM and have PROC_IN_VACUUM set, we mustn't do any updates ---
!  *        somebody vacuuming pg_class might think they could delete a tuple
   *        marked with xmin = our xid.
   *
   *        This routine is shared by VACUUM and ANALYZE.
   */
  void
--- 648,677 ----
   *
   *        We violate transaction semantics here by overwriting the rel's
   *        existing pg_class tuple with the new values.  This is reasonably
!  *        safe as long as we're sure that the new values are correct whether or
!  *        not this transaction commits.  The reason for doing this is that if
!  *        we updated these tuples in the usual way, vacuuming pg_class itself
!  *        wouldn't work very well --- by the time we got done with a vacuum
!  *        cycle, most of the tuples in pg_class would've been obsoleted.  Of
!  *        course, this only works for fixed-size not-null columns, but these are.
   *
   *        Another reason for doing it this way is that when we are in a lazy
!  *        VACUUM and have PROC_IN_VACUUM set, we mustn't do any regular updates.
!  *        Somebody vacuuming pg_class might think they could delete a tuple
   *        marked with xmin = our xid.
   *
+  *        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.
+  *
   *        This routine is shared by VACUUM and ANALYZE.
   */
  void
*************** vac_update_relstats(Relation relation,
*** 689,695 ****
               relid);
      pgcform = (Form_pg_class) GETSTRUCT(ctup);

!     /* Apply required updates, if any, to copied tuple */

      dirty = false;
      if (pgcform->relpages != (int32) num_pages)
--- 696,702 ----
               relid);
      pgcform = (Form_pg_class) GETSTRUCT(ctup);

!     /* Apply statistical updates, if any, to copied tuple */

      dirty = false;
      if (pgcform->relpages != (int32) num_pages)
*************** vac_update_relstats(Relation relation,
*** 707,738 ****
          pgcform->relallvisible = (int32) num_all_visible_pages;
          dirty = true;
      }
-     if (pgcform->relhasindex != hasindex)
-     {
-         pgcform->relhasindex = hasindex;
-         dirty = true;
-     }

!     /*
!      * If we have discovered that there are no indexes, then there's no
!      * primary key either.  This could be done more thoroughly...
!      */
!     if (pgcform->relhaspkey && !hasindex)
!     {
!         pgcform->relhaspkey = false;
!         dirty = true;
!     }

!     /* We also clear relhasrules and relhastriggers if needed */
!     if (pgcform->relhasrules && relation->rd_rules == NULL)
!     {
!         pgcform->relhasrules = false;
!         dirty = true;
!     }
!     if (pgcform->relhastriggers && relation->trigdesc == NULL)
      {
!         pgcform->relhastriggers = false;
!         dirty = true;
      }

      /*
--- 714,754 ----
          pgcform->relallvisible = (int32) num_all_visible_pages;
          dirty = true;
      }

!     /* Apply DDL updates, but not inside a transaction block (see above) */

!     if (!IsTransactionBlock())
      {
!         /*
!          * If we didn't find any indexes, reset relhasindex.
!          */
!         if (pgcform->relhasindex && !hasindex)
!         {
!             pgcform->relhasindex = false;
!             dirty = true;
!         }
!
!         /*
!          * If we have discovered that there are no indexes, then there's no
!          * primary key either.  This could be done more thoroughly...
!          */
!         if (pgcform->relhaspkey && !hasindex)
!         {
!             pgcform->relhaspkey = false;
!             dirty = true;
!         }
!
!         /* We also clear relhasrules and relhastriggers if needed */
!         if (pgcform->relhasrules && relation->rd_rules == NULL)
!         {
!             pgcform->relhasrules = false;
!             dirty = true;
!         }
!         if (pgcform->relhastriggers && relation->trigdesc == NULL)
!         {
!             pgcform->relhastriggers = false;
!             dirty = true;
!         }
      }

      /*
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 10f45f2..f5ae511 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
*************** ALTER TABLE logged1 SET UNLOGGED; -- sil
*** 2517,2519 ****
--- 2517,2539 ----
  DROP TABLE logged3;
  DROP TABLE logged2;
  DROP TABLE logged1;
+ -- check presence of foreign key with transactional ANALYZE
+ CREATE TABLE check_fk_presence_1 (id int PRIMARY KEY, t text);
+ CREATE TABLE check_fk_presence_2 (id int REFERENCES check_fk_presence_1, t text);
+ BEGIN;
+ ALTER TABLE check_fk_presence_2 DROP CONSTRAINT check_fk_presence_2_id_fkey;
+ COPY check_fk_presence_2 FROM stdin;
+ ANALYZE check_fk_presence_2;
+ ALTER TABLE check_fk_presence_2 ADD FOREIGN KEY (table1_fk) REFERENCES check_fk_presence_1 (id);
+ ERROR:  column "table1_fk" referenced in foreign key constraint does not exist
+ ROLLBACK;
+ \d check_fk_presence_2
+ Table "public.check_fk_presence_2"
+  Column |  Type   | Modifiers
+ --------+---------+-----------
+  id     | integer |
+  t      | text    |
+ Foreign-key constraints:
+     "check_fk_presence_2_id_fkey" FOREIGN KEY (id) REFERENCES check_fk_presence_1(id)
+
+ DROP TABLE check_fk_presence_1, check_fk_presence_2;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 12fd7c2..05d8226 100644
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
*************** ALTER TABLE logged1 SET UNLOGGED; -- sil
*** 1676,1678 ****
--- 1676,1691 ----
  DROP TABLE logged3;
  DROP TABLE logged2;
  DROP TABLE logged1;
+ -- check presence of foreign key with transactional ANALYZE
+ CREATE TABLE check_fk_presence_1 (id int PRIMARY KEY, t text);
+ CREATE TABLE check_fk_presence_2 (id int REFERENCES check_fk_presence_1, t text);
+ BEGIN;
+ ALTER TABLE check_fk_presence_2 DROP CONSTRAINT check_fk_presence_2_id_fkey;
+ COPY check_fk_presence_2 FROM stdin;
+ 1    test
+ \.
+ ANALYZE check_fk_presence_2;
+ ALTER TABLE check_fk_presence_2 ADD FOREIGN KEY (table1_fk) REFERENCES check_fk_presence_1 (id);
+ ROLLBACK;
+ \d check_fk_presence_2
+ DROP TABLE check_fk_presence_1, check_fk_presence_2;

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

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

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