On Thu, Nov 30, 2017 at 10:04 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Wed, Nov 8, 2017 at 12:54 AM, FabrÃzio de Royes Mello > <fabriziome...@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