|From:||Michael Paquier <michael(at)paquier(dot)xyz>|
|To:||"Bossart, Nathan" <bossartn(at)amazon(dot)com>|
|Cc:||Postgres hackers <pgsql-hackers(at)postgresql(dot)org>, "horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp" <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>|
|Subject:||Re: Improve behavior of concurrent ANALYZE/VACUUM|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On Mon, Aug 20, 2018 at 08:57:00PM +0000, Bossart, Nathan wrote:
> Sorry for the delay! I looked through the latest patch.
Thanks a lot for the review!
> I like the idea of emitting a separate WARNING for each for clarity on
> what operations are being skipped. However, I think it could be a bit
> confusing with the current wording. Perhaps something like "skipping
> vacuum of..." and "skipping analyze of..." would make things clearer.
> Another thing to keep in mind is how often only one of these messages
> will apply. IIUC the vast majority of VACUUM (ANALYZE) statements
> that need to emit such log statements would emit both. Plus, while
> VACUUM (ANALYZE) is clearly documented as performing both operations,
> I can easily see the argument that users may view it as one big
> command and that emitting multiple log entries could be a confusing
> change in behavior.
> In short, my vote would be to maintain the current behavior for now
> and to bring up any logging improvements separately.
On the other hand, it would be useful for the user to know exactly what
is getting skipped. For example if VACUUM ANALYZE is used then both
operations would happen, but now the user would only know that VACUUM
has been skipped, and may miss the fact that ANALYZE was not attempted.
Let's do as you suggest at the end, aka if both VACOPT_VACUUM and
VACOPT_ANALYZE are passed down to vacuum_is_relation_owner, then only
the log for VACUUM is generated, which is consistent. Any other changes
could happen later on if necessary.
> + /*
> + * Depending on the permission checks done while building the list
> + * of relations to work on, it could be possible that the list is
> + * empty, hence do nothing in this case.
> + */
> + if (relations == NIL)
> + return;
> It might be better to let the command go through normally so that we
> don't miss any cleanup at the end (e.g. deleting vac_context).
Right, that was a bad idea. Updating datfrozenxid can actually be a
> + * Check if a given relation can be safely vacuumed or not. If the
> + * user is not the relation owner, issue a WARNING log message and return
> + * false to let the caller decide what to do with this relation.
> + */
> +vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple, int options)
> + Assert((options & (VACOPT_VACUUM | VACOPT_ANALYZE)) != 0);
> + /*
> + * Check permissions.
> + *
> + * We allow the user to vacuum a table if he is superuser, the table
> + * owner, or the database owner (but in the latter case, only if it's not
> + * a shared relation). pg_class_ownercheck includes the superuser case.
> + *
> + * Note we choose to treat permissions failure as a WARNING and keep
> + * trying to vacuum the rest of the DB --- is this appropriate?
> + */
> Do you think it's worth adding ANALYZE to these comments as well?
> + if (!(pg_class_ownercheck(relid, GetUserId()) ||
> + (pg_database_ownercheck(MyDatabaseId, GetUserId()) && !reltuple->relisshared)))
> Returning true right away when the role does have permissions might be
> a nice way to save a level of indentation.
> I think this actually changes the behavior for partitioned tables.
> Presently, we still go through and collect all the partitions in the
> vacrels list. With this change, we will skip collecting a table's
> partitions if the current role doesn't have the required permissions.
> Perhaps we should skip adding the current relation to vacrels if
> vacuum_is_relation_owner() returns false, and then we could go through
> the partitions and check permissions on those as well. Since we don't
> take any locks on the individual partitions yet, getting the relation
> name and calling pg_class_ownercheck() safely might be tricky, though.
Yes, that's actually intentional on my side as this keeps the logic more
simple, and we avoid risks around deadlocks when working on partitions.
It seems to me that it is also more intuitive to *not* scan a full
partition tree if the user does not have ownership on its root if the
relation is listed, instead of trying to scan all leafs to find perhaps
some of them. In most data models it would matter much anyway, no?
This is also more consistent with what is done for TRUNCATE where the
ACLs of the parent are considered first. The documentation also
actually mentions that:
"To vacuum a table, one must ordinarily be the table's owner or a
Perhaps we could make that clearer for partitions, with something like:
"If listed explicitly, the vacuum of a partitioned table will include
all its partitions if the user is the owner of the partitioned table."
If we don't want to change the current behavior, then one simple
solution would be close to what you mention, aka skip adding the
partitioned table to the list, include *all* the partitions in the list
as we cannot sanely check their ACLs at this stage, and rely on the
checks already happening in vacuum_rel() and analyze_rel(). This would
cause the original early lock attempts to not be solved for partitions,
which is why the approach taken in the patches makes the most sense.
I have split the patch into two parts:
- 0001 includes new tests which generate WARNING messages for VACUUM,
ANALYZE and VACUUM (ANALYZE). That's useful separately.
- 0002 is the original patch discussed here.
|Next Message||Robert Haas||2018-08-21 01:59:58||Re: ATTACH/DETACH PARTITION CONCURRENTLY|
|Previous Message||Kyotaro HORIGUCHI||2018-08-21 00:26:54||Re: Reopen logfile on SIGHUP|