Re: [HACKERS] forcing a rebuild of the visibility map

[Robert Haas]

On Mon, Jun 20, 2016 at 4:57 PM, Tom Lane  wrote:
> Andres Freund  writes:
>> I also don't see why it's a good idea to have knowledge about how to
>> truncate the visibility map outside of visibilitymap.c. Having that in a
>> contrib module just seems like a modularity violation.
>
> That seems like a pretty good argument.

I agree that's a good argument but it passes over one or two points
which motivated my approach.  Most of the work of truncating the
visibility map is, in fact, encapsulated by visibilitymap_truncate, so
the only knowledge we're exporting to the contrib module is what WAL
record to emit.  I put that in the caller of visibilitymap_truncate
because the only existing caller of visibilitymap_truncate is
RelationTruncate, which also knows what WAL record to emit on behalf
of visibilitymap.c.  So I don't think I've exported any more knowledge
from visibilitymap.c than was the case previously.

That having been said, if somebody wants to whack this around, I am
not going to put up a big fight.

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


-- 
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] forcing a rebuild of the visibility map

[Tom Lane]

Andres Freund  writes:
> I also don't see why it's a good idea to have knowledge about how to
> truncate the visibility map outside of visibilitymap.c. Having that in a
> contrib module just seems like a modularity violation.

That seems like a pretty good argument.

regards, tom lane


-- 
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] forcing a rebuild of the visibility map

[Andres Freund]

On 2016-06-18 11:56:51 -0400, Tom Lane wrote:
> Michael Paquier  writes:
> > On Sat, Jun 18, 2016 at 6:53 AM, Robert Haas  wrote:
> >> Andres, do you want to explain the nature of your concern further?
> 
> > I am not in his mind, but my guess is that contrib modules are
> > sometimes used as template examples by other people, and encouraging
> > users to use those routines in modules would increase the risk to
> > misuse them, aka badly-formed records that could corrupt the system.

That's not it, no.


> If Andres' concern is that XLogInsert isn't a very stable API, maybe
> we could address that by providing some wrapper function that knows
> how to emit the specific kind of record that pg_visibility needs.

That's part of the concern I have, yes. It's pretty common that during
minor version updates contrib modules are updated before the main server
is restarted. Increasing the coupling on something as critical as WAL
logging doesn't strike me as a good idea.

I also don't see why it's a good idea to have knowledge about how to
truncate the visibility map outside of visibilitymap.c. Having that in a
contrib module just seems like a modularity violation.  To me this
should be a wal_log paramter to visibilitymap_truncate().


Andres


-- 
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] forcing a rebuild of the visibility map

[Tom Lane]

Michael Paquier  writes:
> On Sat, Jun 18, 2016 at 6:53 AM, Robert Haas  wrote:
>> Andres, do you want to explain the nature of your concern further?

> I am not in his mind, but my guess is that contrib modules are
> sometimes used as template examples by other people, and encouraging
> users to use those routines in modules would increase the risk to
> misuse them, aka badly-formed records that could corrupt the system.

I don't follow that argument.  People writing new extensions are just
as likely to copy from core code as contrib.

If Andres' concern is that XLogInsert isn't a very stable API, maybe
we could address that by providing some wrapper function that knows
how to emit the specific kind of record that pg_visibility needs.
But on the whole it seems like make-work, unless there's a reason
to believe that other extensions will need to generate that exact
same record type.

regards, tom lane


-- 
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] forcing a rebuild of the visibility map

[Michael Paquier]

On Sat, Jun 18, 2016 at 6:53 AM, Robert Haas  wrote:
> On Fri, Jun 17, 2016 at 2:59 PM, Robert Haas  wrote:
>>> Having an XLogInsert() in contrib makes me more than a bit squeamish.  I
>>> think it'd be fair bit better to have that section of code in
>>> visibilitymap.c, and then call that from the extension.
>>
>> I thought about that too, but it seemed like it was just bloating the
>> core server for no real reason.  It's not like contrib is off in space
>> someplace.
>
> So, Andres thinks it's a bad idea to have an XLogInsert() call in
> contrib, and I don't think it matters.  Anyone else have an opinion?

I am meh as well with this practice that we should not encourage. xlog
insertion routines should just be generated by in-core code. We have
now the generic WAL interface for extensions, and if need be it should
be extended.

> Andres, do you want to explain the nature of your concern further?

