BUG #14941: Vacuum crashes

On 12/01/2017 05:09 PM, lyes.amd@gmail.com wrote:

The following bug has been logged on the website:

Bug reference: 14941
Logged by: Lyes Ameddah
Email address: lyes.amd@gmail.com
PostgreSQL version: 9.6.0

The current minor version in 9.6 branch is 9.6. You're missing a year
worth of bugfixes ...

Operating system: CentOs 7
Description:

Hello,

I make a complete empty once a week in an automated way and it happens that
the vacuum is stuck on a table (perhaps another process has a lock first).

1) Completely empty what?

2) Do you mean autovacuum or manual vacuum?

3) Do you see waiting locks in pg_locks catalog while this is happening?

SELECT * FROM pg_locks WHERE NOT granted;

The behavior I would like to see is that the void ignores this table and
moves to another instead of being blocked.

I believe autovacuum should not block waiting for a heavy-weight lock on
a table since this commit that went into 9.1:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=32896c40ca766146312b28a5a0eb3f66ca0300ed

So I'm wondering what problem you're running into.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 12/1/17, 11:16 AM, "Tomas Vondra" <tomas.vondra@2ndquadrant.com> wrote:

The behavior I would like to see is that the void ignores this table and
moves to another instead of being blocked.

I believe autovacuum should not block waiting for a heavy-weight lock on
a table since this commit that went into 9.1:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=32896c40ca766146312b28a5a0eb3f66ca0300ed

So I'm wondering what problem you're running into.

It sounds like Lyes is doing a periodic database-wide VACUUM that is
getting blocked on certain relations that are already locked (perhaps
because autovacuum is already working on it). IIUC, the ask is to provide
a way to skip vacuuming relations that cannot be immediately locked.

There is already an internal flag called VACOPT_NOWAIT that gives
autovacuum this ability in certain cases (introduced in the aforementioned
commit), and I recently brought up this potential use-case as
justification for a patch [0]/messages/by-id/875815E8-7A99-4488-AA07-F254C404E2CF@amazon.com. I'd be happy to submit a patch for
providing VACOPT_NOWAIT to users if others think it's something we should
do.

As a side note, this seems more like a feature request than a bug report,
so I'm adding pgsql-hackers@ here, too.

Nathan

[0]: /messages/by-id/875815E8-7A99-4488-AA07-F254C404E2CF@amazon.com

On Fri, Dec 1, 2017 at 1:53 PM, Bossart, Nathan <bossartn@amazon.com> wrote:

There is already an internal flag called VACOPT_NOWAIT that gives
autovacuum this ability in certain cases (introduced in the aforementioned
commit), and I recently brought up this potential use-case as
justification for a patch [0]. I'd be happy to submit a patch for
providing VACOPT_NOWAIT to users if others think it's something we should
do.

Seems entirely reasonable to me, provided that we only update the
extensible-options syntax:

VACUUM [ ( option [, ...] ) ] [ table_and_columns [, ...] ]

I don't want to add any more options to the older syntax:

VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ table_and_columns [, ...] ]

I am slightly confused as to how we got on to this topic since the
subject is "Vacuum crashes", but perhaps Lyes was simply speaking
imprecisely.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 12/1/17, 3:04 PM, "Robert Haas" <robertmhaas@gmail.com> wrote:

On Fri, Dec 1, 2017 at 1:53 PM, Bossart, Nathan <bossartn@amazon.com> wrote:

There is already an internal flag called VACOPT_NOWAIT that gives
autovacuum this ability in certain cases (introduced in the aforementioned
commit), and I recently brought up this potential use-case as
justification for a patch [0]. I'd be happy to submit a patch for
providing VACOPT_NOWAIT to users if others think it's something we should
do.

Seems entirely reasonable to me, provided that we only update the
extensible-options syntax:

VACUUM [ ( option [, ...] ) ] [ table_and_columns [, ...] ]

I don't want to add any more options to the older syntax:

VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ table_and_columns [, ...] ]

Right. This seems like the right path forward to me, as the
VACUUM documentation states that the unparenthesized syntax is
deprecated, and the DISABLE_PAGE_SKIPPING option was not added
to the old syntax, either.

