Improve behavior of concurrent ANALYZE/VACUUM

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Postgres hackers <pgsql-hackers(at)postgresql(dot)org>
Cc: bossartn(at)amazon(dot)com, horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp
Subject: Improve behavior of concurrent ANALYZE/VACUUM
Date: 2018-08-12 22:21:42
Message-ID: 20180812222142.GA6097@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi all,
(In CC are the folks who have reviewed the first patch versions, Nathan
and Horiguchi-san.)

After TRUNCATE and REINDEX, here is the third and last thread I am
spawning for the previous thread "Canceling authentication due to
timeout aka Denial of Service Attack":
https://www.postgresql.org/message-id/152512087100.19803.12733865831237526317%40wrigleys.postgresql.org

And this time the discussion is about VACUUM/ANALYZE. In this case, we
also check relation ownership after queuing for a lock, which can allow
any user to potentially lock a relation which others could use,
particularly with VACUUM FULL which needs an AEL (access exclusive
lock).

In the previous thread, we discussed a couple of approaches, but I was
not happy with any of those, hence I have been spending more time in
getting to a solution which has no user-facing changes, and still solves
the problems folks have been complaining about, and the result is the
patch attached. The patch changes a couple of things regarding ACL
checks, by simply centralizing the ownership checks into a single
routine used by both ANALYZE and VACUUM. This routine is then used in
two more places for manual ANALYZE and VACUUM:
- When specifying directly one or more relations in the command, in
expand_vacuum_rel().
- When building the complete list of relations to work on in the case of
a database-wide operation, in get_all_vacuum_rels().

analyze_rel() and vacuum_rel() have been using the same logic to check
for relation ownership, so refactoring things into a single routine is a
win in my opinion.

While reviewing the code, I have of course noticed that analyze_rel()
makes an effort to not produce a WARNING if both VACOPT_VACUUM and
VACOPT_ANALYZE are specified in VacuumStmt->options, however we can
never see that scenario as analyze_rel() never gets called at the same
time as vacuum_rel() for a single relation.

The patch attached includes tests which I have used to also check that
correct error messages are produced for VACUUM, VACUUM ANALYZE and
ANALYZE.

Please note that like the previous one for TRUNCATE, I would no plans
for a back-patch with the same arguments as previously. There are also
serious bugs being worked on for REL_11_STABLE so I don't want to take
any risk for this branch.

Thoughts?
--
Michael

Attachment Content-Type Size
0001-Improve-VACUUM-and-ANALYZE-by-avoiding-early-lock-qu.patch text/x-diff 21.9 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2018-08-12 22:53:00 Re: [HACKERS] Optional message to user when terminating/cancelling backend
Previous Message Chapman Flack 2018-08-12 20:26:31 Re: lazy detoasting