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

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;