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

Commita556549

Browse files
committed
Improve VACUUM and ANALYZE by avoiding early lock queue
A caller of VACUUM can perform early lookup obtention which can causeother sessions to block on the request done, causing potentially DOSattacks as even a non-privileged user can attempt a vacuum fill of acritical catalog table to block even all incoming connection attempts.Contrary to TRUNCATE, a client could attempt a system-wide VACUUM afterbuilding the list of relations to VACUUM, which can cause vacuum_rel()or analyze_rel() to try to lock the relation but the operation wouldjust block. When the client specifies a list of relations and therelation needs to be skipped, ownership checks are done when buildingthe list of relations to work on, preventing a later lock attempt.vacuum_rel() already had the sanity checks needed, except that thosewere applied too late. This commit refactors the code so as relationskips are checked beforehand, making it safer to avoid too early locks,for both manual VACUUM with and without a list of relations specified.An isolation test is added emulating the fact that early locks do nothappen anymore, issuing a WARNING message earlier if the user callingVACUUM is not a relation owner.When a partitioned table is listed in a manual VACUUM or ANALYZEcommand, its full list of partitions is fetched, all partitions getadded to the list to work on, and then each one of them is processed oneby one, with ownership checks happening at the later phase ofvacuum_rel() or analyze_rel(). Trying to do early ownership checks foreach partition is proving to be tedious as this would result in deadlockrisks with lock upgrades, and skipping all partitions if the listedpartitioned table is not owned would result in a behavior changecompared to how Postgres 10 has implemented vacuum for partitionedtables. The original problem reported related to early lock queue forcritical relations is fixed anyway, so priority is given to avoiding abackward-incompatible behavior.Reported-by: Lloyd Albin, Jeremy SchneiderAuthor: Michael PaquierReviewed by: Nathan Bossart, Kyotaro HoriguchiDiscussion:https://postgr.es/m/152512087100.19803.12733865831237526317@wrigleys.postgresql.orgDiscussion:https://postgr.es/m/20180812222142.GA6097@paquier.xyz
1 parent18e5867 commita556549

File tree

6 files changed

+328
-60
lines changed

6 files changed

+328
-60
lines changed

‎src/backend/commands/analyze.c

Lines changed: 9 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -196,27 +196,17 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
196196
}
197197

198198
/*
199-
* Check permissions --- this should match vacuum's check!
199+
* Check if relation needs to be skipped based on ownership. This check
200+
* happens also when building the relation list to analyze for a manual
201+
* operation, and needs to be done additionally here as ANALYZE could
202+
* happen across multiple transactions where relation ownership could have
203+
* changed in-between. Make sure to generate only logs for ANALYZE in
204+
* this case.
200205
*/
201-
if (!(pg_class_ownercheck(RelationGetRelid(onerel),GetUserId())||
202-
(pg_database_ownercheck(MyDatabaseId,GetUserId())&& !onerel->rd_rel->relisshared)))
206+
if (!vacuum_is_relation_owner(RelationGetRelid(onerel),
207+
onerel->rd_rel,
208+
options&VACOPT_ANALYZE))
203209
{
204-
/* No need for a WARNING if we already complained during VACUUM */
205-
if (!(options&VACOPT_VACUUM))
206-
{
207-
if (onerel->rd_rel->relisshared)
208-
ereport(WARNING,
209-
(errmsg("skipping \"%s\" --- only superuser can analyze it",
210-
RelationGetRelationName(onerel))));
211-
elseif (onerel->rd_rel->relnamespace==PG_CATALOG_NAMESPACE)
212-
ereport(WARNING,
213-
(errmsg("skipping \"%s\" --- only superuser or database owner can analyze it",
214-
RelationGetRelationName(onerel))));
215-
else
216-
ereport(WARNING,
217-
(errmsg("skipping \"%s\" --- only table or database owner can analyze it",
218-
RelationGetRelationName(onerel))));
219-
}
220210
relation_close(onerel,ShareUpdateExclusiveLock);
221211
return;
222212
}

‎src/backend/commands/vacuum.c

Lines changed: 115 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,8 @@ static BufferAccessStrategy vac_strategy;
6868

6969

7070
/* non-export function prototypes */
71-
staticList*expand_vacuum_rel(VacuumRelation*vrel);
72-
staticList*get_all_vacuum_rels(void);
71+
staticList*expand_vacuum_rel(VacuumRelation*vrel,intoptions);
72+
staticList*get_all_vacuum_rels(intoptions);
7373
staticvoidvac_truncate_clog(TransactionIdfrozenXID,
7474
MultiXactIdminMulti,
7575
TransactionIdlastSaneFrozenXid,
@@ -257,15 +257,15 @@ vacuum(int options, List *relations, VacuumParams *params,
257257
List*sublist;
258258
MemoryContextold_context;
259259

260-
sublist=expand_vacuum_rel(vrel);
260+
sublist=expand_vacuum_rel(vrel,options);
261261
old_context=MemoryContextSwitchTo(vac_context);
262262
newrels=list_concat(newrels,sublist);
263263
MemoryContextSwitchTo(old_context);
264264
}
265265
relations=newrels;
266266
}
267267
else
268-
relations=get_all_vacuum_rels();
268+
relations=get_all_vacuum_rels(options);
269269

