Re: [HACKERS] Additional logging for VACUUM and ANALYZE

[Fabrízio de Royes Mello]

On Tue, Nov 7, 2017 at 1:35 PM, Bossart, Nathan  wrote:
>
> On 11/7/17, 9:13 AM, "Fabrízio Mello"  wrote:
> >> int save_nestlevel;
> >> +   boolrel_lock;
> >>
> >
> > Just remove the additional tab indentation before rel_lock variable.
>
> I've removed the extra tab in v4.
>

Great. Changed status to ready for commiter.

Regards,


--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Additional logging for VACUUM and ANALYZE

[Fabrízio Mello]

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

> int save_nestlevel;
> +   boolrel_lock;
> 

Just remove the additional tab indentation before rel_lock variable. 

The rest looks good to me.

Regards,

Fabrízio Mello


The new status of this patch is: Waiting on Author

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional logging for VACUUM and ANALYZE

[Fabrízio Mello]

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

The patch doesn't apply against master anymore:

fabrizio@macanudo:/d/postgresql (master) 
$ git apply /tmp/new_vacuum_log_messages_v2.patch
error: patch failed: doc/src/sgml/config.sgml:5899
error: doc/src/sgml/config.sgml: patch does not apply

Regards,

Fabrízio Mello

The new status of this patch is: Waiting on Author

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional logging for VACUUM and ANALYZE

[Bossart, Nathan]

On 10/5/17, 12:29 AM, "Michael Paquier"  wrote:
> On Thu, Oct 5, 2017 at 1:23 AM, Bossart, Nathan  wrote:
>> Presently, there are a few edge cases in vacuum_rel() and analyze_rel() that 
>> I
>> believe do not have sufficient logging.  This was discussed a bit in the
>> vacuum-multiple-relations thread [0], but it was ultimately decided that any
>> logging changes should be proposed separately.
>
> I think that I agree with that, especially now with VACUUM allowing
> multiple relations. The discussion then would be how much logging we
> want. WARNING looks adapted per the discussions we had on the other
> thread as manual VACUUMs can now involve much more relations, even
> with partitioned tables. More opinions would be welcome.

Thanks for taking a look.

>> and a test that exercises a bit of this functionality.
>
> My take on those test would be to not include them. This is a lot just
> to test two logging lines where the relation has been dropped.

Yeah, I'm not terribly concerned about those tests.

>> If this change were to be considered for back-patching, we would likely want 
>> to
>> also apply Michael's RangeVar fix for partitioned tables to 10 [1].  Without
>> this change, log messages for unspecified partitions will be emitted with the
>> parent's RangeVar information.
>
> Well, that's assuming that we begin logging some information for
> manual VACUUMs using the specified RangeVar, something that does not
> happen at the top of upstream REL_10_STABLE, but can happen if we were
> to include the patch you are proposing on this thread for
> REL_10_STABLE. But the latter is not going to happen. Or did you patch
> your version of v10 to do so in your stuff? For v10 the ship has
> already sailed, so I think that it would be better to just let it go,
> and rely on v11 which has added all the facility we wanted.

I agree.  I didn't mean to suggest that it should be back-patched, just
that v10 has a small quirk that needs to be handled if others feel
differently.

Nathan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Additional logging for VACUUM and ANALYZE

[Michael Paquier]

On Thu, Oct 5, 2017 at 1:23 AM, Bossart, Nathan  wrote:
> Presently, there are a few edge cases in vacuum_rel() and analyze_rel() that I
> believe do not have sufficient logging.  This was discussed a bit in the
> vacuum-multiple-relations thread [0], but it was ultimately decided that any
> logging changes should be proposed separately.

I think that I agree with that, especially now with VACUUM allowing
multiple relations. The discussion then would be how much logging we
want. WARNING looks adapted per the discussions we had on the other
thread as manual VACUUMs can now involve much more relations, even
with partitioned tables. More opinions would be welcome.

> So, the attached patch changes the existing lock contention message to be
> emitted for non-autovacuum sessions if necessary, and it adds a "skipping"
> message when a specified relation disappears before it is processed.  For
> consistency, autovacuum logs are emitted at LOG, and logs for manual commands
> are emitted at WARNING.  This patch also includes a minor documentation change

This is here:
 250ms or longer will be logged.  In addition, when this parameter is
 set to any value other than -1, a message will be
-logged if an autovacuum action is skipped due to the existence of a
-conflicting lock.  Enabling this parameter can be helpful
+logged if an autovacuum action is skipped due to a
conflicting lock or a
+concurrently dropped relation.  Enabling this parameter can be helpful
 in tracking autovacuum activity.  This parameter can only be set in
So that looks adapted to the patch.

> and a test that exercises a bit of this functionality.

My take on those test would be to not include them. This is a lot just
to test two logging lines where the relation has been dropped.

> If this change were to be considered for back-patching, we would likely want 
> to
> also apply Michael's RangeVar fix for partitioned tables to 10 [1].  Without
> this change, log messages for unspecified partitions will be emitted with the
> parent's RangeVar information.

Well, that's assuming that we begin logging some information for
manual VACUUMs using the specified RangeVar, something that does not
happen at the top of upstream REL_10_STABLE, but can happen if we were
to include the patch you are proposing on this thread for
REL_10_STABLE. But the latter is not going to happen. Or did you patch
your version of v10 to do so in your stuff? For v10 the ship has
already sailed, so I think that it would be better to just let it go,
and rely on v11 which has added all the facility we wanted.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers