On Tue, Aug 14, 2018 at 03:26:29PM +0000, Robert Haas wrote:
> I feel like you're not being very clear about exactly what this new
> approach is. Sorry if I'm being dense.
On HEAD, we check the ownership of the relation vacuumed or analyzed
after taking a lock on it in respectively vacuum_rel() and
analyze_rel(), where we already know the OID of the relation and there
may be no RangeVar which we could use with RangeVarGetRelidExtended
(like partitions). I don't think that we want to use again
RangeVarGetRelidExtended once the relation OID is known anyway. So My
proposal is to add more ownership checks when building the list of
VacuumRelations, and skip the relations the user has no right to work on
at an earlier stage. Looking at the code may be easier to understand
than a comment, please remember that there are three code paths used to
build the list of VacuumRelations (each one may be processed in its own
transaction):
1) autovacuum, which specifies only one relation at a time with its OID,
and we build the list in expand_vacuum_rel(), which finishes with a
single element.
2) Manual VACUUM with a list of relation specified, where the list of
elements is built in the second part of expand_vacuum_rel(), which is
able to expand partitioned tables as well.
3) Manual VACUUM with no list specified, where the list of relations is
built in get_all_vacuum_rels().
My proposal is to add two more ownership checks in 2) and 3), and also
refactor the code so as we use a single routine for ANALYZE and VACUUM.
This has the advantage of not making the code of ANALYZE and VACUUM
diverge anymore for the existing ownership checks, and we still generate
WARNINGs if a relation needs to be skipped.
(Thinking about it, it could make sense to add an extra assert at the
beginning of expand_vacuum_rel like I did in 6551f3d...)
--
Michael