I am slightly confused as to how we got on to this topic since the
subject is "Vacuum crashes", but perhaps Lyes was simply speaking
imprecisely.

I'm hoping Lyes chimes in soon to clarify if I am interpreting
the original report incorrectly.

Nathan

sorry guys, yes I'm talking about a FULL VACUUM and not about Auto-Vacuum.
Thank you very match for your feedback.

That's Waht I do :
vacuum FULL VERBOSE ANALYZE;
reindex database postgres;

I would be happy if there is a patch to fix that !

2017-12-01 22:16 GMT+01:00 Bossart, Nathan <bossartn@amazon.com>:

On 12/1/17, 3:04 PM, "Robert Haas" <robertmhaas@gmail.com> wrote:

On Fri, Dec 1, 2017 at 1:53 PM, Bossart, Nathan <bossartn@amazon.com>

wrote:

There is already an internal flag called VACOPT_NOWAIT that gives
autovacuum this ability in certain cases (introduced in the

aforementioned

commit), and I recently brought up this potential use-case as
justification for a patch [0]. I'd be happy to submit a patch for
providing VACOPT_NOWAIT to users if others think it's something we

should

do.

Seems entirely reasonable to me, provided that we only update the
extensible-options syntax:

VACUUM [ ( option [, ...] ) ] [ table_and_columns [, ...] ]

I don't want to add any more options to the older syntax:

VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ table_and_columns

[, ...] ]

Right. This seems like the right path forward to me, as the
VACUUM documentation states that the unparenthesized syntax is
deprecated, and the DISABLE_PAGE_SKIPPING option was not added
to the old syntax, either.

I am slightly confused as to how we got on to this topic since the
subject is "Vacuum crashes", but perhaps Lyes was simply speaking
imprecisely.

I'm hoping Lyes chimes in soon to clarify if I am interpreting
the original report incorrectly.

Nathan

--
*Lyes AMEDDAH*
*Téléphone portable : 06 66 24 50 70*
*Titre RNCP I - Développement Web*
*HiTema*

On Mon, Dec 4, 2017 at 8:59 AM, Lyes Ameddah <lyes.amd@gmail.com> wrote:

sorry guys, yes I'm talking about a FULL VACUUM and not about Auto-Vacuum.
Thank you very match for your feedback.

OK, but that's not the confusion. What you said is that it CRASHES,
but the behavior you described is that it BLOCKS waiting for a lock.
Blocking and crashing are not the same thing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 12/4/17, 8:52 AM, "Robert Haas" <robertmhaas@gmail.com> wrote:

On Mon, Dec 4, 2017 at 8:59 AM, Lyes Ameddah <lyes.amd@gmail.com> wrote:

sorry guys, yes I'm talking about a FULL VACUUM and not about Auto-Vacuum.
Thank you very match for your feedback.

OK, but that's not the confusion. What you said is that it CRASHES,
but the behavior you described is that it BLOCKS waiting for a lock.
Blocking and crashing are not the same thing.