270270
/*
271271
* Decide whether we need to start/commit our own transactions.
@@ -408,6 +408,80 @@ vacuum(int options, List *relations, VacuumParams *params,
408408
vac_context=NULL;
409409
}
410410

411+
/*
412+
* Check if a given relation can be safely vacuumed or analyzed. If the
413+
* user is not the relation owner, issue a WARNING log message and return
414+
* false to let the caller decide what to do with this relation. This
415+
* routine is used to decide if a relation can be processed for VACUUM or
416+
* ANALYZE.
417+
*/
418+
bool
419+
vacuum_is_relation_owner(Oidrelid,Form_pg_classreltuple,intoptions)
420+
{
421+
char*relname;
422+
423+
Assert((options& (VACOPT_VACUUM |VACOPT_ANALYZE))!=0);
424+
425+
/*
426+
* Check permissions.
427+
*
428+
* We allow the user to vacuum or analyze a table if he is superuser, the
429+
* table owner, or the database owner (but in the latter case, only if
430+
* it's not a shared relation). pg_class_ownercheck includes the
431+
* superuser case.
432+
*
433+
* Note we choose to treat permissions failure as a WARNING and keep
434+
* trying to vacuum or analyze the rest of the DB --- is this appropriate?
435+
*/
436+
if (pg_class_ownercheck(relid,GetUserId())||
437+
(pg_database_ownercheck(MyDatabaseId,GetUserId())&& !reltuple->relisshared))
438+
return true;
439+
440+
relname=NameStr(reltuple->relname);
441+
442+
if ((options&VACOPT_VACUUM)!=0)
443+
{
444+
if (reltuple->relisshared)
445+
ereport(WARNING,
446+
(errmsg("skipping \"%s\" --- only superuser can vacuum it",
447+
relname)));
448+
elseif (reltuple->relnamespace==PG_CATALOG_NAMESPACE)
449+
ereport(WARNING,
450+
(errmsg("skipping \"%s\" --- only superuser or database owner can vacuum it",
451+
relname)));
452+
else
453+
ereport(WARNING,
454+
(errmsg("skipping \"%s\" --- only table or database owner can vacuum it",
455+
relname)));
456+
457+
/*
458+
* For VACUUM ANALYZE, both logs could show up, but just generate
459+
* information for VACUUM as that would be the first one to be
460+
* processed.
461+
*/
462+
return false;
463+
}
464+
465+
if ((options&VACOPT_ANALYZE)!=0)
466+
{
467+
if (reltuple->relisshared)
468+
ereport(WARNING,
469+
(errmsg("skipping \"%s\" --- only superuser can analyze it",
470+
relname)));
471+
elseif (reltuple->relnamespace==PG_CATALOG_NAMESPACE)
472+
ereport(WARNING,
473+
(errmsg("skipping \"%s\" --- only superuser or database owner can analyze it",
474+
relname)));
475+
else
476+
ereport(WARNING,
477+
(errmsg("skipping \"%s\" --- only table or database owner can analyze it",
478+
relname)));
479+
}
480+
481+
return false;
482+
}
483+
484+
411485
/*
412486
* Given a VacuumRelation, fill in the table OID if it wasn't specified,
413487
* and optionally add VacuumRelations for partitions of the table.
@@ -423,7 +497,7 @@ vacuum(int options, List *relations, VacuumParams *params,
423497
* are made in vac_context.
424498
*/
425499
staticList*
426-
expand_vacuum_rel(VacuumRelation*vrel)
500+
expand_vacuum_rel(VacuumRelation*vrel,intoptions)
427501
{
428502
List*vacrels=NIL;
429503
MemoryContextoldcontext;
@@ -457,22 +531,28 @@ expand_vacuum_rel(VacuumRelation *vrel)
457531
relid=RangeVarGetRelid(vrel->relation,AccessShareLock, false);
458532

459533
/*
460-
* Make a returnable VacuumRelation for this rel.
461-
*/
462-
oldcontext=MemoryContextSwitchTo(vac_context);
463-
vacrels=lappend(vacrels,makeVacuumRelation(vrel->relation,
464-
relid,
465-
vrel->va_cols));
466-
MemoryContextSwitchTo(oldcontext);
467-
468-
/*
469-
* To check whether the relation is a partitioned table, fetch its
470-
* syscache entry.
534+
* To check whether the relation is a partitioned table and its
535+
* ownership, fetch its syscache entry.
471536
*/
472537
tuple=SearchSysCache1(RELOID,ObjectIdGetDatum(relid));
473538
if (!HeapTupleIsValid(tuple))
474539
elog(ERROR,"cache lookup failed for relation %u",relid);
475540
classForm= (Form_pg_class)GETSTRUCT(tuple);
541+
542+
/*
543+
* Make a returnable VacuumRelation for this rel if user is a proper
544+
* owner.
545+
*/
546+
if (vacuum_is_relation_owner(relid,classForm,options))
547+
{
548+
oldcontext=MemoryContextSwitchTo(vac_context);
549+
vacrels=lappend(vacrels,makeVacuumRelation(vrel->relation,
550+
relid,
551+
vrel->va_cols));
552+
MemoryContextSwitchTo(oldcontext);
553+
}
554+
555+
476556
include_parts= (classForm->relkind==RELKIND_PARTITIONED_TABLE);
477557
ReleaseSysCache(tuple);
478558

@@ -481,7 +561,9 @@ expand_vacuum_rel(VacuumRelation *vrel)
481561
* the list returned by find_all_inheritors() includes the passed-in
482562
* OID, so we have to skip that. There's no point in taking locks on
483563
* the individual partitions yet, and doing so would just add
484-
* unnecessary deadlock risk.
564+
* unnecessary deadlock risk. For this last reason we do not check
565+
* yet the ownership of the partitions, which get added to the list to
566+
* process. Ownership will be checked later on anyway.
485567
*/
486568
if (include_parts)
487569
{
@@ -530,7 +612,7 @@ expand_vacuum_rel(VacuumRelation *vrel)
530612
* the current database. The list is built in vac_context.
531613
*/
532614
staticList*
533-
get_all_vacuum_rels(void)
615+
get_all_vacuum_rels(intoptions)
534616
{
535617
List*vacrels=NIL;
536618
Relationpgclass;
@@ -545,6 +627,11 @@ get_all_vacuum_rels(void)
545627
{
546628
Form_pg_classclassForm= (Form_pg_class)GETSTRUCT(tuple);
547629
MemoryContextoldcontext;
630+
Oidrelid=HeapTupleGetOid(tuple);
631+
632+
/* check permissions of relation */
633+
if (!vacuum_is_relation_owner(relid,classForm,options))
634+
continue;
548635

549636
/*
550637
* We include partitioned tables here; depending on which operation is
@@ -563,7 +650,7 @@ get_all_vacuum_rels(void)
563650
*/
564651
oldcontext=MemoryContextSwitchTo(vac_context);
565652
vacrels=lappend(vacrels,makeVacuumRelation(NULL,
566-
HeapTupleGetOid(tuple),
653+
relid,
567654
NIL));
568655
MemoryContextSwitchTo(oldcontext);
569656
}
@@ -1436,30 +1523,17 @@ vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params)
14361523
}
14371524

14381525
/*
1439-
* Check permissions.
1440-
*
1441-
* We allow the user to vacuum a table if he is superuser, the table
1442-
* owner, or the database owner (but in the latter case, only if it's not
1443-
* a shared relation). pg_class_ownercheck includes the superuser case.
1444-
*
1445-
* Note we choose to treat permissions failure as a WARNING and keep
1446-
* trying to vacuum the rest of the DB --- is this appropriate?
1526+
* Check if relation needs to be skipped based on ownership. This check
1527+
* happens also when building the relation list to vacuum for a manual
1528+
* operation, and needs to be done additionally here as VACUUM could
1529+
* happen across multiple transactions where relation ownership could have
1530+
* changed in-between. Make sure to only generate logs for VACUUM in this
1531+
* case.
14471532
*/
1448-
if (!(pg_class_ownercheck(RelationGetRelid(onerel),GetUserId())||
1449-
(pg_database_ownercheck(MyDatabaseId,GetUserId())&& !onerel->rd_rel->relisshared)))
1533+
if (!vacuum_is_relation_owner(RelationGetRelid(onerel),
1534+
onerel->rd_rel,
1535+
options&VACOPT_VACUUM))
14501536
{
1451-
if (onerel->rd_rel->relisshared)
1452-
ereport(WARNING,
1453-
(errmsg("skipping \"%s\" --- only superuser can vacuum it",
1454-
RelationGetRelationName(onerel))));
1455-
elseif (onerel->rd_rel->relnamespace==PG_CATALOG_NAMESPACE)
1456-
ereport(WARNING,
1457-
(errmsg("skipping \"%s\" --- only superuser or database owner can vacuum it",
1458-
RelationGetRelationName(onerel))));
1459-
else
1460-
ereport(WARNING,
1461-
(errmsg("skipping \"%s\" --- only table or database owner can vacuum it",
1462-
RelationGetRelationName(onerel))));
14631537
relation_close(onerel,lmode);
14641538
PopActiveSnapshot();
14651539
CommitTransactionCommand();

‎src/include/commands/vacuum.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#defineVACUUM_H
1616

1717
#include"access/htup.h"
18+
#include"catalog/pg_class.h"
1819
#include"catalog/pg_statistic.h"
1920
#include"catalog/pg_type.h"
2021
#include"nodes/parsenodes.h"
@@ -185,6 +186,8 @@ extern void vacuum_set_xid_limits(Relation rel,
185186
MultiXactId*mxactFullScanLimit);
186187
externvoidvac_update_datfrozenxid(void);
187188
externvoidvacuum_delay_point(void);
189+
externboolvacuum_is_relation_owner(Oidrelid,Form_pg_classreltuple,
190+
intoptions);
188191

189192
/* in commands/vacuumlazy.c */
190193
externvoidlazy_vacuum_rel(Relationonerel,intoptions,

0 commit comments

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