Thread: pgsql: When VACUUM or ANALYZE skips a concurrently dropped table,log i

When VACUUM or ANALYZE skips a concurrently dropped table, log it.

Hopefully, the additional logging will help avoid confusion that
could otherwise result.

Nathan Bossart, reviewed by Michael Paquier, Fabrízio Mello, and me

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/ab6eaee88420db58a948849d5a735997728d73a9

Modified Files
--------------
doc/src/sgml/config.sgml                           |  4 +-
src/backend/commands/analyze.c                     | 46 +++++++++++--
src/backend/commands/vacuum.c                      | 49 ++++++++++++--
.../isolation/expected/vacuum-concurrent-drop.out  | 76 ++++++++++++++++++++++
src/test/isolation/isolation_schedule              |  1 +
.../isolation/specs/vacuum-concurrent-drop.spec    | 45 +++++++++++++
6 files changed, 208 insertions(+), 13 deletions(-)

Robert Haas <rhaas@postgresql.org> writes:
> When VACUUM or ANALYZE skips a concurrently dropped table, log it.

When this went in, I was pretty skeptical of the value of an isolation
test for it, but said nothing.  However, I now observe that the isolation
test is falling over on buildfarm machines with -DCLOBBER_CACHE_ALWAYS.
The buildfarm reports are a bit hard to interpret, but it's easy to
reproduce locally, and what I get is

$ more output_iso/regression.diffs
*** /home/postgres/pgsql/src/test/isolation/expected/vacuum-concurrent-drop.out
Mon Dec  4 17:02:55 2017
--- /home/postgres/pgsql/src/test/isolation/output_iso/results/vacuum-concurrent-drop.out       Wed Dec  6 12:07:37
2017
***************
*** 49,54 ****
--- 49,55 ----
        COMMIT;

  step analyze_all: <... completed>
+ error in steps drop_and_commit analyze_all: ERROR:  canceling statement due to user request

  starting permutation: lock vac_analyze_specified drop_and_commit
  step lock:

======================================================================

What appears to be happening is that a database-wide ANALYZE takes more
than a minute under CLOBBER_CACHE_ALWAYS, causing isolationtester.c's
hardwired one-minute timeout to trigger.

While you could imagine doing something to get around that, I do not
believe that this test is worth memorializing in perpetuity to begin
with.  I'd recommend just taking it out again.

            regards, tom lane

On 12/6/17, 11:57 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <rhaas@postgresql.org> writes:
>> When VACUUM or ANALYZE skips a concurrently dropped table, log it.
>
> When this went in, I was pretty skeptical of the value of an isolation
> test for it, but said nothing.  However, I now observe that the isolation
> test is falling over on buildfarm machines with -DCLOBBER_CACHE_ALWAYS.
> The buildfarm reports are a bit hard to interpret, but it's easy to
> reproduce locally, and what I get is
>
> $ more output_iso/regression.diffs
> *** /home/postgres/pgsql/src/test/isolation/expected/vacuum-concurrent-drop.out
> Mon Dec  4 17:02:55 2017
> --- /home/postgres/pgsql/src/test/isolation/output_iso/results/vacuum-concurrent-drop.out       Wed Dec  6 12:07:37
2017
> ***************
> *** 49,54 ****
> --- 49,55 ----
>         COMMIT;
>   
>   step analyze_all: <... completed>
> + error in steps drop_and_commit analyze_all: ERROR:  canceling statement due to user request
>   
>   starting permutation: lock vac_analyze_specified drop_and_commit
>   step lock: 
>
> ======================================================================
>
> What appears to be happening is that a database-wide ANALYZE takes more
> than a minute under CLOBBER_CACHE_ALWAYS, causing isolationtester.c's
> hardwired one-minute timeout to trigger.

Thanks for digging into this.

> While you could imagine doing something to get around that, I do not
> believe that this test is worth memorializing in perpetuity to begin
> with.  I'd recommend just taking it out again.

While the current version of the test is clearly broken, I thought
Robert made a pretty strong argument regarding the value of the test
[0].  ISTM the counter-argument is that coverage on a handful of
lines of code is not worth the extra work needed to maintain the
isolation test.  I’m not strongly opinionated either way, but I lean
towards wanting to keep the test around.

Perhaps this could be fixed by modifying the database-wide cases to
use partitioned tables instead.  The individual partitions will not
have RangeVars specified, so it would cover the case when logging
should be skipped.

Nathan

[0] https://postgr.es/m/CA+TgmobH17W=WdduhXJhxdwHAeTazNp7MDP=k0p=2w1nuSSruw@mail.gmail.com

On 12/6/17, 1:23 PM, "Bossart, Nathan" <bossartn@amazon.com> wrote:
> Perhaps this could be fixed by modifying the database-wide cases to
> use partitioned tables instead.  The individual partitions will not
> have RangeVars specified, so it would cover the case when logging
> should be skipped.

This fixed the problem for me.  I've attached a patch.

Nathan