I am not in his mind, but my guess is that contrib modules are
sometimes used as template examples by other people, and encouraging
users to use those routines in modules would increase the risk to
misuse them, aka badly-formed records that could corrupt the system.
-- 
Michael


-- 
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] forcing a rebuild of the visibility map

[Robert Haas]

Since time is short and it seems better to get the xlog page magic
bump done before beta2, I've gone ahead and committed this, but there
are two points here that seem to warrant some input from other senior
hackers.  Andres and I have discussed these points off list but
without reaching a meeting of the minds.

On Fri, Jun 17, 2016 at 2:59 PM, Robert Haas  wrote:
>> Having an XLogInsert() in contrib makes me more than a bit squeamish.  I
>> think it'd be fair bit better to have that section of code in
>> visibilitymap.c, and then call that from the extension.
>
> I thought about that too, but it seemed like it was just bloating the
> core server for no real reason.  It's not like contrib is off in space
> someplace.

So, Andres thinks it's a bad idea to have an XLogInsert() call in
contrib, and I don't think it matters.  Anyone else have an opinion?
Andres, do you want to explain the nature of your concern further?

>>> + /* Don't keep the relation locked any longer than necessary! */
>>> + relation_close(rel, AccessExclusiveLock);
>>
>> Don't think that's a good idea. We use transactional semantics for a
>> good reason, and the function returns afterwards anyway.
>
> Yeah, but if you want to blow away a bunch of visibility map forks at
> one go, you're not going to appreciate this function collecting all of
> those locks at the same time.  This is also why VACUUM starts a
> separate transaction for each table.

Andres has a concern here that there might be a problem related to
invalidation messages.  Currently, the only invalidation message
that's getting generated here is a non-transactional smgr
invalidation, which is sent before the lock is released and therefore
no race exists.  However, Andres is concerned that this may at a
minimum not be very future-proof and possibly unsafe even today.  To
me, however, this doesn't seem particularly different than what e.g.
lazy_truncate_heap() always does and has done for a long time.  More
input here is welcome and appreciated - it would be good not to screw
this up.

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


-- 
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] forcing a rebuild of the visibility map

[Robert Haas]

On Fri, Jun 17, 2016 at 2:48 PM, Andres Freund  wrote:
>> From 010e99b403ec733d50c71a7d4ef646b1b446ef07 Mon Sep 17 00:00:00 2001
>> From: Robert Haas 
>> Date: Wed, 15 Jun 2016 22:52:58 -0400
>> Subject: [PATCH 2/2] Add VACUUM (DISABLE_PAGE_SKIPPING) for emergencies.
>
>>Furthermore,
>> +  except when performing an aggressive vacuum, some pages may be skipped
>> +  in order to avoid waiting for other sessions to finish using them.
>
> Isn't that still the case? We'll not wait for a cleanup lock and such,
> if not necessary?

That's just there to explain what behavior gets changed when you
specify DISABLE_PAGE_SKIPPING.

>>  /* non-export function prototypes */
>> -static void lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
>> -Relation *Irel, int nindexes, bool aggressive);
>> +static void lazy_scan_heap(Relation onerel, int options,
>> +LVRelStats *vacrelstats, Relation *Irel, int 
>> nindexes,
>> +bool aggressive);
>
> Generally I think it's better to pass bitmaks arguments as an unsigned
> integer. But since other related routines already use int...

Many years ago, I was told by Tom Lane that project standard was int.
My gut was to use unsigned too, but I've got better things to do than
argue about it - and project style is a legitimate argument in any
case.

>>   /*
>> -  * We request an aggressive scan if either the table's frozen Xid is 
>> now
>> +  * We request an aggressive scan if the table's frozen Xid is now
>>* older than or equal to the requested Xid full-table scan limit; or 
>> if
>>* the table's minimum MultiXactId is older than or equal to the 
>> requested
>> -  * mxid full-table scan limit.
>> +  * mxid full-table scan limit; or if DISABLE_PAGE_SKIPPING was 
>> specified.
>>*/
>>   aggressive = 
>> TransactionIdPrecedesOrEquals(onerel->rd_rel->relfrozenxid,
>>  
>> xidFullScanLimit);
>>   aggressive |= MultiXactIdPrecedesOrEquals(onerel->rd_rel->relminmxid,
>>  
>>mxactFullScanLimit);
>> + if (options & VACOPT_DISABLE_PAGE_SKIPPING)
>> + aggressive = true;
>
> I'm inclined to convert the agressive |= to an if, it looks a bit
> jarring if consecutive assignments use differing forms.