Provided that Lyes seems to have described wanting to avoid the blocking
behavior (and since I'm interested in adding this functionality anyway),
here are some patches that open up the VACOPT_NOWAIT option to users for
both VACUUM and ANALYZE commands.

---

0001_fix_unparenthesized_vacuum_grammar_v1.patch

One VacuumStmt grammar rule allows users to specify the VACOPT_ANALYZE
option by including an AnalyzeStmt at the end of the command. This
appears to have been added as part of the introduction of the ANALYZE
command (f905d65e). However, this means that users can call VACUUM
commands like

VACUUM VERBOSE ANALYZE VERBOSE pg_class;

Besides being disallowed according to the documentation, I think this
may give users the idea that there is a difference between the VERBOSE
options for VACUUM versus ANALYZE. In fact, they are the same option,
and specifying it twice is redundant.

This change fixes the unparenthesized VACUUM syntax to disallow the out-
of-order VACUUM options as described above. This is a prerequisite
change for opening up VACOPT_NOWAIT to users, as adding it to the
grammar as-is would cause statements like

VACUUM VERBOSE NOWAIT ANALYZE VERBOSE NOWAIT pg_class;

to be allowed. Since I am also looking to introduce a parenthesized
syntax for ANALYZE, this patch would prevent statements like

VACUUM VERBOSE ANALYZE (VERBOSE, NOWAIT) pg_class;

from being accepted as well.

---

0002_add_parenthesized_analyze_syntax_v1.patch

As a prerequisite to giving users access to the VACOPT_NOWAIT option, I
am introducing a parenthesized syntax for ANALYZE that is similar to
VACUUM's. Following VACUUM's example, any new options will be added to
this syntax, and the old style will become deprecated.

Adding a parenthesized syntax for ANALYZE is not strictly necessary for
providing the NOWAIT option, as NOWAIT is already a keyword in gram.y.
However, I thought it would be good to match VACUUM (since the command
structure is so similar), and this work would be required at some point
anyway if ANALYZE ever accepts options that we do not want to make
reserved keywords.

---

0003_add_nowait_vacuum_option_v1.patch

This change provides the existing VACOPT_NOWAIT option to users. This
option was previously only used by autovacuum in special cases, but it
seems like a potentially valuable option in other cases as well. For
example, perhaps a user wants to run a nightly VACUUM job that skips
tables that cannot be locked due to autovacuum (or something else)
already working on it.

I chose to use NOWAIT as the option name because it is already a keyword
for the LOCK command.

This patch follows the existing logging precedent added by 11d8d72c and
ab6eaee8: if a relation is explicitly specified in the command, a log
statement will be emitted if it is skipped. If the relation is not
specified (e.g. "VACUUM FULL;"), no such log statements will be emitted.

---

I'll be adding an entry to the next commitfest for these patches soon.

Nathan

On Sat, Dec 2, 2017 at 6:16 AM, Bossart, Nathan <bossartn@amazon.com> wrote:

On 12/1/17, 3:04 PM, "Robert Haas" <robertmhaas@gmail.com> wrote:

Seems entirely reasonable to me, provided that we only update the
extensible-options syntax:

VACUUM [ ( option [, ...] ) ] [ table_and_columns [, ...] ]

I don't want to add any more options to the older syntax:

VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ table_and_columns [, ...] ]

Right. This seems like the right path forward to me, as the
VACUUM documentation states that the unparenthesized syntax is
deprecated, and the DISABLE_PAGE_SKIPPING option was not added
to the old syntax, either.

Yeah, the exact same discussion has happened when what has become
DISABLE_PAGE_SKIPPING was discussed for the freeze map.
--
Michael

On Wed, Dec 6, 2017 at 1:52 AM, Bossart, Nathan <bossartn@amazon.com> wrote:

0001_fix_unparenthesized_vacuum_grammar_v1.patch

One VacuumStmt grammar rule allows users to specify the VACOPT_ANALYZE
option by including an AnalyzeStmt at the end of the command. This
appears to have been added as part of the introduction of the ANALYZE
command (f905d65e). However, this means that users can call VACUUM
commands like

VACUUM VERBOSE ANALYZE VERBOSE pg_class;

Besides being disallowed according to the documentation, I think this
may give users the idea that there is a difference between the VERBOSE
options for VACUUM versus ANALYZE. In fact, they are the same option,
and specifying it twice is redundant.

Hmm. Yeah. If adding a parenthesis grammar to ANALYZE this is a good
move for long purposes. If we don't do that now, or at least for the
purpose of this patch set, then a AnalyzeStmt node embedded directly
in the grammar of VACUUM is likely to bite in the future.

This change fixes the unparenthesized VACUUM syntax to disallow the out-
of-order VACUUM options as described above. This is a prerequisite
change for opening up VACOPT_NOWAIT to users, as adding it to the
grammar as-is would cause statements like

VACUUM VERBOSE NOWAIT ANALYZE VERBOSE NOWAIT pg_class;

to be allowed. Since I am also looking to introduce a parenthesized
syntax for ANALYZE, this patch would prevent statements like

If you add only a parenthesized grammar of ANALYZE, it seems to me
that this would not be an allowed query anyway.

VACUUM VERBOSE ANALYZE (VERBOSE, NOWAIT) pg_class;

from being accepted as well.

