Test IsInTransactionChain, not IsTransactionBlock, in vac_update_relstats.

tglsfdc · tglsfdc · commit 38cb8687a9dd · 2014-10-30T13:03:31.000-04:00
As noted by Noah Misch, my initial cut at fixing bug #11638 didn't cover
all cases where ANALYZE might be invoked in an unsafe context.  We need to
test the result of IsInTransactionChain not IsTransactionBlock; which is
notationally a pain because IsInTransactionChain requires an isTopLevel
flag, which would have to be passed down through several levels of callers.
I chose to pass in_outer_xact (ie, the result of IsInTransactionChain)
rather than isTopLevel per se, as that seemed marginally more apropos
for the intermediate functions to know about.
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
@@ -85,7 +85,7 @@ static BufferAccessStrategy vac_strategy;
 
 static void do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 			   AcquireSampleRowsFunc acquirefunc, BlockNumber relpages,
-			   bool inh, int elevel);
+			   bool inh, bool in_outer_xact, int elevel);
 static void BlockSampler_Init(BlockSampler bs, BlockNumber nblocks,
 				  int samplesize);
 static bool BlockSampler_HasMore(BlockSampler bs);
@@ -113,7 +113,8 @@ static Datum ind_fetch_func(VacAttrStatsP stats, int rownum, bool *isNull);
  *	analyze_rel() -- analyze one relation
  */
 void
-analyze_rel(Oid relid, VacuumStmt *vacstmt, BufferAccessStrategy bstrategy)
+analyze_rel(Oid relid, VacuumStmt *vacstmt,
+			bool in_outer_xact, BufferAccessStrategy bstrategy)
 {
 	Relation	onerel;
 	int			elevel;
@@ -262,13 +263,15 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt, BufferAccessStrategy bstrategy)
 	/*
 	 * Do the normal non-recursive ANALYZE.
 	 */
-	do_analyze_rel(onerel, vacstmt, acquirefunc, relpages, false, elevel);
+	do_analyze_rel(onerel, vacstmt, acquirefunc, relpages,
+				   false, in_outer_xact, elevel);
 
 	/*
 	 * If there are child tables, do recursive ANALYZE.
 	 */
 	if (onerel->rd_rel->relhassubclass)
-		do_analyze_rel(onerel, vacstmt, acquirefunc, relpages, true, elevel);
+		do_analyze_rel(onerel, vacstmt, acquirefunc, relpages,
+					   true, in_outer_xact, elevel);
 
 	/*
 	 * Close source relation now, but keep lock so that no one deletes it
@@ -298,7 +301,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt, BufferAccessStrategy bstrategy)
 static void
 do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 			   AcquireSampleRowsFunc acquirefunc, BlockNumber relpages,
-			   bool inh, int elevel)
+			   bool inh, bool in_outer_xact, int elevel)
 {
 	int			attr_cnt,
 				tcnt,
@@ -580,7 +583,8 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 							totalrows,
 							visibilitymap_count(onerel),
 							hasindex,
-							InvalidTransactionId);
+							InvalidTransactionId,
+							in_outer_xact);
 
 	/*
 	 * Same for indexes. Vacuum always scans all indexes, so if we're part of
@@ -600,7 +604,8 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 								totalindexrows,
 								0,
 								false,
-								InvalidTransactionId);
+								InvalidTransactionId,
+								in_outer_xact);
 		}
 	}
 
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
@@ -199,6 +199,8 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 	 */
 	if (use_own_xacts)
 	{
+		Assert(!in_outer_xact);
+
 		/* ActiveSnapshot is not set by autovacuum */
 		if (ActiveSnapshotSet())
 			PopActiveSnapshot();
@@ -244,7 +246,7 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 					PushActiveSnapshot(GetTransactionSnapshot());
 				}
 
-				analyze_rel(relid, vacstmt, vac_strategy);
+				analyze_rel(relid, vacstmt, in_outer_xact, vac_strategy);
 
 				if (use_own_xacts)
 				{
@@ -567,21 +569,22 @@ vac_estimate_reltuples(Relation relation, bool is_analyze,
  *		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 for example the current transaction might
+ *		However, it's *not* safe to do it in an ANALYZE that's within an
+ *		outer transaction, because for example the current transaction might
  *		have dropped the last index; then 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.
+ *		So we refrain from updating the DDL flags if we're inside an outer
+ *		transaction.  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,
 					BlockNumber num_pages, double num_tuples,
 					BlockNumber num_all_visible_pages,
-					bool hasindex, TransactionId frozenxid)
+					bool hasindex, TransactionId frozenxid,
+					bool in_outer_xact)
 {
 	Oid			relid = RelationGetRelid(relation);
 	Relation	rd;
@@ -617,9 +620,9 @@ vac_update_relstats(Relation relation,
 		dirty = true;
 	}
 
-	/* Apply DDL updates, but not inside a transaction block (see above) */
+	/* Apply DDL updates, but not inside an outer transaction (see above) */
 
-	if (!IsTransactionBlock())
+	if (!in_outer_xact)
 	{
 		/*
 		 * If we didn't find any indexes, reset relhasindex.
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
@@ -284,7 +284,8 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 						new_rel_tuples,
 						new_rel_allvisible,
 						vacrelstats->hasindex,
-						new_frozen_xid);
+						new_frozen_xid,
+						false);
 
 	/* report results to the stats collector, too */
 	pgstat_report_vacuum(RelationGetRelid(onerel),
@@ -1283,7 +1284,8 @@ lazy_cleanup_index(Relation indrel,
 							stats->num_index_tuples,
 							0,
 							false,
-							InvalidTransactionId);
+							InvalidTransactionId,
+							false);
 
 	ereport(elevel,
 			(errmsg("index \"%s\" now contains %.0f row versions in %u pages",
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
@@ -153,7 +153,8 @@ extern void vac_update_relstats(Relation relation,
 					double num_tuples,
 					BlockNumber num_all_visible_pages,
 					bool hasindex,
-					TransactionId frozenxid);
+					TransactionId frozenxid,
+					bool in_outer_xact);
 extern void vacuum_set_xid_limits(int freeze_min_age, int freeze_table_age,
 					  bool sharedRel,
 					  TransactionId *oldestXmin,
@@ -168,7 +169,7 @@ extern void lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 
 /* in commands/analyze.c */
 extern void analyze_rel(Oid relid, VacuumStmt *vacstmt,
-			BufferAccessStrategy bstrategy);
+			bool in_outer_xact, BufferAccessStrategy bstrategy);
 extern bool std_typanalyze(VacAttrStats *stats);
 extern double anl_random_fract(void);
 extern double anl_init_selection_state(int n);
