Skip to content

Commit 73f950f

Browse files
tglsfdctglsfdc
committed
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.
1 parent 9d06da5 commit 73f950f

File tree

4 files changed

+36
-20
lines changed

4 files changed

+36
-20
lines changed

‎src/backend/commands/analyze.c

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,8 @@ static MemoryContext anl_context = NULL;
8282
static BufferAccessStrategy vac_strategy;
8383

8484

85-
static void do_analyze_rel(Relation onerel, VacuumStmt *vacstmt, bool inh);
85+
static void do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
86+
bool inh, bool in_outer_xact);
8687
static void BlockSampler_Init(BlockSampler bs, BlockNumber nblocks,
8788
int samplesize);
8889
static bool BlockSampler_HasMore(BlockSampler bs);
@@ -114,7 +115,8 @@ static bool std_typanalyze(VacAttrStats *stats);
114115
* analyze_rel() -- analyze one relation
115116
*/
116117
void
117-
analyze_rel(Oid relid, VacuumStmt *vacstmt, BufferAccessStrategy bstrategy)
118+
analyze_rel(Oid relid, VacuumStmt *vacstmt,
119+
bool in_outer_xact, BufferAccessStrategy bstrategy)
118120
{
119121
Relation onerel;
120122

@@ -214,13 +216,13 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt, BufferAccessStrategy bstrategy)
214216
/*
215217
* Do the normal non-recursive ANALYZE.
216218
*/
217-
do_analyze_rel(onerel, vacstmt, false);
219+
do_analyze_rel(onerel, vacstmt, false, in_outer_xact);
218220

219221
/*
220222
* If there are child tables, do recursive ANALYZE.
221223
*/
222224
if (onerel->rd_rel->relhassubclass)
223-
do_analyze_rel(onerel, vacstmt, true);
225+
do_analyze_rel(onerel, vacstmt, true, in_outer_xact);
224226

225227
/*
226228
* 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)
243245
* do_analyze_rel() -- analyze one relation, recursively or not
244246
*/
245247
static void
246-
do_analyze_rel(Relation onerel, VacuumStmt *vacstmt, bool inh)
248+
do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
249+
bool inh, bool in_outer_xact)
247250
{
248251
int attr_cnt,
249252
tcnt,
@@ -520,7 +523,10 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt, bool inh)
520523
if (!inh)
521524
vac_update_relstats(onerel,
522525
RelationGetNumberOfBlocks(onerel),
523-
totalrows, hasindex, InvalidTransactionId);
526+
totalrows,
527+
hasindex,
528+
InvalidTransactionId,
529+
in_outer_xact);
524530

525531
/*
526532
* 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)
537543
totalindexrows = ceil(thisdata->tupleFract * totalrows);
538544
vac_update_relstats(Irel[ind],
539545
RelationGetNumberOfBlocks(Irel[ind]),
540-
totalindexrows, false, InvalidTransactionId);
546+
totalindexrows,
547+
false,
548+
InvalidTransactionId,
549+
in_outer_xact);
541550
}
542551
}
543552

‎src/backend/commands/vacuum.c

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,8 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
204204
*/
205205
if (use_own_xacts)
206206
{
207+
Assert(!in_outer_xact);
208+
207209
/* ActiveSnapshot is not set by autovacuum */
208210
if (ActiveSnapshotSet())
209211
PopActiveSnapshot();
@@ -243,7 +245,7 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
243245
PushActiveSnapshot(GetTransactionSnapshot());
244246
}
245247

246-
analyze_rel(relid, vacstmt, vac_strategy);
248+
analyze_rel(relid, vacstmt, in_outer_xact, vac_strategy);
247249

248250
if (use_own_xacts)
249251
{
@@ -557,20 +559,21 @@ vac_estimate_reltuples(Relation relation, bool is_analyze,
557559
* DDL flags such as relhasindex, by clearing them if no longer correct.
558560
* It's safe to do this in VACUUM, which can't run in parallel with
559561
* CREATE INDEX/RULE/TRIGGER and can't be part of a transaction block.
560-
* However, it's *not* safe to do it in an ANALYZE that's within a
561-
* transaction block, because for example the current transaction might
562+
* However, it's *not* safe to do it in an ANALYZE that's within an
563+
* outer transaction, because for example the current transaction might
562564
* have dropped the last index; then we'd think relhasindex should be
563565
* cleared, but if the transaction later rolls back this would be wrong.
564-
* So we refrain from updating the DDL flags if we're inside a
565-
* transaction block. This is OK since postponing the flag maintenance
566-
* is always allowable.
566+
* So we refrain from updating the DDL flags if we're inside an outer
567+
* transaction. This is OK since postponing the flag maintenance is
568+
* always allowable.
567569
*
568570
* This routine is shared by VACUUM and ANALYZE.
569571
*/
570572
void
571573
vac_update_relstats(Relation relation,
572574
BlockNumber num_pages, double num_tuples,
573-
bool hasindex, TransactionId frozenxid)
575+
bool hasindex, TransactionId frozenxid,
576+
bool in_outer_xact)
574577
{
575578
Oid relid = RelationGetRelid(relation);
576579
Relation rd;
@@ -601,9 +604,9 @@ vac_update_relstats(Relation relation,
601604
dirty = true;
602605
}
603606

604-
/* Apply DDL updates, but not inside a transaction block (see above) */
607+
/* Apply DDL updates, but not inside an outer transaction (see above) */
605608

606-
if (!IsTransactionBlock())
609+
if (!in_outer_xact)
607610
{
608611
/*
609612
* If we didn't find any indexes, reset relhasindex.

‎src/backend/commands/vacuumlazy.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,8 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
266266
vac_update_relstats(onerel,
267267
new_rel_pages, new_rel_tuples,
268268
vacrelstats->hasindex,
269-
new_frozen_xid);
269+
new_frozen_xid,
270+
false);
270271

271272
/* report results to the stats collector, too */
272273
pgstat_report_vacuum(RelationGetRelid(onerel),
@@ -1091,7 +1092,9 @@ lazy_cleanup_index(Relation indrel,
10911092
if (!stats->estimated_count)
10921093
vac_update_relstats(indrel,
10931094
stats->num_pages, stats->num_index_tuples,
1094-
false, InvalidTransactionId);
1095+
false,
1096+
InvalidTransactionId,
1097+
false);
10951098

10961099
ereport(elevel,
10971100
(errmsg("index \"%s\" now contains %.0f row versions in %u pages",

‎src/include/commands/vacuum.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,8 @@ extern void vac_update_relstats(Relation relation,
146146
BlockNumber num_pages,
147147
double num_tuples,
148148
bool hasindex,
149-
TransactionId frozenxid);
149+
TransactionId frozenxid,
150+
bool in_outer_xact);
150151
extern void vacuum_set_xid_limits(int freeze_min_age, int freeze_table_age,
151152
bool sharedRel,
152153
TransactionId *oldestXmin,
@@ -161,6 +162,6 @@ extern void lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
161162

162163
/* in commands/analyze.c */
163164
extern void analyze_rel(Oid relid, VacuumStmt *vacstmt,
164-
BufferAccessStrategy bstrategy);
165+
bool in_outer_xact, BufferAccessStrategy bstrategy);
165166

166167
#endif /* VACUUM_H */

0 commit comments

Comments
 (0)