Re: [HACKERS] Additional logging for VACUUM and ANALYZE
On 12/4/17, 2:27 PM, "Robert Haas" wrote: > Committed. Thanks! Nathan
Re: [HACKERS] Additional logging for VACUUM and ANALYZE
On Fri, Dec 1, 2017 at 4:05 PM, Bossart, Nathan wrote: > On 12/1/17, 2:03 PM, "Robert Haas" wrote: >> Thanks. I think this looks fine now, except that (1) it needs a >> pgindent run and (2) I vote for putting the test case back. Michael >> thought the test case was too much because this is so obscure, but I >> think that's exactly why it needs a test case. Otherwise, somebody a >> few years from now may not even be able to figure out how to hit this >> message, and if it gets broken, we won't know. This code seems to be >> fairly easy to break in subtle ways, so I think more test coverage is >> good. > > Makes sense. I ran pgindent and re-added the test case for v6 of the > patch. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] Additional logging for VACUUM and ANALYZE
On Fri, Dec 1, 2017 at 11:29 AM, Bossart, Nathan wrote: > Thanks for the review, Robert. I've attached a new version that > addresses your feedback. Thanks. I think this looks fine now, except that (1) it needs a pgindent run and (2) I vote for putting the test case back. Michael thought the test case was too much because this is so obscure, but I think that's exactly why it needs a test case. Otherwise, somebody a few years from now may not even be able to figure out how to hit this message, and if it gets broken, we won't know. This code seems to be fairly easy to break in subtle ways, so I think more test coverage is good. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] Additional logging for VACUUM and ANALYZE
On Thu, Nov 30, 2017 at 10:04 PM, Michael Paquier
wrote:
> On Wed, Nov 8, 2017 at 12:54 AM, FabrÃzio de Royes Mello
> 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
Re: [HACKERS] Additional logging for VACUUM and ANALYZE
On Wed, Nov 8, 2017 at 12:54 AM, FabrÃzio de Royes Mello 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. -- Michael