This could also be equivalent to ANALYZE(VERBOSE, NOWAIT). I can see
pros and cons by keeping the complete extension of AnalyzeStmt in the
VACUUM grammar, but as the documentation does not mention VACUUM
VERBOSE ANALYZE VERBOSE as a legit query I guess that ripping it out
is not going to annoy many users. Still let's see opinions from other
folks on -hackers as another approach to the feature you want to add
here would be to just implement the grammar for VACUUM but *not*
ANALYZE, but I'd like to think that we still want ANALYZE to be
supported, and also we want it to be extensible as a different thing
than what VACUUM is.

0002_add_parenthesized_analyze_syntax_v1.patch

As a prerequisite to giving users access to the VACOPT_NOWAIT option, I
am introducing a parenthesized syntax for ANALYZE that is similar to
VACUUM's. Following VACUUM's example, any new options will be added to
this syntax, and the old style will become deprecated.

Adding a parenthesized syntax for ANALYZE is not strictly necessary for
providing the NOWAIT option, as NOWAIT is already a keyword in gram.y.
However, I thought it would be good to match VACUUM (since the command
structure is so similar), and this work would be required at some point
anyway if ANALYZE ever accepts options that we do not want to make
reserved keywords.

Yep, agreed with the contents of this patch.

0003_add_nowait_vacuum_option_v1.patch

This change provides the existing VACOPT_NOWAIT option to users. This
option was previously only used by autovacuum in special cases, but it
seems like a potentially valuable option in other cases as well. For
example, perhaps a user wants to run a nightly VACUUM job that skips
tables that cannot be locked due to autovacuum (or something else)
already working on it.

I chose to use NOWAIT as the option name because it is already a keyword
for the LOCK command.

Makes sense to me.

This patch follows the existing logging precedent added by 11d8d72c and
ab6eaee8: if a relation is explicitly specified in the command, a log
statement will be emitted if it is skipped. If the relation is not
specified (e.g. "VACUUM FULL;"), no such log statements will be emitted.

+WARNING:  skipping analyze of "test1" --- lock not available
+step analyze_specified: ANALYZE (NOWAIT) test1, test2;
+step commit:
+   COMMIT;
OK for a WARNING for me in this case. We already do that for the other entries.

Let's see what other folks think first about the ANALYZE grammar in
VACUUM, as well as having ANALYZE use a parenthesized grammer. FWIW, I
like the contents of this patch set, and the thing is non-intrusive. I
think that NOWAIT gains more value now that multiple relations can be
vacuumed or analyzed with a single manual command.
--
Michael

On Mon, Dec 11, 2017 at 2:13 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Wed, Dec 6, 2017 at 1:52 AM, Bossart, Nathan <bossartn@amazon.com> wrote:

0001_fix_unparenthesized_vacuum_grammar_v1.patch

One VacuumStmt grammar rule allows users to specify the VACOPT_ANALYZE
option by including an AnalyzeStmt at the end of the command. This
appears to have been added as part of the introduction of the ANALYZE
command (f905d65e). However, this means that users can call VACUUM
commands like

VACUUM VERBOSE ANALYZE VERBOSE pg_class;

Besides being disallowed according to the documentation, I think this
may give users the idea that there is a difference between the VERBOSE
options for VACUUM versus ANALYZE. In fact, they are the same option,
and specifying it twice is redundant.

Hmm. Yeah. If adding a parenthesis grammar to ANALYZE this is a good
move for long purposes. If we don't do that now, or at least for the
purpose of this patch set, then a AnalyzeStmt node embedded directly
in the grammar of VACUUM is likely to bite in the future.

This change fixes the unparenthesized VACUUM syntax to disallow the out-
of-order VACUUM options as described above. This is a prerequisite
change for opening up VACOPT_NOWAIT to users, as adding it to the
grammar as-is would cause statements like

VACUUM VERBOSE NOWAIT ANALYZE VERBOSE NOWAIT pg_class;

to be allowed. Since I am also looking to introduce a parenthesized
syntax for ANALYZE, this patch would prevent statements like

If you add only a parenthesized grammar of ANALYZE, it seems to me
that this would not be an allowed query anyway.

VACUUM VERBOSE ANALYZE (VERBOSE, NOWAIT) pg_class;

