Re: [HACKERS] Additional logging for VACUUM and ANALYZE

On Thu, Nov 30, 2017 at 10:04 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Nov 8, 2017 at 12:54 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
>> Great. Changed status to ready for commiter.
>
> The patch still applies, but no committer seem to be interested in the
> topic, so moved to next CF.

The general idea of this patch seems OK to me.

+    rel_lock = true;

I think it would look nicer to initialize this when you declare the
variable, instead of having a separate line of code for that purpose.

+        if (IsAutoVacuumWorkerProcess() && params->log_min_duration >= 0)
+            elevel = LOG;
+        else if (!IsAutoVacuumWorkerProcess())
+            elevel = WARNING;
+        else
+            return;

This can be rewritten with only one call to
IsAutoVacuumWorkerProcess() by reversing the order of the branches.

+            PopActiveSnapshot();
+            CommitTransactionCommand();
+            return false;

vacuum_rel already has too many copies of this logic -- can we try to
avoid duplicating it into two more places?  It seems like you could
easily do that like this:

int elevel = 0;
if (relation != NULL)
{
/* maybe set elevel */
}
if (elevel != 0)
{
if (!rel_lock)
   /* emit message */
else
   /* emit other message */
}

This wouldn't be the first bit of code to assume that elevel == 0 can
be used as a sentinel value meaning "none", so I think it's OK to do
that.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company