On Tue, Jul 24, 2018 at 05:21:25PM +0000, Bossart, Nathan wrote:
> Here is a patch for refactoring the ereport() calls out of
> vacuum_rel() and analyze_rel(). I've kept all four possible log
> statements separated for ease of translation. I considered
> simplifying these statements by replacing "vacuum" and "analyze" with
> "processing." However, that seems like it could lead to ambiguity for
> commands like "VACUUM (ANALYZE, SKIP_LOCKED) table1 (a);" since both
> VACUUM and ANALYZE could be skipped independently. If we add
> SKIP_LOCKED to CLUSTER in the future, we will need to add two more log
> statements to this function.
+typedef enum SkippedRelStmtType
+{
+ SRS_VACUUM,
+ SRS_ANALYZE
+} SkippedRelStmtType;
Hm... I have not imagined this part but adding a new layer is sort of
ugly, and an extra one would be needed for CLUSTER as well, in which
case adding cluster-related logs into vacuum.c introduces a circular
dependency with cluster.c. What about just skipping this refactoring
and move to the point where CLUSTER also gains its log queries directly
in cluster_rel? VACUUM FULL is also not going to run for autovacuum, so
that's less confusion with IsAutoVacuumWorkerProcess().
Another thing is the set of issues discussed in
https://www.postgresql.org/message-id/20180724041403.GF4035%40paquier.xyz
which are actually going to touch the same code areas that we are going
to change here, for actually rather similar purposes related to lock
handling. My gut feeling is to get the other issue fixed first, and
then rework this patch series so as we get the behavior that we want in
both the case of the previous thread and what we want here when building
a list of relations to VACUUM. There is as well the issue where a user
does not provide a list, so that's an extra argument for fixing the
current relid fetching properly first.
--
Michael