Test IsInTransactionChain, not IsTransactionBlock, in vac_update_relstats.

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
index 0669d241a2fc437c1ed4d7c4a49a85e04445de6d..b34eebe01b8734f9176943f836b0d011da77a0cd 100644 (file)
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -82,7 +82,8 @@ static MemoryContext anl_context = NULL;
 static BufferAccessStrategy vac_strategy;
 
 
-static void do_analyze_rel(Relation onerel, VacuumStmt *vacstmt, bool inh);
+static void do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
+                          bool inh, bool in_outer_xact);
 static void BlockSampler_Init(BlockSampler bs, BlockNumber nblocks,
                                  int samplesize);
 static bool BlockSampler_HasMore(BlockSampler bs);
@@ -114,7 +115,8 @@ static bool std_typanalyze(VacAttrStats *stats);
  *     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;
 
@@ -214,13 +216,13 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt, BufferAccessStrategy bstrategy)
        /*
         * Do the normal non-recursive ANALYZE.
         */
-       do_analyze_rel(onerel, vacstmt, false);
+       do_analyze_rel(onerel, vacstmt, false, in_outer_xact);
 
        /*
         * If there are child tables, do recursive ANALYZE.
         */
        if (onerel->rd_rel->relhassubclass)
-               do_analyze_rel(onerel, vacstmt, true);
+               do_analyze_rel(onerel, vacstmt, true, in_outer_xact);
 
        /*
         * Close source relation now, but keep lock so that no one deletes it
@@ -243,7 +245,8 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt, BufferAccessStrategy bstrategy)
  *     do_analyze_rel() -- analyze one relation, recursively or not
  */
 static void
-do_analyze_rel(Relation onerel, VacuumStmt *vacstmt, bool inh)
+do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
+                          bool inh, bool in_outer_xact)
 {
        int                     attr_cnt,
                                tcnt,
@@ -520,7 +523,10 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt, bool inh)
        if (!inh)
                vac_update_relstats(onerel,
                                                        RelationGetNumberOfBlocks(onerel),
-                                                       totalrows, hasindex, InvalidTransactionId);
+                                                       totalrows,
+                                                       hasindex,
+                                                       InvalidTransactionId,
+                                                       in_outer_xact);
 
        /*
         * Same for indexes. Vacuum always scans all indexes, so if we're part of
@@ -537,7 +543,10 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt, bool inh)
                        totalindexrows = ceil(thisdata->tupleFract * totalrows);
                        vac_update_relstats(Irel[ind],
                                                                RelationGetNumberOfBlocks(Irel[ind]),
-                                                               totalindexrows, false, InvalidTransactionId);
+                                                               totalindexrows,
+                                                               false,
+                                                               InvalidTransactionId,
+                                                               in_outer_xact);
                }
        }
 

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 4c3f7722468637c00cc78ca4f1bf54acb10fce83..30bcce4d3ecebac1ba0cc6b9e2695586285ddbe3 100644 (file)
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -204,6 +204,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();
@@ -243,7 +245,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)
                                {
@@ -557,20 +559,21 @@ 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,
-                                       bool hasindex, TransactionId frozenxid)
+                                       bool hasindex, TransactionId frozenxid,
+                                       bool in_outer_xact)
 {
        Oid                     relid = RelationGetRelid(relation);
        Relation        rd;
@@ -601,9 +604,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
index d0839e6919489d2bda3ab2860ae82534f589c798..4cd4a42ce3c5da0ebdba210ade73acb31229e66f 100644 (file)
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -266,7 +266,8 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
        vac_update_relstats(onerel,
                                                new_rel_pages, new_rel_tuples,
                                                vacrelstats->hasindex,
-                                               new_frozen_xid);
+                                               new_frozen_xid,
+                                               false);
 
        /* report results to the stats collector, too */
        pgstat_report_vacuum(RelationGetRelid(onerel),
@@ -1091,7 +1092,9 @@ lazy_cleanup_index(Relation indrel,
        if (!stats->estimated_count)
                vac_update_relstats(indrel,
                                                        stats->num_pages, stats->num_index_tuples,
-                                                       false, InvalidTransactionId);
+                                                       false,
+                                                       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
index 2343fb197218dfa60ae1ef4e48db72469872f18d..3d9a1b32d6b8a8f629348694f196eea574ee3a4c 100644 (file)
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -146,7 +146,8 @@ extern void vac_update_relstats(Relation relation,
                                        BlockNumber num_pages,
                                        double num_tuples,
                                        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,
@@ -161,6 +162,6 @@ 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);
 
 #endif   /* VACUUM_H */