From: | "Bossart, Nathan" <bossartn(at)amazon(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Fabrízio Mello <fabriziomello(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [HACKERS] Additional logging for VACUUM and ANALYZE |
Date: | 2017-12-01 16:29:06 |
Message-ID: | 16BCE92D-110C-4080-BEE7-1C4275A1E751@amazon.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 12/1/17, 7:34 AM, "Robert Haas" <robertmhaas(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
new_vacuum_log_messages_v5.patch | application/octet-stream | 4.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2017-12-01 16:37:38 | Re: Transform for pl/perl |
Previous Message | Simon Riggs | 2017-12-01 16:12:13 | Re: [HACKERS] SQL procedures |