from being accepted as well.

This could also be equivalent to ANALYZE(VERBOSE, NOWAIT). I can see
pros and cons by keeping the complete extension of AnalyzeStmt in the
VACUUM grammar, but as the documentation does not mention VACUUM
VERBOSE ANALYZE VERBOSE as a legit query I guess that ripping it out
is not going to annoy many users. Still let's see opinions from other
folks on -hackers as another approach to the feature you want to add
here would be to just implement the grammar for VACUUM but *not*
ANALYZE, but I'd like to think that we still want ANALYZE to be
supported, and also we want it to be extensible as a different thing
than what VACUUM is.

0002_add_parenthesized_analyze_syntax_v1.patch

As a prerequisite to giving users access to the VACOPT_NOWAIT option, I
am introducing a parenthesized syntax for ANALYZE that is similar to
VACUUM's. Following VACUUM's example, any new options will be added to
this syntax, and the old style will become deprecated.

Adding a parenthesized syntax for ANALYZE is not strictly necessary for
providing the NOWAIT option, as NOWAIT is already a keyword in gram.y.
However, I thought it would be good to match VACUUM (since the command
structure is so similar), and this work would be required at some point
anyway if ANALYZE ever accepts options that we do not want to make
reserved keywords.

Yep, agreed with the contents of this patch.

0003_add_nowait_vacuum_option_v1.patch

This change provides the existing VACOPT_NOWAIT option to users. This
option was previously only used by autovacuum in special cases, but it
seems like a potentially valuable option in other cases as well. For
example, perhaps a user wants to run a nightly VACUUM job that skips
tables that cannot be locked due to autovacuum (or something else)
already working on it.

I chose to use NOWAIT as the option name because it is already a keyword
for the LOCK command.

Makes sense to me.

This patch follows the existing logging precedent added by 11d8d72c and
ab6eaee8: if a relation is explicitly specified in the command, a log
statement will be emitted if it is skipped. If the relation is not
specified (e.g. "VACUUM FULL;"), no such log statements will be emitted.

+WARNING:  skipping analyze of "test1" --- lock not available
+step analyze_specified: ANALYZE (NOWAIT) test1, test2;
+step commit:
+   COMMIT;
OK for a WARNING for me in this case. We already do that for the other entries.

I also reviewed the patches.

For 0003 patch, if we do VACUUM (NOWAIT) to whole database we don't
get the above WARNING messages, but I think we should get them. The
reported issue also did VACUUM FULL and REINDEX to whole database.
Especially when VACUUM to whole database the information of skipped
relations would be useful for users.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On 12/10/17, 11:13 PM, "Michael Paquier" <michael.paquier@gmail.com> wrote:

Let's see what other folks think first about the ANALYZE grammar in
VACUUM, as well as having ANALYZE use a parenthesized grammer. FWIW, I
like the contents of this patch set, and the thing is non-intrusive. I
think that NOWAIT gains more value now that multiple relations can be
vacuumed or analyzed with a single manual command.

Thanks for taking a look.

Nathan

On 12/11/17, 9:41 PM, "Masahiko Sawada" <sawada.mshk@gmail.com> wrote:

I also reviewed the patches.

Thanks for taking a look.

For 0003 patch, if we do VACUUM (NOWAIT) to whole database we don't
get the above WARNING messages, but I think we should get them. The
reported issue also did VACUUM FULL and REINDEX to whole database.
Especially when VACUUM to whole database the information of skipped
relations would be useful for users.

Perhaps we could attempt to construct the RangeVar just before
logging in this case.

Nathan

On 12/11/17, 9:41 PM, "Masahiko Sawada" <sawada.mshk@gmail.com> wrote:

For 0003 patch, if we do VACUUM (NOWAIT) to whole database we don't
get the above WARNING messages, but I think we should get them. The
reported issue also did VACUUM FULL and REINDEX to whole database.
Especially when VACUUM to whole database the information of skipped
relations would be useful for users.

Here is a new set of patches. 0001 and 0002 are essentially the same
as before except for a rebase and some small indentation fixes. 0003
now includes logic to log all skipped relations regardless of whether
they were specified in the command. I've also modified the isolation
test slightly to use partitioned tables instead of running VACUUM and
ANALYZE on the whole database. This helps prevent timeouts on build-
farm machines with CLOBBER_CACHE_ALWAYS enabled (like with 0a3edbb3).

