Improve behavior of concurrent ANALYZE/VACUUM

Michael Paquier <michael@paquier.xyz>

From: Michael Paquier <michael@paquier.xyz>
To: Postgres hackers <pgsql-hackers@postgresql.org>
Cc: bossartn@amazon.com, horiguchi.kyotaro@lab.ntt.co.jp
Date: 2018-08-12T22:21:42Z
Lists: pgsql-hackers

Attachments

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

Commits

  1. Improve VACUUM and ANALYZE by avoiding early lock queue

  2. Add more tests for VACUUM skips with partitioned tables

  3. Add regression tests for VACUUM and ANALYZE with relation skips