I thought about that, but I like it better the way I have it.

> This code really makes me unhappy, everytime I see it.

That's not a defect in this patch.

>> + | IDENT
>> + {
>> + if (strcmp($1, 
>> "disable_page_skipping") == 0)
>> + $$ = 
>> VACOPT_DISABLE_PAGE_SKIPPING;
>> + else
>> + ereport(ERROR,
>> + 
>> (errcode(ERRCODE_SYNTAX_ERROR),
>> +  errmsg("unrecognized 
>> VACUUM option \"%s\"", $1),
>> +  
>> parser_errposition(@1)));
>> + }
>>   ;
>
> If we could get rid of that indentation behaviour by pgindent, I'd be
> eclectic.

Neither is that, although the indentation there looks fine to me.

>>  /*
>> + * Remove the visibility map fork for a relation.  If there turn out to be
>> + * any bugs in the visibility map code that require rebuilding the VM, this
>> + * provides users with a way to do it that is cleaner than shutting down the
>> + * server and removing files by hand.
>> + *
>> + * This is a cut-down version of RelationTruncate.
>> + */
>> +Datum
>> +pg_truncate_visibility_map(PG_FUNCTION_ARGS)
>> +{
>> + Oid relid = PG_GETARG_OID(0);
>> + Relationrel;
>> +
>> + rel = relation_open(relid, AccessExclusiveLock);
>> +
>> + if (rel->rd_rel->relkind != RELKIND_RELATION &&
>> + rel->rd_rel->relkind != RELKIND_MATVIEW &&
>> + rel->rd_rel->relkind != RELKIND_TOASTVALUE)
>> + ereport(ERROR,
>> + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>> +errmsg("\"%s\" is not a table, materialized view, or TOAST 
>> table",
>> +   RelationGetRelationName(rel;
>> +
>> + RelationOpenSmgr(rel);
>> + rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
>> +
>> + visibilitymap_truncate(rel, 0);
>> +
>> + if (RelationNeedsWAL(rel))
>> + {
>> + xl_smgr_truncatexlrec;
>> +
>> + xlrec.blkno = 0;
>> + xlrec.rnode = rel->rd_node;
>> + xlrec.flags = SMGR_TRUNCATE_VM;
>> +
>> + 

Re: [HACKERS] forcing a rebuild of the visibility map

[Andres Freund]

Hi,


On 2016-06-15 23:06:37 -0400, Robert Haas wrote:
> After having been scared out of my mind by the discovery of
> longstanding breakage in heap_update[1], it occurred to me that this
> is an excellent example of a case in which the option for which Andres
> was agitating - specifically forcing VACUUM to visit ever single page
> in the heap - would be useful. [...] .  Being able
> to simply force every page to be visited is better.  Patch for that
> attached.  I decided to call the option disable_page_skipping rather
> than even_frozen_pages because it should also force visiting pages
> that are all-visible but not all-frozen.
> 
> However, I also think that the approach described above - providing a
> way to excise VM forks specifically - is useful.

I think both make sense.


> From 010e99b403ec733d50c71a7d4ef646b1b446ef07 Mon Sep 17 00:00:00 2001
> From: Robert Haas 
> Date: Wed, 15 Jun 2016 22:52:58 -0400
> Subject: [PATCH 2/2] Add VACUUM (DISABLE_PAGE_SKIPPING) for emergencies.

>Furthermore,
> +  except when performing an aggressive vacuum, some pages may be skipped
> +  in order to avoid waiting for other sessions to finish using them.

Isn't that still the case? We'll not wait for a cleanup lock and such,
if not necessary?


>  /* non-export function prototypes */
> -static void lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
> -Relation *Irel, int nindexes, bool aggressive);
> +static void lazy_scan_heap(Relation onerel, int options,
> +LVRelStats *vacrelstats, Relation *Irel, int 
> nindexes,
> +bool aggressive);

Generally I think it's better to pass bitmaks arguments as an unsigned
integer. But since other related routines already use int...


>   /*
> -  * We request an aggressive scan if either the table's frozen Xid is now
> +  * We request an aggressive scan if the table's frozen Xid is now
>* older than or equal to the requested Xid full-table scan limit; or if
>* the table's minimum MultiXactId is older than or equal to the 
> requested
> -  * mxid full-table scan limit.
> +  * mxid full-table scan limit; or if DISABLE_PAGE_SKIPPING was 
> specified.
>*/
>   aggressive = TransactionIdPrecedesOrEquals(onerel->rd_rel->relfrozenxid,
>   
>xidFullScanLimit);
>   aggressive |= MultiXactIdPrecedesOrEquals(onerel->rd_rel->relminmxid,
>   
>   mxactFullScanLimit);
> + if (options & VACOPT_DISABLE_PAGE_SKIPPING)
> + aggressive = true;

I'm inclined to convert the agressive |= to an if, it looks a bit
jarring if consecutive assignments use differing forms.


>  static void
> -lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
> +lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
>  Relation *Irel, int nindexes, bool aggressive)
>  {
>   BlockNumber nblocks,
> @@ -542,25 +545,28 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
>* the last page.  This is worth avoiding mainly because such a lock 
> must
>* be replayed on any hot standby, where it can be disruptive.
>*/
> - for (next_unskippable_block = 0;
> -  next_unskippable_block < nblocks;
> -  next_unskippable_block++)
> + next_unskippable_block = 0;
> + if ((options & VACOPT_DISABLE_PAGE_SKIPPING) == 0)
>   {
> - uint8   vmstatus;
> -
> - vmstatus = visibilitymap_get_status(onerel, 
> next_unskippable_block,
> - 
> );
> - if (aggressive)
> + while (next_unskippable_block < nblocks)
>   {
> - if ((vmstatus & VISIBILITYMAP_ALL_FROZEN) == 0)
> - break;
> - }
> - else
> - {
> - if ((vmstatus & VISIBILITYMAP_ALL_VISIBLE) == 0)
> - break;
> + uint8   vmstatus;
> +
> + vmstatus = visibilitymap_get_status(onerel, 
> next_unskippable_block,
> + 
> );
> + if (aggressive)
> + {
> + if ((vmstatus & VISIBILITYMAP_ALL_FROZEN) == 0)
> + break;
> + }
> + else
> + {
> + if ((vmstatus & VISIBILITYMAP_ALL_VISIBLE) == 0)
> + break;
> + }
> + vacuum_delay_point();
> + 

Re: [HACKERS] forcing a rebuild of the visibility map

[Masahiko Sawada]

On Thu, Jun 16, 2016 at 10:03 PM, Robert Haas  wrote:
> On Thu, Jun 16, 2016 at 2:33 AM, Masahiko Sawada  
> wrote:
>> Option name DISABLE_PAGE_SKIPPING is good to me.
>> I'm still working on this, but here are some review comments.
>>
>> ---
>> +CREATE FUNCTION pg_truncate_visibility_map(regclass)
>> +RETURNS void
>> +AS 'MODULE_PATHNAME', 'pg_truncate_visibility_map'
>> +LANGUAGE C STRICT
>> +PARALLEL UNSAFE;  -- let's not make this any more dangerous
>> +
>>
>> "REVOKE ALL ON FUNCTION pg_truncate_visibility_map(regclass) FROM
>> PUBLIC;" is missing in pg_visibility/pg_visibility--1.0--1.1.sql.
>
> OK, thanks.  I'll fix that.
>
>> I think that VACUUM with VERBOSE option can show the information for
>> how many frozen pages we skipped like autovacuum does. Thought?
>> Patch attached.
>
> I'm not sure.  The messages VACUUM emits are already quite long and
> hard to read, and adding more lines to them might make that problem
> worse.  On the other hand, having more details can be helpful, too.

Because these informations are emitted only when VERBOSE option is
specified, I think that it would be better to output that rather than
nothing, although it makes messages more complicated.

Regards,

--
Masahiko Sawada


-- 
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] forcing a rebuild of the visibility map

[Robert Haas]

On Thu, Jun 16, 2016 at 2:33 AM, Masahiko Sawada  wrote:
> Option name DISABLE_PAGE_SKIPPING is good to me.
> I'm still working on this, but here are some review comments.
>
> ---
> +CREATE FUNCTION pg_truncate_visibility_map(regclass)
> +RETURNS void
> +AS 'MODULE_PATHNAME', 'pg_truncate_visibility_map'
> +LANGUAGE C STRICT
> +PARALLEL UNSAFE;  -- let's not make this any more dangerous
> +
>
> "REVOKE ALL ON FUNCTION pg_truncate_visibility_map(regclass) FROM
> PUBLIC;" is missing in pg_visibility/pg_visibility--1.0--1.1.sql.

OK, thanks.  I'll fix that.

> I think that VACUUM with VERBOSE option can show the information for
> how many frozen pages we skipped like autovacuum does. Thought?
> Patch attached.

I'm not sure.  The messages VACUUM emits are already quite long and
hard to read, and adding more lines to them might make that problem
worse.  On the other hand, having more details can be helpful, too.

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


-- 
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] forcing a rebuild of the visibility map

[Masahiko Sawada]

On Thu, Jun 16, 2016 at 12:06 PM, Robert Haas  wrote:
> [ Changing subject line in the hopes of keeping separate issues in
> separate threads. ]
>
> On Mon, Jun 6, 2016 at 11:00 AM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> I'm intuitively sympathetic to the idea that we should have an option
>>> for this, but I can't figure out in what case we'd actually tell
>>> anyone to use it.  It would be useful for the kinds of bugs listed
>>> above to have VACUUM (rebuild_vm) to blow away the VM fork and rebuild
>>> it, but that's different semantics than what we proposed for VACUUM
>>> (even_frozen_pages).  And I'd be sort of inclined to handle that case
>>> by providing some other way to remove VM forks (like a new function in
>>> the pg_visibilitymap contrib module, maybe?) and then just tell people
>>> to run regular VACUUM afterwards, rather than putting the actual VM
>>> fork removal into VACUUM.
>>
>> There's a lot to be said for that approach.  If we do it, I'd be a bit
>> inclined to offer an option to blow away the FSM as well.
>
> After having been scared out of my mind by the discovery of
> longstanding breakage in heap_update[1], it occurred to me that this
> is an excellent example of a case in which the option for which Andres
> was agitating - specifically forcing VACUUM to visit ever single page
> in the heap - would be useful.  Even if there's functionality
> available to blow away the visibility map forks, you might not want to
> just go remove them all and re-vacuum everything, because while you
> were rebuilding the VM forks, index-only scans would stop being
> index-only, and that might cause an operational problem.  Being able
> to simply force every page to be visited is better.  Patch for that
> attached.  I decided to call the option disable_page_skipping rather
> than even_frozen_pages because it should also force visiting pages
> that are all-visible but not all-frozen.  (I was sorely tempted to go
> with the competing proposal of calling it VACUUM (THEWHOLEDAMNTHING)
> but I refrained.)
>
> However, I also think that the approach described above - providing a
> way to excise VM forks specifically - is useful.  Patch for that
> attached, too.  It turns out that can't be done without either adding
> a new WAL record type or extending an existing one; I chose to add a
> "flags" argument to XLOG_SMGR_TRUNCATE, specifying the forks to be
> truncated.  Since this will require bumping the XLOG version, if we're
> going to do it, and I think we should, it would be good to try to get
> it done before beta2 rather than closer to release.  However, I don't
> want to commit this without some review, so please review this.
> Actually, please review both patches.[2]  The same core support could
> be used to add an option to truncate the FSM, but I didn't write a
> patch for that because I'm incredibly { busy | lazy }.
>

Option name DISABLE_PAGE_SKIPPING is good to me.
I'm still working on this, but here are some review comments.

---
+CREATE FUNCTION pg_truncate_visibility_map(regclass)
+RETURNS void
+AS 'MODULE_PATHNAME', 'pg_truncate_visibility_map'
+LANGUAGE C STRICT
+PARALLEL UNSAFE;  -- let's not make this any more dangerous
+

"REVOKE ALL ON FUNCTION pg_truncate_visibility_map(regclass) FROM
PUBLIC;" is missing in pg_visibility/pg_visibility--1.0--1.1.sql.

---
I think that VACUUM with VERBOSE option can show the information for
how many frozen pages we skipped like autovacuum does. Thought?
Patch attached.

Regards,

--
Masahiko Sawada
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 7a67fa5..ddbb835 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -1329,6 +1329,8 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
 	"Skipped %u pages due to buffer pins.\n",
 	vacrelstats->pinskipped_pages),
 	 vacrelstats->pinskipped_pages);
+	appendStringInfo(, _("Skipped %u frozen pages.\n"),
+	 vacrelstats->frozenskipped_pages);
 	appendStringInfo(, ngettext("%u page is entirely empty.\n",
 	"%u pages are entirely empty.\n",
 	empty_pages),

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

[HACKERS] forcing a rebuild of the visibility map

[Robert Haas]

[ Changing subject line in the hopes of keeping separate issues in
separate threads. ]

On Mon, Jun 6, 2016 at 11:00 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> I'm intuitively sympathetic to the idea that we should have an option
>> for this, but I can't figure out in what case we'd actually tell
>> anyone to use it.  It would be useful for the kinds of bugs listed
>> above to have VACUUM (rebuild_vm) to blow away the VM fork and rebuild
>> it, but that's different semantics than what we proposed for VACUUM
>> (even_frozen_pages).  And I'd be sort of inclined to handle that case
>> by providing some other way to remove VM forks (like a new function in
>> the pg_visibilitymap contrib module, maybe?) and then just tell people
>> to run regular VACUUM afterwards, rather than putting the actual VM
>> fork removal into VACUUM.
>
> There's a lot to be said for that approach.  If we do it, I'd be a bit
> inclined to offer an option to blow away the FSM as well.

After having been scared out of my mind by the discovery of
longstanding breakage in heap_update[1], it occurred to me that this
is an excellent example of a case in which the option for which Andres
was agitating - specifically forcing VACUUM to visit ever single page
in the heap - would be useful.  Even if there's functionality
available to blow away the visibility map forks, you might not want to
just go remove them all and re-vacuum everything, because while you
were rebuilding the VM forks, index-only scans would stop being
index-only, and that might cause an operational problem.  Being able
to simply force every page to be visited is better.  Patch for that
attached.  I decided to call the option disable_page_skipping rather
than even_frozen_pages because it should also force visiting pages
that are all-visible but not all-frozen.  (I was sorely tempted to go
with the competing proposal of calling it VACUUM (THEWHOLEDAMNTHING)
but I refrained.)

However, I also think that the approach described above - providing a
way to excise VM forks specifically - is useful.  Patch for that
attached, too.  It turns out that can't be done without either adding
a new WAL record type or extending an existing one; I chose to add a
"flags" argument to XLOG_SMGR_TRUNCATE, specifying the forks to be
truncated.  Since this will require bumping the XLOG version, if we're
going to do it, and I think we should, it would be good to try to get
it done before beta2 rather than closer to release.  However, I don't
want to commit this without some review, so please review this.
Actually, please review both patches.[2]  The same core support could
be used to add an option to truncate the FSM, but I didn't write a
patch for that because I'm incredibly { busy | lazy }.

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

[1] 
https://www.postgresql.org/message-id/ca+tgmoz-1txcsxwcp3i-kmo5dnb-6p-odqw7c-n_q3pzzy1...@mail.gmail.com
[2] This request is directed at the list generally, not at any
specific individual ... well, OK, I mean you.  Yeah, you!
From 010e99b403ec733d50c71a7d4ef646b1b446ef07 Mon Sep 17 00:00:00 2001
From: Robert Haas 
Date: Wed, 15 Jun 2016 22:52:58 -0400
Subject: [PATCH 2/2] Add VACUUM (DISABLE_PAGE_SKIPPING) for emergencies.

---
 doc/src/sgml/ref/vacuum.sgml | 21 -
 src/backend/commands/vacuum.c|  9 
 src/backend/commands/vacuumlazy.c| 87 
 src/backend/parser/gram.y| 10 +
 src/include/nodes/parsenodes.h   |  3 +-
 src/test/regress/expected/vacuum.out |  1 +
 src/test/regress/sql/vacuum.sql  |  2 +
 7 files changed, 92 insertions(+), 41 deletions(-)

diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index 19fd748..dee1c5b 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  
 
-VACUUM [ ( { FULL | FREEZE | VERBOSE | ANALYZE } [, ...] ) ] [ table_name [ (column_name [, ...] ) ] ]
+VACUUM [ ( { FULL | FREEZE | VERBOSE | ANALYZE | DISABLE_PAGE_SKIPPING } [, ...] ) ] [ table_name [ (column_name [, ...] ) ] ]
 VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ table_name ]
 VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] ANALYZE [ table_name [ (column_name [, ...] ) ] ]
 
@@ -130,6 +130,25 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] ANALYZE [ 

 

+DISABLE_PAGE_SKIPPING
+
+ 
+  Normally, VACUUM will skip pages based on the visibility map.  Pages where
+  all tuples are known to be frozen can always be skipped, and those
+  where all tuples are known to be visible to all transactions may be
+  skipped except when performing an aggressive vacuum.  Furthermore,
+  except when performing an aggressive vacuum, some pages may be skipped
+  in order to avoid waiting for other sessions to finish using them.
+  This option disables all page-skipping