Nathan

On Tue, Dec 19, 2017 at 2:35 AM, Bossart, Nathan <bossartn@amazon.com> wrote:

On 12/11/17, 9:41 PM, "Masahiko Sawada" <sawada.mshk@gmail.com> wrote:

For 0003 patch, if we do VACUUM (NOWAIT) to whole database we don't
get the above WARNING messages, but I think we should get them. The
reported issue also did VACUUM FULL and REINDEX to whole database.
Especially when VACUUM to whole database the information of skipped
relations would be useful for users.

Here is a new set of patches. 0001 and 0002 are essentially the same
as before except for a rebase and some small indentation fixes. 0003
now includes logic to log all skipped relations regardless of whether
they were specified in the command. I've also modified the isolation
test slightly to use partitioned tables instead of running VACUUM and
ANALYZE on the whole database. This helps prevent timeouts on build-
farm machines with CLOBBER_CACHE_ALWAYS enabled (like with 0a3edbb3).

Thank you for updating the patches.

According to the following old comment, there might be reason why we
didn't pass the information to vacuum_rel(). But your patch fetches
the relation
name even if the "relation" is not provided. I wonder if it can be
problem in some cases.

-                * If the RangeVar is not defined, we do not have
enough information
-                * to provide a meaningful log statement.  Chances are that
-                * vacuum_rel's caller has intentionally not provided
this information
-                * so that this logging is skipped, anyway.
+                * If relname is NULL, we do not have enough
information to provide a
+                * meaningful log statement.  Chances are that this
information was
+                * intentionally not provided so that this logging is
skipped, anyway.

It would be an another discussion but autovacuum logs usually use
explicit names. Since the following two logs could be emitted by
autovacuum I wonder if we can make an explicit relation name if we're
autovacuum.

if (elevel != 0)
{
if (!rel_lock)
ereport(elevel,
(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
errmsg("skipping vacuum of \"%s\" --- lock not available",
relname)));
else
ereport(elevel,
(errcode(ERRCODE_UNDEFINED_TABLE),
errmsg("skipping vacuum of \"%s\" --- relation
no longer exists",
relname)));
}

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On 12/18/17, 3:30 PM, "Masahiko Sawada" <sawada.mshk@gmail.com> wrote:

According to the following old comment, there might be reason why we
didn't pass the information to vacuum_rel(). But your patch fetches
the relation
name even if the "relation" is not provided. I wonder if it can be
problem in some cases.

Thanks for taking another look.

I've thought through a few edge cases here, but I haven't noticed
anything that I think is a problem. If an unspecified relation is
renamed prior to get_rel_name(), we'll use the updated name, which
doesn't seem like an issue. If an unspecified relation is renamed
between get_rel_name() and the log statement, we'll use the old name,
which seems possible in the current logging logic for VACUUM/ANALYZE.
And if an unspecified relation is dropped just prior to
get_rel_name(), the result will be NULL, and the logging will be
skipped (as it already is for concurrently dropped relations that are
not specified in the command).

Nathan

On Tue, Dec 19, 2017 at 7:18 AM, Bossart, Nathan <bossartn@amazon.com> wrote:

On 12/18/17, 3:30 PM, "Masahiko Sawada" <sawada.mshk@gmail.com> wrote:

According to the following old comment, there might be reason why we
didn't pass the information to vacuum_rel(). But your patch fetches
the relation
name even if the "relation" is not provided. I wonder if it can be
problem in some cases.

Thanks for taking another look.

I've thought through a few edge cases here, but I haven't noticed
anything that I think is a problem. If an unspecified relation is
renamed prior to get_rel_name(), we'll use the updated name, which
doesn't seem like an issue. If an unspecified relation is renamed
between get_rel_name() and the log statement, we'll use the old name,
which seems possible in the current logging logic for VACUUM/ANALYZE.
And if an unspecified relation is dropped just prior to
get_rel_name(), the result will be NULL, and the logging will be
skipped (as it already is for concurrently dropped relations that are
not specified in the command).

Thank you for explanation.

There are three cases where "relation" is set NULL:
* Vacuum to whole database
* Child tables when vacuum to parent table
* TOAST tables when vacuum to normal table
As current related comment says, it would not be appropriate to
complain if we could not open such unspecified relations later. But
with you patch, we would complain it only if we could not take a lock
on these relations. It's fine with me.

On the latest patch, it looks good to me except for a following comment.

-                * If the RangeVar is not defined, we do not have
enough information
-                * to provide a meaningful log statement.  Chances are that
-                * vacuum_rel's caller has intentionally not provided
this information
-                * so that this logging is skipped, anyway.
+                * If relname is NULL, we do not have enough
information to provide a
+                * meaningful log statement.  Chances are that this
information was
+                * intentionally not provided so that this logging is
skipped, anyway.

This comment looks odd to me because we ourselves didn't set relname
just before. For example, we can move the sentence to above comment
block as follows. Thoughts?

/*
* If relation is NULL, we do not have enough information to provide a
* meaningful log statement. Chances are that this information was
* intentionally not provided so that this logging is skipped, anyway.
* However, iff VACOPT_NOWAIT is specified, we always try to emit
* a log statement even if relation is NULL.
*/

(snip)

/*
* Determine the log level.
*
* For autovacuum logs, we emit a LOG if
* log_autovacuum_min_duration is not diabled. For manual VACUUM, we
* emit a WARNING to match the log statements in the permission
* checks.
*/

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On 12/21/17, 11:07 PM, "Masahiko Sawada" <sawada.mshk@gmail.com> wrote:

On the latest patch, it looks good to me except for a following comment.

-                * If the RangeVar is not defined, we do not have
enough information
-                * to provide a meaningful log statement.  Chances are that
-                * vacuum_rel's caller has intentionally not provided
this information
-                * so that this logging is skipped, anyway.
+                * If relname is NULL, we do not have enough
information to provide a
+                * meaningful log statement.  Chances are that this
information was
+                * intentionally not provided so that this logging is
skipped, anyway.

This comment looks odd to me because we ourselves didn't set relname
just before. For example, we can move the sentence to above comment
block as follows. Thoughts?

/*
* If relation is NULL, we do not have enough information to provide a
* meaningful log statement. Chances are that this information was
* intentionally not provided so that this logging is skipped, anyway.
* However, iff VACOPT_NOWAIT is specified, we always try to emit
* a log statement even if relation is NULL.
*/

(snip)

/*
* Determine the log level.
*
* For autovacuum logs, we emit a LOG if
* log_autovacuum_min_duration is not diabled. For manual VACUUM, we
* emit a WARNING to match the log statements in the permission
* checks.
*/

I agree, this makes more sense. I've made this change in v3 of 0003.

Nathan

On Thu, Dec 28, 2017 at 10:46:18PM +0000, Bossart, Nathan wrote:

I agree, this makes more sense. I've made this change in v3 of 0003.

Based on the opinions gathered on this thread, 0001 and 0002 seem to be
in the shape wanted, and those look good for shipping. Now for 0003 we
are not there yet.

-       if (relation == NULL)
+       if (relation != NULL)
+           relname = relation->relname;
+       else if (!rel_lock)
+           relname = get_rel_name(relid);
+
+       if (relname == NULL)
I think that you are doing it wrong here. In get_all_vacuum_rels() you
should build a RangeVar to be reused in the context of this error
message, and hence you'll save an extra lookup based on the relation
OID here, saving from any timing issues that you have overseen as in
this code path a lock on the relation whose name is looked at is not
taken. Relying on the RangeVar being NULL to not generate any logs is
fine as a concept to me, but let's fill it where it is needed, and in
the case of this patch a VACUUM NOWAIT on the whole database is such a
case.
--
Michael

On Mon, Jan 8, 2018 at 11:27 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, Dec 28, 2017 at 10:46:18PM +0000, Bossart, Nathan wrote:

I agree, this makes more sense. I've made this change in v3 of 0003.

Based on the opinions gathered on this thread, 0001 and 0002 seem to be
in the shape wanted, and those look good for shipping.

Committed 0001.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company