Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit19de0ab

Browse files
committed
Fix inadequate locking during get_rel_oids().
get_rel_oids used to not take any relation locks at all, but that stoppedbeing a good idea with commit3c3bb99, which inserted a syscache lookupinto the function. A concurrent DROP TABLE could now produce "cache lookupfailed", which we don't want to have happen in normal operation. The bestsolution seems to be to transiently take a lock on the relation named bythe RangeVar (which also makes the result of RangeVarGetRelid a lot lessspongy). But we shouldn't hold the lock beyond this function, because wedon't want VACUUM to lock more than one table at a time. (That would notbe a big problem right now, but it will become one after the pendingfeature patch to allow multiple tables to be named in VACUUM.)In passing, adjust vacuum_rel and analyze_rel to document that we don'ttrust the passed RangeVar to be accurate, and allow the RangeVar topossibly be NULL --- which it is anyway for a whole-database VACUUM,though we accidentally didn't crash for that case.The passed RangeVar is in fact inaccurate when dealing with a childpartition, as of v10, and it has been wrong for a whole long time in thecase of vacuum_rel() recursing to a TOAST table. None of these thingspresent visible bugs up to now, because the passed RangeVar is in factonly consulted for autovacuum logging, and in that particular context it'salways accurate because autovacuum doesn't let vacuum.c expand partitionsnor recurse to toast tables. Still, this seems like trouble waiting tohappen, so let's nail the door at least partly shut. (Further cleanupis planned, in HEAD only, as part of the pending feature patch.)Fix some sadly inaccurate/obsolete comments too. Back-patch to v10.Michael Paquier and Tom LaneDiscussion:https://postgr.es/m/25023.1506107590@sss.pgh.pa.us
1 parent69c1698 commit19de0ab

File tree

2 files changed

+45
-22
lines changed

2 files changed

+45
-22
lines changed

‎src/backend/commands/analyze.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,10 @@ static Datum ind_fetch_func(VacAttrStatsP stats, int rownum, bool *isNull);
106106

107107
/*
108108
*analyze_rel() -- analyze one relation
109+
*
110+
* relid identifies the relation to analyze. If relation is supplied, use
111+
* the name therein for reporting any failure to open/lock the rel; do not
112+
* use it once we've successfully opened the rel, since it might be stale.
109113
*/
110114
void
111115
analyze_rel(Oidrelid,RangeVar*relation,intoptions,
@@ -145,7 +149,8 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
145149
else
146150
{
147151
onerel=NULL;
148-
if (IsAutoVacuumWorkerProcess()&&params->log_min_duration >=0)
152+
if (relation&&
153+
IsAutoVacuumWorkerProcess()&&params->log_min_duration >=0)
149154
ereport(LOG,
150155
(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
151156
errmsg("skipping analyze of \"%s\" --- lock not available",

‎src/backend/commands/vacuum.c

Lines changed: 39 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -128,9 +128,11 @@ ExecVacuum(VacuumStmt *vacstmt, bool isTopLevel)
128128
*
129129
* options is a bitmask of VacuumOption flags, indicating what to do.
130130
*
131-
* relid, if not InvalidOid, indicate the relation to process; otherwise,
132-
* the RangeVar is used. (The latter must always be passed, because it's
133-
* used for error messages.)
131+
* relid, if not InvalidOid, indicates the relation to process; otherwise,
132+
* if a RangeVar is supplied, that's what to process; otherwise, we process
133+
* all relevant tables in the database. (If both relid and a RangeVar are
134+
* supplied, the relid is what is processed, but we use the RangeVar's name
135+
* to report any open/lock failure.)
134136
*
135137
* params contains a set of parameters that can be used to customize the
136138
* behavior.
@@ -226,8 +228,8 @@ vacuum(int options, RangeVar *relation, Oid relid, VacuumParams *params,
226228
vac_strategy=bstrategy;
227229

228230
/*
229-
* Build list ofrelationsto process,unless caller gave us one. (If we
230-
*build one, we put it in vac_context forsafekeeping.)
231+
* Build list ofrelation OID(s)to process,putting it in vac_context for
232+
* safekeeping.
231233
*/
232234
relations=get_rel_oids(relid,relation);
233235

@@ -393,22 +395,18 @@ get_rel_oids(Oid relid, const RangeVar *vacrel)
393395
}
394396
elseif (vacrel)
395397
{
396-
/* Process a specific relation */
398+
/* Process a specific relation, and possibly partitions thereof */
397399
Oidrelid;
398400
HeapTupletuple;
399401
Form_pg_classclassForm;
400402
boolinclude_parts;
401403

402404
/*
403-
* Since we don't take a lock here, the relation might be gone, or the
404-
* RangeVar might no longer refer to the OID we look up here. In the
405-
* former case, VACUUM will do nothing; in the latter case, it will
406-
* process the OID we looked up here, rather than the new one. Neither
407-
* is ideal, but there's little practical alternative, since we're
408-
* going to commit this transaction and begin a new one between now
409-
* and then.
405+
* We transiently take AccessShareLock to protect the syscache lookup
406+
* below, as well as find_all_inheritors's expectation that the caller
407+
* holds some lock on the starting relation.
410408
*/
411-
relid=RangeVarGetRelid(vacrel,NoLock, false);
409+
relid=RangeVarGetRelid(vacrel,AccessShareLock, false);
412410

413411
/*
414412
* To check whether the relation is a partitioned table, fetch its
@@ -422,10 +420,11 @@ get_rel_oids(Oid relid, const RangeVar *vacrel)
422420
ReleaseSysCache(tuple);
423421

424422
/*
425-
* Make relation list entries for this guy and its partitions, if any.
426-
* Note that the list returned by find_all_inheritors() include the
427-
* passed-in OID at its head. Also note that we did not request a
428-
* lock to be taken to match what would be done otherwise.
423+
* Make relation list entries for this rel and its partitions, if any.
424+
* Note that the list returned by find_all_inheritors() includes the
425+
* passed-in OID at its head. There's no point in taking locks on the
426+
* individual partitions yet, and doing so would just add unnecessary
427+
* deadlock risk.
429428
*/
430429
oldcontext=MemoryContextSwitchTo(vac_context);
431430
if (include_parts)
@@ -434,6 +433,19 @@ get_rel_oids(Oid relid, const RangeVar *vacrel)
434433
else
435434
oid_list=lappend_oid(oid_list,relid);
436435
MemoryContextSwitchTo(oldcontext);
436+
437+
/*
438+
* Release lock again. This means that by the time we actually try to
439+
* process the table, it might be gone or renamed. In the former case
440+
* we'll silently ignore it; in the latter case we'll process it
441+
* anyway, but we must beware that the RangeVar doesn't necessarily
442+
* identify it anymore. This isn't ideal, perhaps, but there's little
443+
* practical alternative, since we're typically going to commit this
444+
* transaction and begin a new one between now and then. Moreover,
445+
* holding locks on multiple relations would create significant risk
446+
* of deadlock.
447+
*/
448+
UnlockRelationOid(relid,AccessShareLock);
437449
}
438450
else
439451
{
@@ -463,7 +475,7 @@ get_rel_oids(Oid relid, const RangeVar *vacrel)
463475
classForm->relkind!=RELKIND_PARTITIONED_TABLE)
464476
continue;
465477

466-
/* Make a relation list entry for thisguy */
478+
/* Make a relation list entry for thisrel */
467479
oldcontext=MemoryContextSwitchTo(vac_context);
468480
oid_list=lappend_oid(oid_list,HeapTupleGetOid(tuple));
469481
MemoryContextSwitchTo(oldcontext);
@@ -1212,6 +1224,11 @@ vac_truncate_clog(TransactionId frozenXID,
12121224
/*
12131225
*vacuum_rel() -- vacuum one heap relation
12141226
*
1227+
*relid identifies the relation to vacuum. If relation is supplied,
1228+
*use the name therein for reporting any failure to open/lock the rel;
1229+
*do not use it once we've successfully opened the rel, since it might
1230+
*be stale.
1231+
*
12151232
*Doing one heap at a time incurs extra overhead, since we need to
12161233
*check that the heap exists again just before we vacuum it. The
12171234
*reason that we do this is so that vacuuming can be spread across
@@ -1300,7 +1317,8 @@ vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params)
13001317
else
13011318
{
13021319
onerel=NULL;
1303-
if (IsAutoVacuumWorkerProcess()&&params->log_min_duration >=0)
1320+
if (relation&&
1321+
IsAutoVacuumWorkerProcess()&&params->log_min_duration >=0)
13041322
ereport(LOG,
13051323
(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
13061324
errmsg("skipping vacuum of \"%s\" --- lock not available",
@@ -1468,7 +1486,7 @@ vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params)
14681486
* totally unimportant for toast relations.
14691487
*/
14701488
if (toast_relid!=InvalidOid)
1471-
vacuum_rel(toast_relid,relation,options,params);
1489+
vacuum_rel(toast_relid,NULL,options,params);
14721490

14731491
/*
14741492
* Now release the session-level lock on the master table.

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp