Re: [HACKERS] Additional logging for VACUUM and ANALYZE

On 12/1/17, 7:34 AM, "Robert Haas" <robertmhaas@gmail.com> wrote:
> The general idea of this patch seems OK to me.

Thanks for the review, Robert.  I've attached a new version that
addresses your feedback.

> +    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.

Sure, that seems fine to me.

> +        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.

Yes, good catch.

> +            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.

Sure.  This is how I originally put it together, but I wasn't
sure if setting elevel to 0 was a sanctioned approach.

Nathan