Re: Reviewing freeze map code

Hi,

The freeze map changes, besides being very important, seem to be one of
the patches with a high risk profile in 9.6. Robert had asked whether
I'd take a look. I thought it'd be a good idea to review that while
running tests for
http://www.postgresql.org/message-id/CAMkU=1w85Dqt766AUrCnyqCXfZ+rsk1witAc_=v5+Pce93Sftw@mail.gmail.com

For starters, I'm just going through the commits. It seems the relevant
pieces are:

a892234 Change the format of the VM fork to add a second bit per page.
77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.
fd31cd2 Don't vacuum all-frozen pages.
7087166 pg_upgrade: Convert old visibility map format to new format.
ba0a198 Add pg_visibility contrib module.

did I miss anything important?

Greetings,

Andres Freund

Hi,

some of the review items here are mere matters of style/preference. Feel
entirely free to discard them, but I thought if I'm going through the
change anyway...

On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
> a892234 Change the format of the VM fork to add a second bit per page.

TL;DR: fairly minor stuff.

+ * heap_tuple_needs_eventual_freeze
+ *
+ * Check to see whether any of the XID fields of a tuple (xmin, xmax, xvac)
+ * will eventually require freezing. Similar to heap_tuple_needs_freeze,
+ * but there's no cutoff, since we're trying to figure out whether freezing
+ * will ever be needed, not whether it's needed now.
+ */
+bool
+heap_tuple_needs_eventual_freeze(HeapTupleHeader tuple)

Wouldn't redefining this to heap_tuple_is_frozen() and then inverting the
checks be easier to understand?

+ /*
+ * If xmax is a valid xact or multixact, this tuple is also not frozen.
+ */
+ if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
+ {
+ MultiXactId multi;
+
+ multi = HeapTupleHeaderGetRawXmax(tuple);
+ if (MultiXactIdIsValid(multi))
+ return true;
+ }

Hm. What's the test inside the if() for? There shouldn't be any case
where xmax is invalid if HEAP_XMAX_IS_MULTI is set. Now there's a
check like that outside of this commit, but it seems strange to me
(Alvaro, perhaps you could comment on this?).

+ *
+ * Clearing both visibility map bits is not separately WAL-logged. The callers
* must make sure that whenever a bit is cleared, the bit is cleared on WAL
* replay of the updating operation as well.

I think including "both" here makes things less clear, because it
differentiates clearing one bit from clearing both. There's no practical
differentce atm, but still.

*
* VACUUM will normally skip pages for which the visibility map bit is set;
* such pages can't contain any dead tuples and therefore don't need vacuuming.
- * The visibility map is not used for anti-wraparound vacuums, because
- * an anti-wraparound vacuum needs to freeze tuples and observe the latest xid
- * present in the table, even on pages that don't have any dead tuples.
*

I think the remaining sentence isn't entirely accurate, there's now more
than one bit, and they're different with regard to scan_all/!scan_all
vacuums (or will be - maybe this updated further in a later commit? But
if so, that sentence shouldn't yet be removed...).

-
-/* Number of heap blocks we can represent in one byte. */
-#define HEAPBLOCKS_PER_BYTE 8
-

Hm, why was this moved to the header? Sounds like something the outside
shouldn't care about.

#define HEAPBLK_TO_MAPBIT(x) (((x) % HEAPBLOCKS_PER_BYTE) * BITS_PER_HEAPBLOCK)

Hm. This isn't really a mapping to an individual bit anymore - but I
don't really have a better name in mind. Maybe TO_OFFSET?

+static const uint8 number_of_ones_for_visible[256] = {
...
+};
+static const uint8 number_of_ones_for_frozen[256] = {
...
};

Did somebody verify the new contents are correct?

/*
- * visibilitymap_clear - clear a bit in visibility map
+ * visibilitymap_clear - clear all bits in visibility map
*

This seems rather easy to misunderstand, as this really only clears all
the bits for one page, not actually all the bits.

* the bit for heapBlk, or InvalidBuffer. The caller is responsible for
- * releasing *buf after it's done testing and setting bits.
+ * releasing *buf after it's done testing and setting bits, and must pass flags
+ * for which it needs to check the value in visibility map.
*
* NOTE: This function is typically called without a lock on the heap page,
* so somebody else could change the bit just after we look at it. In fact,
@@ -327,17 +351,16 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,

I'm not seing what flags the above comment change is referring to?

/*
- * A single-bit read is atomic. There could be memory-ordering effects
+ * A single byte read is atomic. There could be memory-ordering effects
* here, but for performance reasons we make it the caller's job to worry
* about that.
*/
- result = (map[mapByte] & (1 << mapBit)) ? true : false;
-
- return result;
+ return ((map[mapByte] >> mapBit) & VISIBILITYMAP_VALID_BITS);
}

Not a new issue, and *very* likely to be irrelevant in practice (given
the value is only referenced once): But there's really no guarantee
map[mapByte] is only read once here.

-BlockNumber
-visibilitymap_count(Relation rel)
+void
+visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_frozen)

Not really a new issue again: The parameter types (previously return
type) to this function seem wrong to me.

@@ -1934,5 +1992,14 @@ heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId *visibility_cut
}
+ /*
+ * We don't bother clearing *all_frozen when the page is discovered not
+ * to be all-visible, so do that now if necessary. The page might fail
+ * to be all-frozen for other reasons anyway, but if it's not all-visible,
+ * then it definitely isn't all-frozen.
+ */
+ if (!all_visible)
+ *all_frozen = false;
+

Why don't we just set *all_frozen to false when appropriate? It'd be
just as many lines and probably easier to understand?

+ /*
+ * If the page is marked as all-visible but not all-frozen, we should
+ * so mark it. Note that all_frozen is only valid if all_visible is
+ * true, so we must check both.
+ */

This kinda seems to imply that all-visible implies all_frozen. Also, why
has that block been added to the end of the if/else if chain? Seems like
it belongs below the (all_visible && !all_visible_according_to_vm) block.

Greetings,

Andres Freund

On Tue, May 3, 2016 at 6:48 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Hi,
>
> The freeze map changes, besides being very important, seem to be one of
> the patches with a high risk profile in 9.6. Robert had asked whether
> I'd take a look. I thought it'd be a good idea to review that while
> running tests for
> http://www.postgresql.org/message-id/CAMkU=1w85Dqt766AUrCnyqCXfZ+rsk1witAc_=v5+Pce93Sftw@mail.gmail.com

Thank you for reviewing.

> For starters, I'm just going through the commits. It seems the relevant
> pieces are:
>
> a892234 Change the format of the VM fork to add a second bit per page.
> 77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.
> fd31cd2 Don't vacuum all-frozen pages.
> 7087166 pg_upgrade: Convert old visibility map format to new format.
> ba0a198 Add pg_visibility contrib module.
>
> did I miss anything important?
>

That's all.

Regards,

--
Masahiko Sawada

On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
> 77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.

Nothing to say here.

> fd31cd2 Don't vacuum all-frozen pages.

Hm. I do wonder if it's going to bite us that we don't have a way to
actually force vacuuming of the whole table (besides manually rm'ing the
VM). I've more than once seen VACUUM used to try to do some integrity
checking of the database. How are we actually going to test that the
feature works correctly? They'd have to write checks ontop of
pg_visibility to see whether things are borked.

/*
* Compute whether we actually scanned the whole relation. If we did, we
* can adjust relfrozenxid and relminmxid.
*
* NB: We need to check this before truncating the relation, because that
* will change ->rel_pages.
*/

Comment is out-of-date now.

- if (blkno == next_not_all_visible_block)
+ if (blkno == next_unskippable_block)
{
- /* Time to advance next_not_all_visible_block */
- for (next_not_all_visible_block++;
- next_not_all_visible_block < nblocks;
- next_not_all_visible_block++)
+ /* Time to advance next_unskippable_block */
+ for (next_unskippable_block++;
+ next_unskippable_block < nblocks;
+ next_unskippable_block++)

Hm. So we continue with the course of re-processing pages, even if
they're marked all-frozen. For all-visible there at least can be a
benefit by freezing earlier, but for all-frozen pages there's really no
point. I don't really buy the arguments for the skipping logic. But
even disregarding that, maybe we should skip processing a block if it's
all-frozen (without preventing the page from being read?); as there's no
possible benefit? Acquring the exclusive/content lock and stuff is far
from free.

Not really related to this patch, but the FORCE_CHECK_PAGE is rather
ugly.

+ /*
+ * The current block is potentially skippable; if we've seen a
+ * long enough run of skippable blocks to justify skipping it, and
+ * we're not forced to check it, then go ahead and skip.
+ * Otherwise, the page must be at least all-visible if not
+ * all-frozen, so we can set all_visible_according_to_vm = true.
+ */
+ if (skipping_blocks && !FORCE_CHECK_PAGE())
+ {
+ /*
+ * Tricky, tricky. If this is in aggressive vacuum, the page
+ * must have been all-frozen at the time we checked whether it
+ * was skippable, but it might not be any more. We must be
+ * careful to count it as a skipped all-frozen page in that
+ * case, or else we'll think we can't update relfrozenxid and
+ * relminmxid. If it's not an aggressive vacuum, we don't
+ * know whether it was all-frozen, so we have to recheck; but
+ * in this case an approximate answer is OK.
+ */
+ if (aggressive || VM_ALL_FROZEN(onerel, blkno, &vmbuffer))
+ vacrelstats->frozenskipped_pages++;
continue;
+ }

Hm. This indeed seems a bit tricky. Not sure how to make it easier
though without just ripping out the SKIP_PAGES_THRESHOLD stuff.

Hm. This also doubles the number of VM accesses. While I guess that's
not noticeable most of the time, it's still not nice; especially when a
large relation is entirely frozen, because it'll mean we'll sequentially
go through the visibilityma twice.

I wondered for a minute whether #14057 could cause really bad issues
here
http://www.postgresql.org/message-id/20160331103739.8956.94469@wrigleys.postgresql.org
but I don't see it being more relevant here.

Andres

Hi,

On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
> 7087166 pg_upgrade: Convert old visibility map format to new format.

+const char *
+rewriteVisibilityMap(const char *fromfile, const char *tofile, bool force)
...

+ while ((bytesRead = read(src_fd, buffer, BLCKSZ)) == BLCKSZ)
+ {
..

Uh, shouldn't we actually fail if we read incompletely? Rather than
silently ignoring the problem? Ok, this causes no corruption, but it
indicates that something went significantly wrong.

+ char new_vmbuf[BLCKSZ];
+ char *new_cur = new_vmbuf;
+ bool empty = true;
+ bool old_lastpart;
+
+ /* Copy page header in advance */
+ memcpy(new_vmbuf, &pageheader, SizeOfPageHeaderData);

Shouldn't we zero out new_vmbuf? Afaics we're not necessarily zeroing it
with old_lastpart && !empty, right?

+ if ((dst_fd = open(tofile, O_RDWR | O_CREAT | (force ? 0 : O_EXCL), S_IRUSR | S_IWUSR)) < 0)
+ {
+ close(src_fd);
+ return getErrorText();
+ }

I know you guys copied this, but what's the force thing about?
Expecially as it's always set to true by the callers (i.e. what is the
parameter even about?)? Wouldn't we at least have to specify O_TRUNC in
the force case?

+ old_cur += BITS_PER_HEAPBLOCK_OLD;
+ new_cur += BITS_PER_HEAPBLOCK;

I'm not sure I'm understanding the point of the BITS_PER_HEAPBLOCK_OLD
stuff - as long as it's hardcoded into rewriteVisibilityMap() we'll not
be able to have differing ones anyway, should we decide to add a third
bit?

- Andres

On Mon, May 2, 2016 at 8:25 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> + * heap_tuple_needs_eventual_freeze
> + *
> + * Check to see whether any of the XID fields of a tuple (xmin, xmax, xvac)
> + * will eventually require freezing. Similar to heap_tuple_needs_freeze,
> + * but there's no cutoff, since we're trying to figure out whether freezing
> + * will ever be needed, not whether it's needed now.
> + */
> +bool
> +heap_tuple_needs_eventual_freeze(HeapTupleHeader tuple)
>
> Wouldn't redefining this to heap_tuple_is_frozen() and then inverting the
> checks be easier to understand?

I thought it much safer to keep this as close to a copy of
heap_tuple_needs_freeze() as possible. Copying a function and
inverting all of the return values is much more likely to introduce
bugs, IME.

> + /*
> + * If xmax is a valid xact or multixact, this tuple is also not frozen.
> + */
> + if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
> + {
> + MultiXactId multi;
> +
> + multi = HeapTupleHeaderGetRawXmax(tuple);
> + if (MultiXactIdIsValid(multi))
> + return true;
> + }
>
> Hm. What's the test inside the if() for? There shouldn't be any case
> where xmax is invalid if HEAP_XMAX_IS_MULTI is set. Now there's a
> check like that outside of this commit, but it seems strange to me
> (Alvaro, perhaps you could comment on this?).

Here again I was copying existing code, with appropriate simplifications.

> + *
> + * Clearing both visibility map bits is not separately WAL-logged. The callers
> * must make sure that whenever a bit is cleared, the bit is cleared on WAL
> * replay of the updating operation as well.
>
> I think including "both" here makes things less clear, because it
> differentiates clearing one bit from clearing both. There's no practical
> differentce atm, but still.

I agree.

> *
> * VACUUM will normally skip pages for which the visibility map bit is set;
> * such pages can't contain any dead tuples and therefore don't need vacuuming.
> - * The visibility map is not used for anti-wraparound vacuums, because
> - * an anti-wraparound vacuum needs to freeze tuples and observe the latest xid
> - * present in the table, even on pages that don't have any dead tuples.
> *
>
> I think the remaining sentence isn't entirely accurate, there's now more
> than one bit, and they're different with regard to scan_all/!scan_all
> vacuums (or will be - maybe this updated further in a later commit? But
> if so, that sentence shouldn't yet be removed...).

We can adjust the language, but I don't really see a big problem here.

> -/* Number of heap blocks we can represent in one byte. */
> -#define HEAPBLOCKS_PER_BYTE 8
> -
> Hm, why was this moved to the header? Sounds like something the outside
> shouldn't care about.

Oh... yeah. Let's undo that.

> #define HEAPBLK_TO_MAPBIT(x) (((x) % HEAPBLOCKS_PER_BYTE) * BITS_PER_HEAPBLOCK)
>
> Hm. This isn't really a mapping to an individual bit anymore - but I
> don't really have a better name in mind. Maybe TO_OFFSET?

Well, it sorta is... but we could change it, I suppose.

> +static const uint8 number_of_ones_for_visible[256] = {
> ...
> +};
> +static const uint8 number_of_ones_for_frozen[256] = {
> ...
> };
>
> Did somebody verify the new contents are correct?

I admit that I didn't. It seemed like an unlikely place for a goof,
but I guess we should verify.

> /*
> - * visibilitymap_clear - clear a bit in visibility map
> + * visibilitymap_clear - clear all bits in visibility map
> *
>
> This seems rather easy to misunderstand, as this really only clears all
> the bits for one page, not actually all the bits.

We could change "in" to "for one page in the".

> * the bit for heapBlk, or InvalidBuffer. The caller is responsible for
> - * releasing *buf after it's done testing and setting bits.
> + * releasing *buf after it's done testing and setting bits, and must pass flags
> + * for which it needs to check the value in visibility map.
> *
> * NOTE: This function is typically called without a lock on the heap page,
> * so somebody else could change the bit just after we look at it. In fact,
> @@ -327,17 +351,16 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
>
> I'm not seing what flags the above comment change is referring to?

Ugh. I think that's leftover cruft from an earlier patch version that
should have been excised from what got committed.

> /*
> - * A single-bit read is atomic. There could be memory-ordering effects
> + * A single byte read is atomic. There could be memory-ordering effects
> * here, but for performance reasons we make it the caller's job to worry
> * about that.
> */
> - result = (map[mapByte] & (1 << mapBit)) ? true : false;
> -
> - return result;
> + return ((map[mapByte] >> mapBit) & VISIBILITYMAP_VALID_BITS);
> }
>
> Not a new issue, and *very* likely to be irrelevant in practice (given
> the value is only referenced once): But there's really no guarantee
> map[mapByte] is only read once here.

Meh. But we can fix if you want to.

> -BlockNumber
> -visibilitymap_count(Relation rel)
> +void
> +visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_frozen)
>
> Not really a new issue again: The parameter types (previously return
> type) to this function seem wrong to me.

Not this patch's job to tinker.

> @@ -1934,5 +1992,14 @@ heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId *visibility_cut
> }
> + /*
> + * We don't bother clearing *all_frozen when the page is discovered not
> + * to be all-visible, so do that now if necessary. The page might fail
> + * to be all-frozen for other reasons anyway, but if it's not all-visible,
> + * then it definitely isn't all-frozen.
> + */
> + if (!all_visible)
> + *all_frozen = false;
> +
>
> Why don't we just set *all_frozen to false when appropriate? It'd be
> just as many lines and probably easier to understand?

I thought that looked really easy to mess up, either now or down the
road. This way seemed more solid to me. That's a judgement call, of
course.

> + /*
> + * If the page is marked as all-visible but not all-frozen, we should
> + * so mark it. Note that all_frozen is only valid if all_visible is
> + * true, so we must check both.
> + */
>
> This kinda seems to imply that all-visible implies all_frozen. Also, why
> has that block been added to the end of the if/else if chain? Seems like
> it belongs below the (all_visible && !all_visible_according_to_vm) block.

We can adjust the comment a bit to make it more clear, if you like,
but I doubt it's going to cause serious misunderstanding. As for the
placement, the reason I put it at the end is because I figured that we
did not want to mark it all-frozen if any of the "oh crap, emit a
warning" cases applied.

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

On Wed, May 4, 2016 at 8:08 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
>> 77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.
>
> Nothing to say here.
>
>> fd31cd2 Don't vacuum all-frozen pages.
>
> Hm. I do wonder if it's going to bite us that we don't have a way to
> actually force vacuuming of the whole table (besides manually rm'ing the
> VM). I've more than once seen VACUUM used to try to do some integrity
> checking of the database. How are we actually going to test that the
> feature works correctly? They'd have to write checks ontop of
> pg_visibility to see whether things are borked.

Let's add VACUUM (FORCE) or something like that.

> /*
> * Compute whether we actually scanned the whole relation. If we did, we
> * can adjust relfrozenxid and relminmxid.
> *
> * NB: We need to check this before truncating the relation, because that
> * will change ->rel_pages.
> */
>
> Comment is out-of-date now.

OK.

> - if (blkno == next_not_all_visible_block)
> + if (blkno == next_unskippable_block)
> {
> - /* Time to advance next_not_all_visible_block */
> - for (next_not_all_visible_block++;
> - next_not_all_visible_block < nblocks;
> - next_not_all_visible_block++)
> + /* Time to advance next_unskippable_block */
> + for (next_unskippable_block++;
> + next_unskippable_block < nblocks;
> + next_unskippable_block++)
>
> Hm. So we continue with the course of re-processing pages, even if
> they're marked all-frozen. For all-visible there at least can be a
> benefit by freezing earlier, but for all-frozen pages there's really no
> point. I don't really buy the arguments for the skipping logic. But
> even disregarding that, maybe we should skip processing a block if it's
> all-frozen (without preventing the page from being read?); as there's no
> possible benefit? Acquring the exclusive/content lock and stuff is far
> from free.

I wanted to tinker with this logic as little as possible in the
interest of ending up with something that worked. I would not have
written it this way.

> Not really related to this patch, but the FORCE_CHECK_PAGE is rather
> ugly.

+1.

> + /*
> + * The current block is potentially skippable; if we've seen a
> + * long enough run of skippable blocks to justify skipping it, and
> + * we're not forced to check it, then go ahead and skip.
> + * Otherwise, the page must be at least all-visible if not
> + * all-frozen, so we can set all_visible_according_to_vm = true.
> + */
> + if (skipping_blocks && !FORCE_CHECK_PAGE())
> + {
> + /*
> + * Tricky, tricky. If this is in aggressive vacuum, the page
> + * must have been all-frozen at the time we checked whether it
> + * was skippable, but it might not be any more. We must be
> + * careful to count it as a skipped all-frozen page in that
> + * case, or else we'll think we can't update relfrozenxid and
> + * relminmxid. If it's not an aggressive vacuum, we don't
> + * know whether it was all-frozen, so we have to recheck; but
> + * in this case an approximate answer is OK.
> + */
> + if (aggressive || VM_ALL_FROZEN(onerel, blkno, &vmbuffer))
> + vacrelstats->frozenskipped_pages++;
> continue;
> + }
>
> Hm. This indeed seems a bit tricky. Not sure how to make it easier
> though without just ripping out the SKIP_PAGES_THRESHOLD stuff.

Yep, I had the same problem.

> Hm. This also doubles the number of VM accesses. While I guess that's
> not noticeable most of the time, it's still not nice; especially when a
> large relation is entirely frozen, because it'll mean we'll sequentially
> go through the visibilityma twice.

Compared to what we're saving, that's obviously a trivial cost.
That's not to say that we might not want to improve it, but it's
hardly a disaster.

In short: wah, wah, wah.

> I wondered for a minute whether #14057 could cause really bad issues
> here
> http://www.postgresql.org/message-id/20160331103739.8956.94469@wrigleys.postgresql.org
> but I don't see it being more relevant here.

I don't really understand what the concern is here, but if it's not a
problem, let's not spend time trying to clarify.

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

On Thu, May 5, 2016 at 2:20 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
>> 7087166 pg_upgrade: Convert old visibility map format to new format.
>
> +const char *
> +rewriteVisibilityMap(const char *fromfile, const char *tofile, bool force)
> ...
>
> + while ((bytesRead = read(src_fd, buffer, BLCKSZ)) == BLCKSZ)
> + {
> ..
>
> Uh, shouldn't we actually fail if we read incompletely? Rather than
> silently ignoring the problem? Ok, this causes no corruption, but it
> indicates that something went significantly wrong.

Sure, that's reasonable.

> + char new_vmbuf[BLCKSZ];
> + char *new_cur = new_vmbuf;
> + bool empty = true;
> + bool old_lastpart;
> +
> + /* Copy page header in advance */
> + memcpy(new_vmbuf, &pageheader, SizeOfPageHeaderData);
>
> Shouldn't we zero out new_vmbuf? Afaics we're not necessarily zeroing it
> with old_lastpart && !empty, right?

Oh, dear. That seems like a possible data corruption bug. Maybe we'd
better fix that right away (although I don't actually have time before
the wrap).

> + if ((dst_fd = open(tofile, O_RDWR | O_CREAT | (force ? 0 : O_EXCL), S_IRUSR | S_IWUSR)) < 0)
> + {
> + close(src_fd);
> + return getErrorText();
> + }
>
> I know you guys copied this, but what's the force thing about?
> Expecially as it's always set to true by the callers (i.e. what is the
> parameter even about?)? Wouldn't we at least have to specify O_TRUNC in
> the force case?

I just work here.

> + old_cur += BITS_PER_HEAPBLOCK_OLD;
> + new_cur += BITS_PER_HEAPBLOCK;
>
> I'm not sure I'm understanding the point of the BITS_PER_HEAPBLOCK_OLD
> stuff - as long as it's hardcoded into rewriteVisibilityMap() we'll not
> be able to have differing ones anyway, should we decide to add a third
> bit?

I think that's just a matter of style.

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

On 05/06/2016 01:40 PM, Robert Haas wrote:
> On Wed, May 4, 2016 at 8:08 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
>>> 77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.
>>
>> Nothing to say here.
>>
>>> fd31cd2 Don't vacuum all-frozen pages.
>>
>> Hm. I do wonder if it's going to bite us that we don't have a way to
>> actually force vacuuming of the whole table (besides manually rm'ing the
>> VM). I've more than once seen VACUUM used to try to do some integrity
>> checking of the database. How are we actually going to test that the
>> feature works correctly? They'd have to write checks ontop of
>> pg_visibility to see whether things are borked.
>
> Let's add VACUUM (FORCE) or something like that.

This is actually inverted. Vacuum by default should vacuum the entire
relation, however if we are going to keep the existing behavior of this
patch, VACUUM (FROZEN) seems to be better than (FORCE)?

Sincerely,

JD

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

On 2016-05-06 13:48:09 -0700, Joshua D. Drake wrote:
> On 05/06/2016 01:40 PM, Robert Haas wrote:
> > On Wed, May 4, 2016 at 8:08 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
> > > > 77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.
> > >
> > > Nothing to say here.
> > >
> > > > fd31cd2 Don't vacuum all-frozen pages.
> > >
> > > Hm. I do wonder if it's going to bite us that we don't have a way to
> > > actually force vacuuming of the whole table (besides manually rm'ing the
> > > VM). I've more than once seen VACUUM used to try to do some integrity
> > > checking of the database. How are we actually going to test that the
> > > feature works correctly? They'd have to write checks ontop of
> > > pg_visibility to see whether things are borked.
> >
> > Let's add VACUUM (FORCE) or something like that.

Yes, that makes sense.

> This is actually inverted. Vacuum by default should vacuum the entire
> relation

What? Why on earth would that be a good idea? Not to speak of hte fact
that that's not been the case since ~8.4?

>,however if we are going to keep the existing behavior of this
> patch, VACUUM (FROZEN) seems to be better than (FORCE)?

There already is FREEZE - meaning something different - so I doubt it.

Andres

On 05/06/2016 01:50 PM, Andres Freund wrote:

>>> Let's add VACUUM (FORCE) or something like that.
>
> Yes, that makes sense.
>
>
>> This is actually inverted. Vacuum by default should vacuum the entire
>> relation
>
> What? Why on earth would that be a good idea? Not to speak of hte fact
> that that's not been the case since ~8.4?

Sorry, I just meant the default behavior shouldn't change but I do agree
that we need the ability to keep the same behavior.

>> ,however if we are going to keep the existing behavior of this
>> patch, VACUUM (FROZEN) seems to be better than (FORCE)?
>
> There already is FREEZE - meaning something different - so I doubt it.

Yeah I thought about that, it is the word "FORCE" that bothers me. When
you use FORCE there is an assumption that no matter what, it plows
through (think rm -f). So if we don't use FROZEN, that's cool but FORCE
doesn't work either.

Sincerely,

JD

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

* Joshua D. Drake (jd(at)commandprompt(dot)com) wrote:
> Yeah I thought about that, it is the word "FORCE" that bothers me.
> When you use FORCE there is an assumption that no matter what, it
> plows through (think rm -f). So if we don't use FROZEN, that's cool
> but FORCE doesn't work either.

Isn't that exactly what this FORCE option being contemplated would do
though? Plow through the entire relation, regardless of what the VM
says is all frozen or not?

Seems like FORCE is a good word for that to me.

Thanks!

Stephen

On 2016-05-06 13:54:09 -0700, Joshua D. Drake wrote:
> On 05/06/2016 01:50 PM, Andres Freund wrote:
>
> > > > Let's add VACUUM (FORCE) or something like that.
> >
> > Yes, that makes sense.
> >
> >
> > > This is actually inverted. Vacuum by default should vacuum the entire
> > > relation
> >
> > What? Why on earth would that be a good idea? Not to speak of hte fact
> > that that's not been the case since ~8.4?
>
> Sorry, I just meant the default behavior shouldn't change but I do agree
> that we need the ability to keep the same behavior.

Which default behaviour shouldn't change? The one in master where we
skip known frozen pages? Or the released branches where can't skip those?

> > > ,however if we are going to keep the existing behavior of this
> > > patch, VACUUM (FROZEN) seems to be better than (FORCE)?
> >
> > There already is FREEZE - meaning something different - so I doubt it.
>
> Yeah I thought about that, it is the word "FORCE" that bothers me. When you
> use FORCE there is an assumption that no matter what, it plows through
> (think rm -f). So if we don't use FROZEN, that's cool but FORCE doesn't work
> either.

SCANALL?

On 05/06/2016 01:58 PM, Stephen Frost wrote:
> * Joshua D. Drake (jd(at)commandprompt(dot)com) wrote:
>> Yeah I thought about that, it is the word "FORCE" that bothers me.
>> When you use FORCE there is an assumption that no matter what, it
>> plows through (think rm -f). So if we don't use FROZEN, that's cool
>> but FORCE doesn't work either.
>
> Isn't that exactly what this FORCE option being contemplated would do
> though? Plow through the entire relation, regardless of what the VM
> says is all frozen or not?
>
> Seems like FORCE is a good word for that to me.

Except that we aren't FORCING a vacuum. That is the part I have
contention with. To me, FORCE means:

No matter what else is happening, we are vacuuming this relation (think
locks).

But I am also not going to dig in my heals. If that is truly what
-hackers come up with, thank you at least considering what I said.

Sincerely,

JD

>
> Thanks!
>
> Stephen
>

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

On 05/06/2016 01:58 PM, Andres Freund wrote:
> On 2016-05-06 13:54:09 -0700, Joshua D. Drake wrote:
>> On 05/06/2016 01:50 PM, Andres Freund wrote:

>>> There already is FREEZE - meaning something different - so I doubt it.
>>
>> Yeah I thought about that, it is the word "FORCE" that bothers me. When you
>> use FORCE there is an assumption that no matter what, it plows through
>> (think rm -f). So if we don't use FROZEN, that's cool but FORCE doesn't work
>> either.
>
> SCANALL?
>

VACUUM THEWHOLEDAMNTHING

--
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

On 05/06/2016 02:01 PM, Josh berkus wrote:
> On 05/06/2016 01:58 PM, Andres Freund wrote:
>> On 2016-05-06 13:54:09 -0700, Joshua D. Drake wrote:
>>> On 05/06/2016 01:50 PM, Andres Freund wrote:
>
>>>> There already is FREEZE - meaning something different - so I doubt it.
>>>
>>> Yeah I thought about that, it is the word "FORCE" that bothers me. When you
>>> use FORCE there is an assumption that no matter what, it plows through
>>> (think rm -f). So if we don't use FROZEN, that's cool but FORCE doesn't work
>>> either.
>>
>> SCANALL?
>>
>
> VACUUM THEWHOLEDAMNTHING
>

I know that would never fly but damn if that wouldn't be an awesome
keyword for VACUUM.

JD

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

* Josh berkus (josh(at)agliodbs(dot)com) wrote:
> On 05/06/2016 01:58 PM, Andres Freund wrote:
> > On 2016-05-06 13:54:09 -0700, Joshua D. Drake wrote:
> >> On 05/06/2016 01:50 PM, Andres Freund wrote:
>
> >>> There already is FREEZE - meaning something different - so I doubt it.
> >>
> >> Yeah I thought about that, it is the word "FORCE" that bothers me. When you
> >> use FORCE there is an assumption that no matter what, it plows through
> >> (think rm -f). So if we don't use FROZEN, that's cool but FORCE doesn't work
> >> either.
> >
> > SCANALL?
> >
>
> VACUUM THEWHOLEDAMNTHING

+100

(hahahaha)

Thanks!

Stephen

On 2016-05-06 14:03:11 -0700, Joshua D. Drake wrote:
> On 05/06/2016 02:01 PM, Josh berkus wrote:
> > On 05/06/2016 01:58 PM, Andres Freund wrote:
> > > On 2016-05-06 13:54:09 -0700, Joshua D. Drake wrote:
> > > > On 05/06/2016 01:50 PM, Andres Freund wrote:
> >
> > > > > There already is FREEZE - meaning something different - so I doubt it.
> > > >
> > > > Yeah I thought about that, it is the word "FORCE" that bothers me. When you
> > > > use FORCE there is an assumption that no matter what, it plows through
> > > > (think rm -f). So if we don't use FROZEN, that's cool but FORCE doesn't work
> > > > either.
> > >
> > > SCANALL?
> > >
> >
> > VACUUM THEWHOLEDAMNTHING
> >
>
> I know that would never fly but damn if that wouldn't be an awesome keyword
> for VACUUM.

It bothers me more than it probably should: Nobdy tests, reviews,
whatever a complex patch with significant data-loss potential. But as
soon somebody dares to mention an option name...

On 05/06/2016 02:03 PM, Stephen Frost wrote:

>>
>> VACUUM THEWHOLEDAMNTHING
>
> +100
>
> (hahahaha)

You know what? Why not? Seriously? We aren't product. This is supposed
to be a bit fun. Let's have some fun with it? It would be so easy to
turn that into a positive advocacy opportunity.

JD

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

On 05/06/2016 02:08 PM, Andres Freund wrote:

> It bothers me more than it probably should: Nobdy tests, reviews,
> whatever a complex patch with significant data-loss potential. But as
> soon somebody dares to mention an option name...

Definitely more than it should, because it's gonna happen *every* time.

https://en.wikipedia.org/wiki/Law_of_triviality

--
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

On 2016-05-06 14:10:04 -0700, Josh berkus wrote:
> On 05/06/2016 02:08 PM, Andres Freund wrote:
>
> > It bothers me more than it probably should: Nobdy tests, reviews,
> > whatever a complex patch with significant data-loss potential. But as
> > soon somebody dares to mention an option name...
>
> Definitely more than it should, because it's gonna happen *every* time.
>
> https://en.wikipedia.org/wiki/Law_of_triviality

Doesn't mean it should not be frowned upon.

On 05/06/2016 02:12 PM, Andres Freund wrote:
> On 2016-05-06 14:10:04 -0700, Josh berkus wrote:
>> On 05/06/2016 02:08 PM, Andres Freund wrote:
>>
>>> It bothers me more than it probably should: Nobdy tests, reviews,
>>> whatever a complex patch with significant data-loss potential. But as
>>> soon somebody dares to mention an option name...
>>
>> Definitely more than it should, because it's gonna happen *every* time.
>>
>> https://en.wikipedia.org/wiki/Law_of_triviality
>
> Doesn't mean it should not be frowned upon.

Or made light of, hence my post. Personally I don't care what the
option is called, as long as we have docs for it.

For the serious testing, does anyone have a good technique for creating
loads which would stress-test vacuum freezing? It's hard for me to come
up with anything which wouldn't be very time-and-resource intensive
(like running at 10,000 TPS for a week).

--
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

On 05/06/2016 02:08 PM, Andres Freund wrote:

>>> VACUUM THEWHOLEDAMNTHING
>>>
>>
>> I know that would never fly but damn if that wouldn't be an awesome keyword
>> for VACUUM.
>
> It bothers me more than it probably should: Nobdy tests, reviews,
> whatever a complex patch with significant data-loss potential. But as
> soon somebody dares to mention an option name...

That is a fair complaint but let me ask you something:

How do I test?

Is there a script I can run? Are there specific things I can do to try
and break it? What are we looking for exactly?

A lot of -hackers seem to forget that although we have 100 -hackers, we
have 10000 "consultant/practitioners". Could I read the code and with a
weekend of WTF and -hackers questions figure out what is going on, yes
but a lot of people couldn't and I don't have the time.

You want me (or people like me) to test more? Give us an easy way to do
it. Otherwise, we do what we can, which is try and interface on the
things that will directly and immediately affect us (like keywords and
syntax).

Sincerely,

JD

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

On 2016-05-06 14:15:47 -0700, Josh berkus wrote:
> For the serious testing, does anyone have a good technique for creating
> loads which would stress-test vacuum freezing? It's hard for me to come
> up with anything which wouldn't be very time-and-resource intensive
> (like running at 10,000 TPS for a week).

I've changed the limits for freezing options a while back, so you can
now set autovacuum_freeze_max as low as 100000 (best set
vacuum_freeze_table_age accordingly). You'll have to come up with a
workload that doesn't overwrite all data continuously (otherwise
there'll never be old rows), but otherwise it should now be fairly easy
to test that kind of scenario.

Andres

Hi,

On 2016-05-06 14:17:13 -0700, Joshua D. Drake wrote:
> How do I test?
>
> Is there a script I can run?

Unfortunately there's few interesting things to test with pre-made
scripts. There's no relevant OS dependency here, so each already
existing test doesn't really lead to significantly increased coverage
being run by other people. Generally, when testing for correctness
issues, it's often of limited benefit to run tests written by the author
of reviewer - such scripts will usually just test things either has
thought of. The dangerous areas are the ones neither author or reviewer
has considered.

> Are there specific things I can do to try and break it?

Upgrade clusters using pg_upgrade and make sure things like index only
scans still work and yield correct data. Set up workloads that involve
freezing, and check that less WAL (and not more!) is generated with 9.6
than with 9.5. Make sure queries still work.

> What are we looking for exactly?

Data corruption, efficiency problems.

> A lot of -hackers seem to forget that although we have 100 -hackers, we have
> 10000 "consultant/practitioners". Could I read the code and with a weekend
> of WTF and -hackers questions figure out what is going on, yes but a lot of
> people couldn't and I don't have the time.

I think tests without reading the code are quite sensible and
important. And it perfectly makes sense to ask for information about
what to test. But fundamentally testing is a lot of work, as is writing
and reviewing code; unless you're really really good at destructive
testing, you won't find much in a 15 minute break.

> You want me (or people like me) to test more? Give us an easy way to
> do it.

Useful additional testing and easy just don't go well together. By the
time I have made it easy I've done the testing that's needed.

> Otherwise, we do what we can, which is try and interface on the things that
> will directly and immediately affect us (like keywords and syntax).

The amount of bikeshedding on -hackers steals energy and time for
actually working on stuff, including testing. So I have little sympathy
for the amount of bike shedding done.

Greetings,

Andres Freund

Joshua D. Drake wrote:
> On 05/06/2016 01:40 PM, Robert Haas wrote:
> >On Wed, May 4, 2016 at 8:08 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >>On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
> >>>77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.
> >>
> >>Nothing to say here.
> >>
> >>>fd31cd2 Don't vacuum all-frozen pages.
> >>
> >>Hm. I do wonder if it's going to bite us that we don't have a way to
> >>actually force vacuuming of the whole table (besides manually rm'ing the
> >>VM). I've more than once seen VACUUM used to try to do some integrity
> >>checking of the database. How are we actually going to test that the
> >>feature works correctly? They'd have to write checks ontop of
> >>pg_visibility to see whether things are borked.
> >
> >Let's add VACUUM (FORCE) or something like that.
>
> This is actually inverted. Vacuum by default should vacuum the entire
> relation, however if we are going to keep the existing behavior of this
> patch, VACUUM (FROZEN) seems to be better than (FORCE)?

Prior to some 7.x release, VACUUM actually did what we ripped out in
9.0 release as VACUUM FULL. We actually changed the mode of operation
quite heavily into the "lazy" mode which didn't acquire access exclusive
lock, and it was a huge relief. I think that changing the mode of
operation to be the lightest possible thing that gets the job done
is convenient for users, because their existing scripts continue to
clean their tables only they take less time. No need to tweak the
maintenance scripts.

I don't know what happens when the freeze_table_age threshold is
reached. Do we scan the whole table when that happens? Because if we
do, then we don't need a new keyword: just invoke the command after
lowering the setting.

Another question on this feature is what happens with the table age
(relfrozenxid, relminmxid) when the table is not wholly scanned by
vacuum.

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

Andres Freund wrote:

> On 2016-05-06 14:17:13 -0700, Joshua D. Drake wrote:
> > How do I test?
> >
> > Is there a script I can run?
>
> Unfortunately there's few interesting things to test with pre-made
> scripts. There's no relevant OS dependency here, so each already
> existing test doesn't really lead to significantly increased coverage
> being run by other people. Generally, when testing for correctness
> issues, it's often of limited benefit to run tests written by the author
> of reviewer - such scripts will usually just test things either has
> thought of. The dangerous areas are the ones neither author or reviewer
> has considered.

We touched this question in connection with multixact freezing and
wraparound. Testers seem to want to be given a script that they can
install and run, then go for a beer and get back to a bunch of errors to
report. But it doesn't work that way; writing a useful test script
requires a lot of effort. Jeff Janes has done astounding work in these
matters. (I don't think we credit him enough for that.)

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

On 05/06/2016 02:29 PM, Andres Freund wrote:
> Hi,
>
>
> On 2016-05-06 14:17:13 -0700, Joshua D. Drake wrote:
>> How do I test?
>>
>> Is there a script I can run?
>
> Unfortunately there's few interesting things to test with pre-made
> scripts. There's no relevant OS dependency here, so each already
> existing test doesn't really lead to significantly increased coverage
> being run by other people. Generally, when testing for correctness
> issues, it's often of limited benefit to run tests written by the author
> of reviewer - such scripts will usually just test things either has
> thought of. The dangerous areas are the ones neither author or reviewer
> has considered.

I can't argue with that.

>
>
>> Are there specific things I can do to try and break it?
>
> Upgrade clusters using pg_upgrade and make sure things like index only
> scans still work and yield correct data. Set up workloads that involve
> freezing, and check that less WAL (and not more!) is generated with 9.6
> than with 9.5. Make sure queries still work.
>
>
>> What are we looking for exactly?
>
> Data corruption, efficiency problems.
>

I am really not trying to be difficult here but Data Corruption is an
easy one... what is the metric we accept as an efficiency problem?

>
>> A lot of -hackers seem to forget that although we have 100 -hackers, we have
>> 10000 "consultant/practitioners". Could I read the code and with a weekend
>> of WTF and -hackers questions figure out what is going on, yes but a lot of
>> people couldn't and I don't have the time.
>
> I think tests without reading the code are quite sensible and
> important. And it perfectly makes sense to ask for information about
> what to test. But fundamentally testing is a lot of work, as is writing
> and reviewing code; unless you're really really good at destructive
> testing, you won't find much in a 15 minute break.
>

Yes, this is true but with a proper testing framework, I don't need a 15
minute break. I need 1 hour to configure, the rest just "happens" and
reports back.

I have cycles to test, I have team members to help test (as do *lots* of
other people) but sometimes we just get lost in how to help.

>
>> You want me (or people like me) to test more? Give us an easy way to
>> do it.
>
> Useful additional testing and easy just don't go well together. By the
> time I have made it easy I've done the testing that's needed.

I don't know that I can agree with this. A proper harness allows you to
execute: go.sh and boom... 2, 4, even 8 hours later you get a report. I
will not argue that it isn't easy to implement but I know it can be done.

>> Otherwise, we do what we can, which is try and interface on the things that
>> will directly and immediately affect us (like keywords and syntax).
>
> The amount of bikeshedding on -hackers steals energy and time for
> actually working on stuff, including testing. So I have little sympathy
> for the amount of bike shedding done.

Insuring a reasonable and thought out interface for users is not bike
shedding, it is at least as important and possibly more important than
any feature we add.

Sincerely,

JD

>
> Greetings,
>
> Andres Freund
>

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

On 2016-05-06 14:39:57 -0700, Joshua D. Drake wrote:
> > > What are we looking for exactly?
> >
> > Data corruption, efficiency problems.
> >
>
> I am really not trying to be difficult here but Data Corruption is an easy
> one... what is the metric we accept as an efficiency problem?

That's indeed not easy to define. In this case I'd say vacuums taking
longer, index only scans being slower, more WAL being generated would
count?

> > I think tests without reading the code are quite sensible and
> > important. And it perfectly makes sense to ask for information about
> > what to test. But fundamentally testing is a lot of work, as is writing
> > and reviewing code; unless you're really really good at destructive
> > testing, you won't find much in a 15 minute break.
> >
>
> Yes, this is true but with a proper testing framework, I don't need a 15
> minute break. I need 1 hour to configure, the rest just "happens" and
> reports back.

That only works if somebody writes such tests. And in that case the
tester having run will often suffice (until related changes are being
made). I'm not arguing against introducing more tests into the codebase
- I rather fervently for that. But that really isn't what's going to
avoid issues like this feature (or multixact) causing problems, because
those tests will just test what the author thought of.

> > > You want me (or people like me) to test more? Give us an easy way to
> > > do it.
> >
> > Useful additional testing and easy just don't go well together. By the
> > time I have made it easy I've done the testing that's needed.
>
> I don't know that I can agree with this. A proper harness allows you to
> execute: go.sh and boom... 2, 4, even 8 hours later you get a report. I will
> not argue that it isn't easy to implement but I know it can be done.

The problem is that the contents of go.sh are the much more relevant
part than the 8 hours.

Greetings,

Andres Freund

On 2016-05-06 18:36:52 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
>
> > On 2016-05-06 14:17:13 -0700, Joshua D. Drake wrote:
> > > How do I test?
> > >
> > > Is there a script I can run?
> >
> > Unfortunately there's few interesting things to test with pre-made
> > scripts. There's no relevant OS dependency here, so each already
> > existing test doesn't really lead to significantly increased coverage
> > being run by other people. Generally, when testing for correctness
> > issues, it's often of limited benefit to run tests written by the author
> > of reviewer - such scripts will usually just test things either has
> > thought of. The dangerous areas are the ones neither author or reviewer
> > has considered.
>
> We touched this question in connection with multixact freezing and
> wraparound. Testers seem to want to be given a script that they can
> install and run, then go for a beer and get back to a bunch of errors to
> report. But it doesn't work that way; writing a useful test script
> requires a lot of effort.

Right. And once written, often enough running it on a lot more instances
only marginally increases the coverage.

> Jeff Janes has done astounding work in these matters. (I don't think
> we credit him enough for that.)

+many.

On 05/06/2016 02:48 PM, Andres Freund wrote:
> On 2016-05-06 14:39:57 -0700, Joshua D. Drake wrote:

>> Yes, this is true but with a proper testing framework, I don't need a 15
>> minute break. I need 1 hour to configure, the rest just "happens" and
>> reports back.
>
> That only works if somebody writes such tests.

Agreed.

> And in that case the
> tester having run will often suffice (until related changes are being
> made). I'm not arguing against introducing more tests into the codebase
> - I rather fervently for that. But that really isn't what's going to
> avoid issues like this feature (or multixact) causing problems, because
> those tests will just test what the author thought of.
>

Good point. I am not sure how to address the alternative though.

>
>>>> You want me (or people like me) to test more? Give us an easy way to
>>>> do it.
>>>
>>> Useful additional testing and easy just don't go well together. By the
>>> time I have made it easy I've done the testing that's needed.
>>
>> I don't know that I can agree with this. A proper harness allows you to
>> execute: go.sh and boom... 2, 4, even 8 hours later you get a report. I will
>> not argue that it isn't easy to implement but I know it can be done.
>
> The problem is that the contents of go.sh are the much more relevant
> part than the 8 hours.

True.

Please don't misunderstand, I am not saying this is "easy". I just hope
that it is something we work for.

Sincerely,

JD

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

On 2016-05-06 18:31:03 -0300, Alvaro Herrera wrote:
> I don't know what happens when the freeze_table_age threshold is
> reached.

We scan all non-frozen pages, whereas we earlier had to scan all pages.

That's really both the significant benefit, and the danger. Because if
we screw up the all-frozen bits in the visibilitymap, we'll be screwed
soon after.

> Do we scan the whole table when that happens?

No, there's atm no way to force a whole-table vacuum, besides manually
rm'ing the _vm fork.

> Another question on this feature is what happens with the table age
> (relfrozenxid, relminmxid) when the table is not wholly scanned by
> vacuum.

Basically we increase the horizons whenever scanning all pages that are
not known to be frozen (+ potentially some frozen ones due to the
skipping logic). Without that there'd really not be a point in the
freeze map feature, as we'd continue to have the expensive anti
wraparound vacuums.

Andres

On Fri, May 6, 2016 at 2:49 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> Jeff Janes has done astounding work in these matters. (I don't think
>> we credit him enough for that.)
>
> +many.

Agreed. I'm a huge fan of what Jeff has been able to do in this area.
I often say so. It would be even better if Jeff's approach to testing
was followed as an example by other people, but I wouldn't bet on it
ever happening. It requires real persistence and deep understanding to
do well.

--
Peter Geoghegan

On Sat, May 7, 2016 at 8:34 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, May 2, 2016 at 8:25 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> +static const uint8 number_of_ones_for_visible[256] = {
>> ...
>> +};
>> +static const uint8 number_of_ones_for_frozen[256] = {
>> ...
>> };
>>
>> Did somebody verify the new contents are correct?
>
> I admit that I didn't. It seemed like an unlikely place for a goof,
> but I guess we should verify.

Looks correct. The tables match the output of the attached script.

--
Thomas Munro
http://www.enterprisedb.com

On 2016-05-07 10:00:27 +1200, Thomas Munro wrote:
> On Sat, May 7, 2016 at 8:34 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >> Did somebody verify the new contents are correct?
> >
> > I admit that I didn't. It seemed like an unlikely place for a goof,
> > but I guess we should verify.
>
> Looks correct. The tables match the output of the attached script.

Great!

Alvaro Herrera wrote:

> We touched this question in connection with multixact freezing and
> wraparound. Testers seem to want to be given a script that they can
> install and run, then go for a beer and get back to a bunch of errors to
> report.

Here I spent some time trying to explain what to test to try and find
certain multixact bugs
http://www.postgresql.org/message-id/20150605213832.GZ133018@postgresql.org

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

On Sat, May 7, 2016 at 6:00 AM, Joshua D. Drake <jd(at)commandprompt(dot)com> wrote:
> On 05/06/2016 01:58 PM, Stephen Frost wrote:
>>
>> * Joshua D. Drake (jd(at)commandprompt(dot)com) wrote:
>>>
>>> Yeah I thought about that, it is the word "FORCE" that bothers me.
>>> When you use FORCE there is an assumption that no matter what, it
>>> plows through (think rm -f). So if we don't use FROZEN, that's cool
>>> but FORCE doesn't work either.
>>
>>
>> Isn't that exactly what this FORCE option being contemplated would do
>> though? Plow through the entire relation, regardless of what the VM
>> says is all frozen or not?
>>
>> Seems like FORCE is a good word for that to me.
>
>
> Except that we aren't FORCING a vacuum. That is the part I have contention
> with. To me, FORCE means:
>
> No matter what else is happening, we are vacuuming this relation (think
> locks).
>
> But I am also not going to dig in my heals. If that is truly what -hackers
> come up with, thank you at least considering what I said.
>
> Sincerely,
>
> JD
>

As Joshua mentioned, FORCE word might imply doing VACUUM while plowing
through locks.
I guess that it might confuse the users.
IMO, since this option will be a way for emergency, SCANALL word works for me.

Or other ideas are,
VACUUM IGNOREVM
VACUUM RESCURE

Regards,

--
Masahiko Sawada

On Sat, May 7, 2016 at 11:08 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Sat, May 7, 2016 at 6:00 AM, Joshua D. Drake <jd(at)commandprompt(dot)com> wrote:
>> On 05/06/2016 01:58 PM, Stephen Frost wrote:
>>>
>>> * Joshua D. Drake (jd(at)commandprompt(dot)com) wrote:
>>>>
>>>> Yeah I thought about that, it is the word "FORCE" that bothers me.
>>>> When you use FORCE there is an assumption that no matter what, it
>>>> plows through (think rm -f). So if we don't use FROZEN, that's cool
>>>> but FORCE doesn't work either.
>>>
>>>
>>> Isn't that exactly what this FORCE option being contemplated would do
>>> though? Plow through the entire relation, regardless of what the VM
>>> says is all frozen or not?
>>>
>>> Seems like FORCE is a good word for that to me.
>>
>>
>> Except that we aren't FORCING a vacuum. That is the part I have contention
>> with. To me, FORCE means:
>>
>> No matter what else is happening, we are vacuuming this relation (think
>> locks).
>>
>> But I am also not going to dig in my heals. If that is truly what -hackers
>> come up with, thank you at least considering what I said.
>>
>> Sincerely,
>>
>> JD
>>
>
> As Joshua mentioned, FORCE word might imply doing VACUUM while plowing
> through locks.
> I guess that it might confuse the users.
> IMO, since this option will be a way for emergency, SCANALL word works for me.
>
> Or other ideas are,
> VACUUM IGNOREVM
> VACUUM RESCURE
>

Oops, VACUUM RESCUE is correct.

Regards,

--
Masahiko Sawada

On Sun, May 8, 2016 at 3:18 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Sat, May 7, 2016 at 11:08 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Sat, May 7, 2016 at 6:00 AM, Joshua D. Drake <jd(at)commandprompt(dot)com> wrote:
>>> On 05/06/2016 01:58 PM, Stephen Frost wrote:
>>>>
>>>> * Joshua D. Drake (jd(at)commandprompt(dot)com) wrote:
>>>>>
>>>>> Yeah I thought about that, it is the word "FORCE" that bothers me.
>>>>> When you use FORCE there is an assumption that no matter what, it
>>>>> plows through (think rm -f). So if we don't use FROZEN, that's cool
>>>>> but FORCE doesn't work either.
>>>>
>>>>
>>>> Isn't that exactly what this FORCE option being contemplated would do
>>>> though? Plow through the entire relation, regardless of what the VM
>>>> says is all frozen or not?
>>>>
>>>> Seems like FORCE is a good word for that to me.
>>>
>>>
>>> Except that we aren't FORCING a vacuum. That is the part I have contention
>>> with. To me, FORCE means:
>>>
>>> No matter what else is happening, we are vacuuming this relation (think
>>> locks).
>>>
>>> But I am also not going to dig in my heals. If that is truly what -hackers
>>> come up with, thank you at least considering what I said.
>>>
>>> Sincerely,
>>>
>>> JD
>>>
>>
>> As Joshua mentioned, FORCE word might imply doing VACUUM while plowing
>> through locks.
>> I guess that it might confuse the users.
>> IMO, since this option will be a way for emergency, SCANALL word works for me.
>>
>> Or other ideas are,
>> VACUUM IGNOREVM
>> VACUUM RESCURE
>>
>
> Oops, VACUUM RESCUE is correct.
>

Attached draft patch adds SCANALL option to VACUUM in order to scan
all pages forcibly while ignoring visibility map information.
The option name is SCANALL for now but we could change it after got consensus.

Regards,

--
Masahiko Sawada

On Tue, May 3, 2016 at 6:48 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> fd31cd2 Don't vacuum all-frozen pages.

- appendStringInfo(&buf, _("pages: %u removed,
%u remain, %u skipped due to pins\n"),
+ appendStringInfo(&buf, _("pages: %u removed,
%u remain, %u skipped due to pins, %u skipped frozen\n"),

vacrelstats->pages_removed,
vacrelstats->rel_pages,
-
vacrelstats->pinskipped_pages);
+
vacrelstats->pinskipped_pages,
+
vacrelstats->frozenskipped_pages);

The verbose information about skipping frozen page is emitted by only
autovacuum.
But I think that this information is also helpful for manual vacuum.

Please find attached patch which fixes that.

Regards,

--
Masahiko Sawada

On Sun, May 8, 2016 at 10:42 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> Attached draft patch adds SCANALL option to VACUUM in order to scan
> all pages forcibly while ignoring visibility map information.
> The option name is SCANALL for now but we could change it after got consensus.

If we're going to go that way, I'd say it should be scan_all rather
than scanall. Makes it clearer, at least IMHO.

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

On Mon, May 9, 2016 at 10:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sun, May 8, 2016 at 10:42 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> Attached draft patch adds SCANALL option to VACUUM in order to scan
>> all pages forcibly while ignoring visibility map information.
>> The option name is SCANALL for now but we could change it after got consensus.
>
> If we're going to go that way, I'd say it should be scan_all rather
> than scanall. Makes it clearer, at least IMHO.

Just to add some diversity to opinions, maybe there should be a
separate command for performing integrity checks. Currently the best
ways to actually verify database correctness do so as a side effect.
The question that I get pretty much every time after I explain why we
have data checksums, is "how do I check that they are correct" and we
don't have a nice answer for that now. We could also use some ways to
sniff out corrupted rows that don't involve crashing the server in a
loop. Vacuuming pages that supposedly don't need vacuuming just to
verify integrity seems very much in the same vein.

I know right now isn't exactly the best time to hastily slap on such a
feature, but I just wanted the thought to be out there for
consideration.

Regards,
Ants Aasma

On Mon, May 9, 2016 at 7:40 PM, Ants Aasma <ants(dot)aasma(at)eesti(dot)ee> wrote:
> On Mon, May 9, 2016 at 10:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Sun, May 8, 2016 at 10:42 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> Attached draft patch adds SCANALL option to VACUUM in order to scan
>>> all pages forcibly while ignoring visibility map information.
>>> The option name is SCANALL for now but we could change it after got consensus.
>>
>> If we're going to go that way, I'd say it should be scan_all rather
>> than scanall. Makes it clearer, at least IMHO.
>
> Just to add some diversity to opinions, maybe there should be a
> separate command for performing integrity checks. Currently the best
> ways to actually verify database correctness do so as a side effect.
> The question that I get pretty much every time after I explain why we
> have data checksums, is "how do I check that they are correct" and we
> don't have a nice answer for that now. We could also use some ways to
> sniff out corrupted rows that don't involve crashing the server in a
> loop. Vacuuming pages that supposedly don't need vacuuming just to
> verify integrity seems very much in the same vein.
>
> I know right now isn't exactly the best time to hastily slap on such a
> feature, but I just wanted the thought to be out there for
> consideration.

I think that it's quite reasonable to have ways of performing an
integrity check that are separate from VACUUM, but this is about
having a way to force VACUUM to scan all-frozen pages - and it's hard
to imagine that we want a different command name for that.

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

On Tue, May 10, 2016 at 11:30 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, May 9, 2016 at 7:40 PM, Ants Aasma <ants(dot)aasma(at)eesti(dot)ee> wrote:
>> On Mon, May 9, 2016 at 10:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Sun, May 8, 2016 at 10:42 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>> Attached draft patch adds SCANALL option to VACUUM in order to scan
>>>> all pages forcibly while ignoring visibility map information.
>>>> The option name is SCANALL for now but we could change it after got consensus.
>>>
>>> If we're going to go that way, I'd say it should be scan_all rather
>>> than scanall. Makes it clearer, at least IMHO.
>>
>> Just to add some diversity to opinions, maybe there should be a
>> separate command for performing integrity checks. Currently the best
>> ways to actually verify database correctness do so as a side effect.
>> The question that I get pretty much every time after I explain why we
>> have data checksums, is "how do I check that they are correct" and we
>> don't have a nice answer for that now. We could also use some ways to
>> sniff out corrupted rows that don't involve crashing the server in a
>> loop. Vacuuming pages that supposedly don't need vacuuming just to
>> verify integrity seems very much in the same vein.
>>
>> I know right now isn't exactly the best time to hastily slap on such a
>> feature, but I just wanted the thought to be out there for
>> consideration.
>
> I think that it's quite reasonable to have ways of performing an
> integrity check that are separate from VACUUM, but this is about
> having a way to force VACUUM to scan all-frozen pages

Or second way I came up with is having tool to remove particular _vm
file safely, which is executed via SQL or client tool like
pg_resetxlog.

Attached updated VACUUM SCAN_ALL patch.
Please find it.

Regards,

--
Masahiko Sawada

On 5/6/16 4:20 PM, Andres Freund wrote:
> On 2016-05-06 14:15:47 -0700, Josh berkus wrote:
>> For the serious testing, does anyone have a good technique for creating
>> loads which would stress-test vacuum freezing? It's hard for me to come
>> up with anything which wouldn't be very time-and-resource intensive
>> (like running at 10,000 TPS for a week).
>
> I've changed the limits for freezing options a while back, so you can
> now set autovacuum_freeze_max as low as 100000 (best set
> vacuum_freeze_table_age accordingly). You'll have to come up with a
> workload that doesn't overwrite all data continuously (otherwise
> there'll never be old rows), but otherwise it should now be fairly easy
> to test that kind of scenario.

There's also been a tool for forcibly advancing XID floating around for
quite some time. Using that could have the added benefit of verifying
anti-wrap still works correctly. (Might be worth testing mxid wrap too...)
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

On 5/6/16 4:55 PM, Peter Geoghegan wrote:
> On Fri, May 6, 2016 at 2:49 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>> Jeff Janes has done astounding work in these matters. (I don't think
>>> we credit him enough for that.)
>>
>> +many.
>
> Agreed. I'm a huge fan of what Jeff has been able to do in this area.
> I often say so. It would be even better if Jeff's approach to testing
> was followed as an example by other people, but I wouldn't bet on it
> ever happening. It requires real persistence and deep understanding to
> do well.

It takes deep understanding to *design* the tests, not to write them.
There's a lot of folks out there that will never understand enough to
design tests meant to expose data corruption but who could easily code
someone else's design, especially if we provided tools/ways to tweak a
cluster to make testing easier/faster (such as artificially advancing
XID/MXID).
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

On 5/6/16 4:08 PM, Joshua D. Drake wrote:
>>>
>>> VACUUM THEWHOLEDAMNTHING
>>
>> +100
>>
>> (hahahaha)
>
> You know what? Why not? Seriously? We aren't product. This is supposed
> to be a bit fun. Let's have some fun with it? It would be so easy to
> turn that into a positive advocacy opportunity.

Honestly, for an option this obscure, I agree. I don't think we'd want
any normally used stuff named so glibly, but I sure as heck could have
used some easter-eggs like this when I was doing training.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

On 5/10/16 11:42 PM, Jim Nasby wrote:
> On 5/6/16 4:55 PM, Peter Geoghegan wrote:
>> On Fri, May 6, 2016 at 2:49 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>>> Jeff Janes has done astounding work in these matters. (I don't think
>>>> we credit him enough for that.)
>>>
>>> +many.
>>
>> Agreed. I'm a huge fan of what Jeff has been able to do in this area.
>> I often say so. It would be even better if Jeff's approach to testing
>> was followed as an example by other people, but I wouldn't bet on it
>> ever happening. It requires real persistence and deep understanding to
>> do well.
>
> It takes deep understanding to *design* the tests, not to write them.
> There's a lot of folks out there that will never understand enough to
> design tests meant to expose data corruption but who could easily code
> someone else's design, especially if we provided tools/ways to tweak a
> cluster to make testing easier/faster (such as artificially advancing
> XID/MXID).

Speaking of which, another email in the thread made me realize that
there's a test condition no one has mentioned: verifying we don't lose
tuples after wraparound.

To test this, you'd want a table that's mostly frozen. Ideally, dirty a
single tuple on a bunch of frozen pages, with committed updates,
deletes, and un-committed inserts. Advance XID far enough to get you
close to wrap-around. Do a vacuum, SELECT count(*), advance XID past
wraparound, SELECT count(*) again and you should get the same number.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

On Tue, May 10, 2016 at 10:40 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> Or second way I came up with is having tool to remove particular _vm
> file safely, which is executed via SQL or client tool like
> pg_resetxlog.
>
> Attached updated VACUUM SCAN_ALL patch.
> Please find it.

We should support scan_all only with the new-style options syntax for
VACUUM; that is, vacuum (scan_all) rename. That doesn't require
making scan_all a keyword, which is good: this is a minor feature, and
we don't want to bloat the parsing tables for it.

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

On Mon, May 16, 2016 at 10:49 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, May 10, 2016 at 10:40 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> Or second way I came up with is having tool to remove particular _vm
>> file safely, which is executed via SQL or client tool like
>> pg_resetxlog.
>>
>> Attached updated VACUUM SCAN_ALL patch.
>> Please find it.
>
> We should support scan_all only with the new-style options syntax for
> VACUUM; that is, vacuum (scan_all) rename. That doesn't require
> making scan_all a keyword, which is good: this is a minor feature, and
> we don't want to bloat the parsing tables for it.
>

I agree with having new-style options syntax.
Isn't it better to have SCAN_ALL option without parentheses?

Syntaxes are;
VACUUM SCAN_ALL table_name;
VACUUM SCAN_ALL; -- for all tables on database

Regards,

--
Masahiko Sawada

Masahiko Sawada wrote:
> On Mon, May 16, 2016 at 10:49 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> > We should support scan_all only with the new-style options syntax for
> > VACUUM; that is, vacuum (scan_all) rename. That doesn't require
> > making scan_all a keyword, which is good: this is a minor feature, and
> > we don't want to bloat the parsing tables for it.
>
> I agree with having new-style options syntax.
> Isn't it better to have SCAN_ALL option without parentheses?
>
> Syntaxes are;
> VACUUM SCAN_ALL table_name;
> VACUUM SCAN_ALL; -- for all tables on database

No, I agree with Robert that we shouldn't add any more such options to
avoid keyword proliferation.

Syntaxes are;
VACUUM (SCAN_ALL) table_name;
VACUUM (SCAN_ALL); -- for all tables on database

Is SCAN_ALL really the best we can do here? The business of having an
underscore in an option name has no precedent (other than
CURRENT_DATABASE and the like). How about COMPLETE, TOTAL, or WHOLE?

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

On Tue, May 17, 2016 at 3:32 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Masahiko Sawada wrote:
>> On Mon, May 16, 2016 at 10:49 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> > We should support scan_all only with the new-style options syntax for
>> > VACUUM; that is, vacuum (scan_all) rename. That doesn't require
>> > making scan_all a keyword, which is good: this is a minor feature, and
>> > we don't want to bloat the parsing tables for it.
>>
>> I agree with having new-style options syntax.
>> Isn't it better to have SCAN_ALL option without parentheses?
>>
>> Syntaxes are;
>> VACUUM SCAN_ALL table_name;
>> VACUUM SCAN_ALL; -- for all tables on database
>
> No, I agree with Robert that we shouldn't add any more such options to
> avoid keyword proliferation.
>
> Syntaxes are;
> VACUUM (SCAN_ALL) table_name;
> VACUUM (SCAN_ALL); -- for all tables on database

Okay, I agree with this.

> Is SCAN_ALL really the best we can do here? The business of having an
> underscore in an option name has no precedent (other than
> CURRENT_DATABASE and the like).

Another way is having tool or function that removes _vm file safely for example.

> How about COMPLETE, TOTAL, or WHOLE?

IMHO, I don't have strong opinion about SCAN_ALL as long as we have
document about that option and option name doesn't confuse users.
But ISTM that COMPLETE, TOTAL might make users mislead normal vacuum
as it doesn't do that completely.

Regards,

--
Masahiko Sawada

On 05/17/2016 12:32 PM, Alvaro Herrera wrote:

> Syntaxes are;
> VACUUM (SCAN_ALL) table_name;
> VACUUM (SCAN_ALL); -- for all tables on database
>
> Is SCAN_ALL really the best we can do here? The business of having an
> underscore in an option name has no precedent (other than
> CURRENT_DATABASE and the like). How about COMPLETE, TOTAL, or WHOLE?
>

VACUUM (ANALYZE, VERBOSE, WHOLE)
....

That seems reasonable? I agree that SCAN_ALL doesn't fit. I am not
trying to pull a left turn but is there a technical reason we don't just
make FULL do this?

JD

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

On Tue, May 17, 2016 at 4:34 PM, Joshua D. Drake <jd(at)commandprompt(dot)com> wrote:
> On 05/17/2016 12:32 PM, Alvaro Herrera wrote:
>
>> Syntaxes are;
>> VACUUM (SCAN_ALL) table_name;
>> VACUUM (SCAN_ALL); -- for all tables on database
>>
>> Is SCAN_ALL really the best we can do here? The business of having an
>> underscore in an option name has no precedent (other than
>> CURRENT_DATABASE and the like). How about COMPLETE, TOTAL, or WHOLE?
>>
>
> VACUUM (ANALYZE, VERBOSE, WHOLE)
> ....
>
> That seems reasonable? I agree that SCAN_ALL doesn't fit. I am not trying to
> pull a left turn but is there a technical reason we don't just make FULL do
> this?
>

FULL option requires AccessExclusiveLock, which could be a problem.

Regards,

--
Masahiko Sawada

On 17/05/16 21:32, Alvaro Herrera wrote:
> Is SCAN_ALL really the best we can do here? The business of having an
> underscore in an option name has no precedent (other than
> CURRENT_DATABASE and the like).

ALTER DATABASE has options for ALLOW_CONNECTIONS, CONNECTION_LIMIT, and
IS_TEMPLATE.

> How about COMPLETE, TOTAL, or WHOLE?

Sure, I'll play this game. I like EXHAUSTIVE.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On 18/05/16 09:34, Vik Fearing wrote:
> On 17/05/16 21:32, Alvaro Herrera wrote:
>> Is SCAN_ALL really the best we can do here? The business of having an
>> underscore in an option name has no precedent (other than
>> CURRENT_DATABASE and the like).
> ALTER DATABASE has options for ALLOW_CONNECTIONS, CONNECTION_LIMIT, and
> IS_TEMPLATE.
>
>> How about COMPLETE, TOTAL, or WHOLE?
> Sure, I'll play this game. I like EXHAUSTIVE.

I prefer 'WHOLE', as it seems more obvious (and not because of the pun
relating to 'wholesomeness'!!!)

On Tue, May 17, 2016 at 5:47 PM, Gavin Flower
<GavinFlower(at)archidevsys(dot)co(dot)nz> wrote:
> On 18/05/16 09:34, Vik Fearing wrote:
>> On 17/05/16 21:32, Alvaro Herrera wrote:
>>>
>>> Is SCAN_ALL really the best we can do here? The business of having an
>>> underscore in an option name has no precedent (other than
>>> CURRENT_DATABASE and the like).
>>
>> ALTER DATABASE has options for ALLOW_CONNECTIONS, CONNECTION_LIMIT, and
>> IS_TEMPLATE.
>>
>>> How about COMPLETE, TOTAL, or WHOLE?
>>
>> Sure, I'll play this game. I like EXHAUSTIVE.
>
> I prefer 'WHOLE', as it seems more obvious (and not because of the pun
> relating to 'wholesomeness'!!!)

I think that users might believe that they need VACUUM (WHOLE) a lot
more often than they will actually need this option. "Of course I
want to vacuum my whole table!"

I think we should give this a name that hints more strongly at this
being an exceptional thing, like vacuum (even_frozen_pages).

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

On 5/18/16 6:37 AM, Robert Haas wrote:
> On Tue, May 17, 2016 at 5:47 PM, Gavin Flower
> <GavinFlower(at)archidevsys(dot)co(dot)nz> wrote:
>> On 18/05/16 09:34, Vik Fearing wrote:
>>> On 17/05/16 21:32, Alvaro Herrera wrote:
>>>>
>>>> Is SCAN_ALL really the best we can do here? The business of having an
>>>> underscore in an option name has no precedent (other than
>>>> CURRENT_DATABASE and the like).
>>>
>>> ALTER DATABASE has options for ALLOW_CONNECTIONS, CONNECTION_LIMIT, and
>>> IS_TEMPLATE.
>>>
>>>> How about COMPLETE, TOTAL, or WHOLE?
>>>
>>> Sure, I'll play this game. I like EXHAUSTIVE.
>>
>> I prefer 'WHOLE', as it seems more obvious (and not because of the pun
>> relating to 'wholesomeness'!!!)
>
> I think that users might believe that they need VACUUM (WHOLE) a lot
> more often than they will actually need this option. "Of course I
> want to vacuum my whole table!"
>
> I think we should give this a name that hints more strongly at this
> being an exceptional thing, like vacuum (even_frozen_pages).

How about just FROZEN? Perhaps it's too confusing to have that and
FREEZE, but I thought I would throw it out there.

--
-David
david(at)pgmasters(dot)net

On Wed, May 18, 2016 at 8:41 AM, David Steele <david(at)pgmasters(dot)net> wrote:
>> I think we should give this a name that hints more strongly at this
>> being an exceptional thing, like vacuum (even_frozen_pages).
>
> How about just FROZEN? Perhaps it's too confusing to have that and FREEZE,
> but I thought I would throw it out there.

It's not a bad thought, but I do think it might be a bit confusing.
My main priority for this new option is that people aren't tempted to
use it very often, and I think a name like "even_frozen_pages" is more
likely to accomplish that than just "frozen".

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

On 05/18/2016 05:51 AM, Robert Haas wrote:
> On Wed, May 18, 2016 at 8:41 AM, David Steele <david(at)pgmasters(dot)net> wrote:
>>> I think we should give this a name that hints more strongly at this
>>> being an exceptional thing, like vacuum (even_frozen_pages).
>>
>> How about just FROZEN? Perhaps it's too confusing to have that and FREEZE,
>> but I thought I would throw it out there.
>
> It's not a bad thought, but I do think it might be a bit confusing.
> My main priority for this new option is that people aren't tempted to
> use it very often, and I think a name like "even_frozen_pages" is more
> likely to accomplish that than just "frozen".
>

freeze_all_pages?

JD

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

On Wed, May 18, 2016 at 9:42 AM, Joshua D. Drake <jd(at)commandprompt(dot)com> wrote:
>> It's not a bad thought, but I do think it might be a bit confusing.
>> My main priority for this new option is that people aren't tempted to
>> use it very often, and I think a name like "even_frozen_pages" is more
>> likely to accomplish that than just "frozen".
>
> freeze_all_pages?

No, that's what the existing FREEZE option does. This new option is
about unnecessarily vacuuming pages that don't need it. The
expectation is that vacuuming all-frozen pages will be a no-op.

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

2016-05-18 16:45 GMT+03:00 Robert Haas <robertmhaas(at)gmail(dot)com>:

> No, that's what the existing FREEZE option does. This new option is
> about unnecessarily vacuuming pages that don't need it. The
> expectation is that vacuuming all-frozen pages will be a no-op.
>

VACUUM (INCLUDING ALL) ?

--
Victor Y. Yegorov

On 05/18/2016 09:55 AM, Victor Yegorov wrote:
> 2016-05-18 16:45 GMT+03:00 Robert Haas <robertmhaas(at)gmail(dot)com
> <mailto:robertmhaas(at)gmail(dot)com>>:
>
> No, that's what the existing FREEZE option does. This new option is
> about unnecessarily vacuuming pages that don't need it. The
> expectation is that vacuuming all-frozen pages will be a no-op.
>
>
> VACUUM (INCLUDING ALL) ?

VACUUM (FORCE ALL) ?

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Wed, May 18, 2016 at 7:09 AM, Joe Conway <mail(at)joeconway(dot)com> wrote:
> On 05/18/2016 09:55 AM, Victor Yegorov wrote:
>> 2016-05-18 16:45 GMT+03:00 Robert Haas <robertmhaas(at)gmail(dot)com
>> <mailto:robertmhaas(at)gmail(dot)com>>:
>>
>> No, that's what the existing FREEZE option does. This new option is
>> about unnecessarily vacuuming pages that don't need it. The
>> expectation is that vacuuming all-frozen pages will be a no-op.
>>
>>
>> VACUUM (INCLUDING ALL) ?
>
> VACUUM (FORCE ALL) ?

How about going with something that says more about why we are doing
it, rather than trying to describe in one or two words what it is
doing?

VACUUM (FORENSIC)

VACUUM (DEBUG)

VACUUM (LINT)

Cheers,

Jeff

On Wed, May 18, 2016 at 8:52 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> How about going with something that says more about why we are doing
> it, rather than trying to describe in one or two words what it is
> doing?
>
> VACUUM (FORENSIC)
>
> VACUUM (DEBUG)
>
> VACUUM (LINT)

+1

--
Peter Geoghegan

On 05/18/2016 03:51 PM, Peter Geoghegan wrote:
> On Wed, May 18, 2016 at 8:52 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>> How about going with something that says more about why we are doing
>> it, rather than trying to describe in one or two words what it is
>> doing?
>>
>> VACUUM (FORENSIC)
>>
>> VACUUM (DEBUG)
>>
>> VACUUM (LINT)
>
> +1

Maybe this is the wrong perspective. I mean, is there a reason we even
need this option, other than a lack of any other way to do a full table
scan to check for corruption, etc.? If we're only doing this for
integrity checking, then maybe it's better if it becomes a function,
which could be later extended with additional forensic features?

--
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

Josh berkus <josh(at)agliodbs(dot)com> writes:
> Maybe this is the wrong perspective. I mean, is there a reason we even
> need this option, other than a lack of any other way to do a full table
> scan to check for corruption, etc.? If we're only doing this for
> integrity checking, then maybe it's better if it becomes a function,
> which could be later extended with additional forensic features?

Yes, I've been wondering that too. VACUUM is not meant as a corruption
checker, and should not be made into one, so what is the point of this
flag exactly?

(AFAIK, "select count(*) from table" would offer a similar amount of
sanity checking as a full-table VACUUM scan does, so it's not like
we've removed functionality with no near-term replacement.)

regards, tom lane

On 2016-05-18 18:25:39 -0400, Tom Lane wrote:
> Josh berkus <josh(at)agliodbs(dot)com> writes:
> > Maybe this is the wrong perspective. I mean, is there a reason we even
> > need this option, other than a lack of any other way to do a full table
> > scan to check for corruption, etc.? If we're only doing this for
> > integrity checking, then maybe it's better if it becomes a function,
> > which could be later extended with additional forensic features?
>
> Yes, I've been wondering that too. VACUUM is not meant as a corruption
> checker, and should not be made into one, so what is the point of this
> flag exactly?

Well, so far a VACUUM FREEZE (or just setting vacuum_freeze_table_age =
0) verified the correctness of the visibility map; and that found a
number of bugs. Now visibilitymap grew additional responsibilities,
with a noticeable risk of data eating bugs, and there's no way to verify
whether visibilitymap's frozen bits are set correctly.

> (AFAIK, "select count(*) from table" would offer a similar amount of
> sanity checking as a full-table VACUUM scan does, so it's not like
> we've removed functionality with no near-term replacement.)

I don't think that'd do anything comparable to
/*
* As of PostgreSQL 9.2, the visibility map bit should never be set if
* the page-level bit is clear. However, it's possible that the bit
* got cleared after we checked it and before we took the buffer
* content lock, so we must recheck before jumping to the conclusion
* that something bad has happened.
*/
else if (all_visible_according_to_vm && !PageIsAllVisible(page)
&& VM_ALL_VISIBLE(onerel, blkno, &vmbuffer))
{
elog(WARNING, "page is not marked all-visible but visibility map bit is set in relation \"%s\" page %u",
relname, blkno);
visibilitymap_clear(onerel, blkno, vmbuffer);
}

If we had a checking module for all this it'd possibly be sufficient,
but we don't.

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-05-18 18:25:39 -0400, Tom Lane wrote:
>> Yes, I've been wondering that too. VACUUM is not meant as a corruption
>> checker, and should not be made into one, so what is the point of this
>> flag exactly?

> Well, so far a VACUUM FREEZE (or just setting vacuum_freeze_table_age =
> 0) verified the correctness of the visibility map; and that found a
> number of bugs. Now visibilitymap grew additional responsibilities,
> with a noticeable risk of data eating bugs, and there's no way to verify
> whether visibilitymap's frozen bits are set correctly.

Meh. I'm not sure we should grow a rather half-baked feature we'll never
be able to remove as a substitute for a separate sanity checker. The
latter is really the right place for this kind of thing.

regards, tom lane

On 2016-05-18 18:42:16 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2016-05-18 18:25:39 -0400, Tom Lane wrote:
> >> Yes, I've been wondering that too. VACUUM is not meant as a corruption
> >> checker, and should not be made into one, so what is the point of this
> >> flag exactly?
>
> > Well, so far a VACUUM FREEZE (or just setting vacuum_freeze_table_age =
> > 0) verified the correctness of the visibility map; and that found a
> > number of bugs. Now visibilitymap grew additional responsibilities,
> > with a noticeable risk of data eating bugs, and there's no way to verify
> > whether visibilitymap's frozen bits are set correctly.
>
> Meh. I'm not sure we should grow a rather half-baked feature we'll never
> be able to remove as a substitute for a separate sanity checker. The
> latter is really the right place for this kind of thing.

It's not a new feature, it's a feature we removed as a side effect. And
one that allows us to evaluate whether the new feature actually works.

Andres Freund wrote:

> > (AFAIK, "select count(*) from table" would offer a similar amount of
> > sanity checking as a full-table VACUUM scan does, so it's not like
> > we've removed functionality with no near-term replacement.)
>
> I don't think that'd do anything comparable to
> /*
> * As of PostgreSQL 9.2, the visibility map bit should never be set if
> * the page-level bit is clear. However, it's possible that the bit
> * got cleared after we checked it and before we took the buffer
> * content lock, so we must recheck before jumping to the conclusion
> * that something bad has happened.
> */
> else if (all_visible_according_to_vm && !PageIsAllVisible(page)
> && VM_ALL_VISIBLE(onerel, blkno, &vmbuffer))
> {
> elog(WARNING, "page is not marked all-visible but visibility map bit is set in relation \"%s\" page %u",
> relname, blkno);
> visibilitymap_clear(onerel, blkno, vmbuffer);
> }
>
> If we had a checking module for all this it'd possibly be sufficient,
> but we don't.

Here's an idea. We need core-blessed extensions (src/extensions/, you
know I've proposed this before), so why not take this opportunity to
create our first such and make it carry a function to scan a table
completely to do this task.

Since we were considering a new VACUUM option, surely this is serious
enough to warrant more than just contrib.

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

On Wed, May 18, 2016 at 3:57 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Since we were considering a new VACUUM option, surely this is serious
> enough to warrant more than just contrib.

I would like to see us consider the long-term best place for amcheck's
functionality at the same time. Ideally, verification would be a
somewhat generic operation, with AM-specific code invoked as
appropriate.

--
Peter Geoghegan

On Fri, May 06, 2016 at 04:42:48PM -0400, Robert Haas wrote:
> On Thu, May 5, 2016 at 2:20 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
> > + char new_vmbuf[BLCKSZ];
> > + char *new_cur = new_vmbuf;
> > + bool empty = true;
> > + bool old_lastpart;
> > +
> > + /* Copy page header in advance */
> > + memcpy(new_vmbuf, &pageheader, SizeOfPageHeaderData);
> >
> > Shouldn't we zero out new_vmbuf? Afaics we're not necessarily zeroing it
> > with old_lastpart && !empty, right?
>
> Oh, dear. That seems like a possible data corruption bug. Maybe we'd
> better fix that right away (although I don't actually have time before
> the wrap).

[This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item. Robert,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
9.6 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1] and send a status update within 72 hours of this
message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping 9.6rc1. Consequently, I will appreciate your
efforts toward speedy resolution. Thanks.

[1] http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com

On Sun, May 29, 2016 at 2:44 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Fri, May 06, 2016 at 04:42:48PM -0400, Robert Haas wrote:
>> On Thu, May 5, 2016 at 2:20 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> > On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
>> > + char new_vmbuf[BLCKSZ];
>> > + char *new_cur = new_vmbuf;
>> > + bool empty = true;
>> > + bool old_lastpart;
>> > +
>> > + /* Copy page header in advance */
>> > + memcpy(new_vmbuf, &pageheader, SizeOfPageHeaderData);
>> >
>> > Shouldn't we zero out new_vmbuf? Afaics we're not necessarily zeroing it
>> > with old_lastpart && !empty, right?
>>
>> Oh, dear. That seems like a possible data corruption bug. Maybe we'd
>> better fix that right away (although I don't actually have time before
>> the wrap).
>
> [This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 9.6 open item. Robert,
> since you committed the patch believed to have created it, you own this open
> item. If some other commit is more relevant or if this does not belong as a
> 9.6 open item, please let us know. Otherwise, please observe the policy on
> open item ownership[1] and send a status update within 72 hours of this
> message. Include a date for your subsequent status update. Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping 9.6rc1. Consequently, I will appreciate your
> efforts toward speedy resolution. Thanks.
>
> [1] http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com

Thank you for notification.

Regarding check tool for visibility map is still under the discussion.
I'm going to address other review comments, and send the patch ASAP.

Regards,

--
Masahiko Sawada

On Wed, May 18, 2016 at 3:57 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Andres Freund wrote:
>
>>
>> If we had a checking module for all this it'd possibly be sufficient,
>> but we don't.
>
> Here's an idea. We need core-blessed extensions (src/extensions/, you
> know I've proposed this before), so why not take this opportunity to
> create our first such and make it carry a function to scan a table
> completely to do this task.
>
> Since we were considering a new VACUUM option, surely this is serious
> enough to warrant more than just contrib.

What does "core-blessed" mean? The commit rights for contrib/ are the
same as they are for src/

Cheers,

Jeff

On Tue, May 31, 2016 at 4:40 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> On Wed, May 18, 2016 at 3:57 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
>> Andres Freund wrote:
>>
>>>
>>> If we had a checking module for all this it'd possibly be sufficient,
>>> but we don't.
>>
>> Here's an idea. We need core-blessed extensions (src/extensions/, you
>> know I've proposed this before), so why not take this opportunity to
>> create our first such and make it carry a function to scan a table
>> completely to do this task.
>>
>> Since we were considering a new VACUUM option, surely this is serious
>> enough to warrant more than just contrib.
>
> What does "core-blessed" mean? The commit rights for contrib/ are the
> same as they are for src/

Personally I understand contrib/ modules as third-part plugins that
are considered as not enough mature to be part of src/backend or
src/bin, but one day they could become so. See pg_upgrade's recent
move for example. src/extensions/ includes third-part plugins that are
thought as useful, are part of the main server package, but are not
something that we want to enable by default.
--
Michael

On Sun, May 29, 2016 at 1:44 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Fri, May 06, 2016 at 04:42:48PM -0400, Robert Haas wrote:
>> On Thu, May 5, 2016 at 2:20 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> > On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
>> > + char new_vmbuf[BLCKSZ];
>> > + char *new_cur = new_vmbuf;
>> > + bool empty = true;
>> > + bool old_lastpart;
>> > +
>> > + /* Copy page header in advance */
>> > + memcpy(new_vmbuf, &pageheader, SizeOfPageHeaderData);
>> >
>> > Shouldn't we zero out new_vmbuf? Afaics we're not necessarily zeroing it
>> > with old_lastpart && !empty, right?
>>
>> Oh, dear. That seems like a possible data corruption bug. Maybe we'd
>> better fix that right away (although I don't actually have time before
>> the wrap).
>
> [This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 9.6 open item. Robert,
> since you committed the patch believed to have created it, you own this open
> item. If some other commit is more relevant or if this does not belong as a
> 9.6 open item, please let us know. Otherwise, please observe the policy on
> open item ownership[1] and send a status update within 72 hours of this
> message. Include a date for your subsequent status update. Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping 9.6rc1. Consequently, I will appreciate your
> efforts toward speedy resolution. Thanks.

I am going to try to find time to look at this later this week, but
realistically it's going to be a little bit difficult to find that
time. I was away over Memorial Day weekend and was in meetings most
of today. I have a huge pile of email to catch up on. I will send
another status update no later than Friday. If Andres or anyone else
wants to jump in and fix this up meanwhile, that would be great.

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

On Sat, May 7, 2016 at 5:34 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, May 2, 2016 at 8:25 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> + * heap_tuple_needs_eventual_freeze
>> + *
>> + * Check to see whether any of the XID fields of a tuple (xmin, xmax, xvac)
>> + * will eventually require freezing. Similar to heap_tuple_needs_freeze,
>> + * but there's no cutoff, since we're trying to figure out whether freezing
>> + * will ever be needed, not whether it's needed now.
>> + */
>> +bool
>> +heap_tuple_needs_eventual_freeze(HeapTupleHeader tuple)
>>
>> Wouldn't redefining this to heap_tuple_is_frozen() and then inverting the
>> checks be easier to understand?
>
> I thought it much safer to keep this as close to a copy of
> heap_tuple_needs_freeze() as possible. Copying a function and
> inverting all of the return values is much more likely to introduce
> bugs, IME.

I agree.

>> + /*
>> + * If xmax is a valid xact or multixact, this tuple is also not frozen.
>> + */
>> + if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
>> + {
>> + MultiXactId multi;
>> +
>> + multi = HeapTupleHeaderGetRawXmax(tuple);
>> + if (MultiXactIdIsValid(multi))
>> + return true;
>> + }
>>
>> Hm. What's the test inside the if() for? There shouldn't be any case
>> where xmax is invalid if HEAP_XMAX_IS_MULTI is set. Now there's a
>> check like that outside of this commit, but it seems strange to me
>> (Alvaro, perhaps you could comment on this?).
>
> Here again I was copying existing code, with appropriate simplifications.
>
>> + *
>> + * Clearing both visibility map bits is not separately WAL-logged. The callers
>> * must make sure that whenever a bit is cleared, the bit is cleared on WAL
>> * replay of the updating operation as well.
>>
>> I think including "both" here makes things less clear, because it
>> differentiates clearing one bit from clearing both. There's no practical
>> differentce atm, but still.
>
> I agree.

Fixed.

>> *
>> * VACUUM will normally skip pages for which the visibility map bit is set;
>> * such pages can't contain any dead tuples and therefore don't need vacuuming.
>> - * The visibility map is not used for anti-wraparound vacuums, because
>> - * an anti-wraparound vacuum needs to freeze tuples and observe the latest xid
>> - * present in the table, even on pages that don't have any dead tuples.
>> *
>>
>> I think the remaining sentence isn't entirely accurate, there's now more
>> than one bit, and they're different with regard to scan_all/!scan_all
>> vacuums (or will be - maybe this updated further in a later commit? But
>> if so, that sentence shouldn't yet be removed...).
>
> We can adjust the language, but I don't really see a big problem here.

This comment is not incorporate this patch so far.

>> -/* Number of heap blocks we can represent in one byte. */
>> -#define HEAPBLOCKS_PER_BYTE 8
>> -
>> Hm, why was this moved to the header? Sounds like something the outside
>> shouldn't care about.
>
> Oh... yeah. Let's undo that.

Fixed.

>> #define HEAPBLK_TO_MAPBIT(x) (((x) % HEAPBLOCKS_PER_BYTE) * BITS_PER_HEAPBLOCK)
>>
>> Hm. This isn't really a mapping to an individual bit anymore - but I
>> don't really have a better name in mind. Maybe TO_OFFSET?
>
> Well, it sorta is... but we could change it, I suppose.
>
>> +static const uint8 number_of_ones_for_visible[256] = {
>> ...
>> +};
>> +static const uint8 number_of_ones_for_frozen[256] = {
>> ...
>> };
>>
>> Did somebody verify the new contents are correct?
>
> I admit that I didn't. It seemed like an unlikely place for a goof,
> but I guess we should verify.
>> /*
>> - * visibilitymap_clear - clear a bit in visibility map
>> + * visibilitymap_clear - clear all bits in visibility map
>> *
>>
>> This seems rather easy to misunderstand, as this really only clears all
>> the bits for one page, not actually all the bits.
>
> We could change "in" to "for one page in the".

Fixed.

>> * the bit for heapBlk, or InvalidBuffer. The caller is responsible for
>> - * releasing *buf after it's done testing and setting bits.
>> + * releasing *buf after it's done testing and setting bits, and must pass flags
>> + * for which it needs to check the value in visibility map.
>> *
>> * NOTE: This function is typically called without a lock on the heap page,
>> * so somebody else could change the bit just after we look at it. In fact,
>> @@ -327,17 +351,16 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
>>
>> I'm not seing what flags the above comment change is referring to?
>
> Ugh. I think that's leftover cruft from an earlier patch version that
> should have been excised from what got committed.

Fixed.

>> /*
>> - * A single-bit read is atomic. There could be memory-ordering effects
>> + * A single byte read is atomic. There could be memory-ordering effects
>> * here, but for performance reasons we make it the caller's job to worry
>> * about that.
>> */
>> - result = (map[mapByte] & (1 << mapBit)) ? true : false;
>> -
>> - return result;
>> + return ((map[mapByte] >> mapBit) & VISIBILITYMAP_VALID_BITS);
>> }
>>
>> Not a new issue, and *very* likely to be irrelevant in practice (given
>> the value is only referenced once): But there's really no guarantee
>> map[mapByte] is only read once here.
>
> Meh. But we can fix if you want to.

Fixed.

>> -BlockNumber
>> -visibilitymap_count(Relation rel)
>> +void
>> +visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_frozen)
>>
>> Not really a new issue again: The parameter types (previously return
>> type) to this function seem wrong to me.
>
> Not this patch's job to tinker.

This comment is not incorporate this patch yet.

>> @@ -1934,5 +1992,14 @@ heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId *visibility_cut
>> }
>> + /*
>> + * We don't bother clearing *all_frozen when the page is discovered not
>> + * to be all-visible, so do that now if necessary. The page might fail
>> + * to be all-frozen for other reasons anyway, but if it's not all-visible,
>> + * then it definitely isn't all-frozen.
>> + */
>> + if (!all_visible)
>> + *all_frozen = false;
>> +
>>
>> Why don't we just set *all_frozen to false when appropriate? It'd be
>> just as many lines and probably easier to understand?
>
> I thought that looked really easy to mess up, either now or down the
> road. This way seemed more solid to me. That's a judgement call, of
> course.

To be understanding easier, I changed it so.

>> + /*
>> + * If the page is marked as all-visible but not all-frozen, we should
>> + * so mark it. Note that all_frozen is only valid if all_visible is
>> + * true, so we must check both.
>> + */
>>
>> This kinda seems to imply that all-visible implies all_frozen. Also, why
>> has that block been added to the end of the if/else if chain? Seems like
>> it belongs below the (all_visible && !all_visible_according_to_vm) block.
>
> We can adjust the comment a bit to make it more clear, if you like,
> but I doubt it's going to cause serious misunderstanding. As for the
> placement, the reason I put it at the end is because I figured that we
> did not want to mark it all-frozen if any of the "oh crap, emit a
> warning" cases applied.
>

Fixed comment.
I think that we should care about all-visible problem first, and then
care all-frozen problem.
So this patch doesn't change the placement.

Attached patch fixes only above comments, other are being addressed now.

--
Regards,

--
Masahiko Sawada

On Sat, May 7, 2016 at 5:40 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, May 4, 2016 at 8:08 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
>>> 77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.
>>
>> Nothing to say here.
>>
>>> fd31cd2 Don't vacuum all-frozen pages.
>>
>> Hm. I do wonder if it's going to bite us that we don't have a way to
>> actually force vacuuming of the whole table (besides manually rm'ing the
>> VM). I've more than once seen VACUUM used to try to do some integrity
>> checking of the database. How are we actually going to test that the
>> feature works correctly? They'd have to write checks ontop of
>> pg_visibility to see whether things are borked.
>
> Let's add VACUUM (FORCE) or something like that.
>
>> /*
>> * Compute whether we actually scanned the whole relation. If we did, we
>> * can adjust relfrozenxid and relminmxid.
>> *
>> * NB: We need to check this before truncating the relation, because that
>> * will change ->rel_pages.
>> */
>>
>> Comment is out-of-date now.
>
> OK.

Fixed.

>> - if (blkno == next_not_all_visible_block)
>> + if (blkno == next_unskippable_block)
>> {
>> - /* Time to advance next_not_all_visible_block */
>> - for (next_not_all_visible_block++;
>> - next_not_all_visible_block < nblocks;
>> - next_not_all_visible_block++)
>> + /* Time to advance next_unskippable_block */
>> + for (next_unskippable_block++;
>> + next_unskippable_block < nblocks;
>> + next_unskippable_block++)
>>
>> Hm. So we continue with the course of re-processing pages, even if
>> they're marked all-frozen. For all-visible there at least can be a
>> benefit by freezing earlier, but for all-frozen pages there's really no
>> point. I don't really buy the arguments for the skipping logic. But
>> even disregarding that, maybe we should skip processing a block if it's
>> all-frozen (without preventing the page from being read?); as there's no
>> possible benefit? Acquring the exclusive/content lock and stuff is far
>> from free.
>
> I wanted to tinker with this logic as little as possible in the
> interest of ending up with something that worked. I would not have
> written it this way.
>
>> Not really related to this patch, but the FORCE_CHECK_PAGE is rather
>> ugly.
>
> +1.
>> + /*
>> + * The current block is potentially skippable; if we've seen a
>> + * long enough run of skippable blocks to justify skipping it, and
>> + * we're not forced to check it, then go ahead and skip.
>> + * Otherwise, the page must be at least all-visible if not
>> + * all-frozen, so we can set all_visible_according_to_vm = true.
>> + */
>> + if (skipping_blocks && !FORCE_CHECK_PAGE())
>> + {
>> + /*
>> + * Tricky, tricky. If this is in aggressive vacuum, the page
>> + * must have been all-frozen at the time we checked whether it
>> + * was skippable, but it might not be any more. We must be
>> + * careful to count it as a skipped all-frozen page in that
>> + * case, or else we'll think we can't update relfrozenxid and
>> + * relminmxid. If it's not an aggressive vacuum, we don't
>> + * know whether it was all-frozen, so we have to recheck; but
>> + * in this case an approximate answer is OK.
>> + */
>> + if (aggressive || VM_ALL_FROZEN(onerel, blkno, &vmbuffer))
>> + vacrelstats->frozenskipped_pages++;
>> continue;
>> + }
>>
>> Hm. This indeed seems a bit tricky. Not sure how to make it easier
>> though without just ripping out the SKIP_PAGES_THRESHOLD stuff.
>
> Yep, I had the same problem.
>> Hm. This also doubles the number of VM accesses. While I guess that's
>> not noticeable most of the time, it's still not nice; especially when a
>> large relation is entirely frozen, because it'll mean we'll sequentially
>> go through the visibilityma twice.
>
> Compared to what we're saving, that's obviously a trivial cost.
> That's not to say that we might not want to improve it, but it's
> hardly a disaster.
>
> In short: wah, wah, wah.
>

Attached patch optimises skipping pages logic so that blkno can jump to
next_unskippable_block directly while counting the number of all_visible
and all_frozen pages. So we can avoid double checking visibility map.

Regards,

--
Masahiko Sawada

On Wed, Jun 1, 2016 at 3:50 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> Attached patch fixes only above comments, other are being addressed now.

Committed.

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

On Thu, Jun 2, 2016 at 11:24 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> Attached patch optimises skipping pages logic so that blkno can jump to
> next_unskippable_block directly while counting the number of all_visible
> and all_frozen pages. So we can avoid double checking visibility map.

I think this is 9.7 material. This patch has already won the
"scariest patch" tournament. Changing the logic more than necessary
at this late date seems like it just increases the scariness. I think
this is an opportunity for further optimization, not a defect.

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

On Fri, Jun 3, 2016 at 11:03 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jun 2, 2016 at 11:24 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> Attached patch optimises skipping pages logic so that blkno can jump to
>> next_unskippable_block directly while counting the number of all_visible
>> and all_frozen pages. So we can avoid double checking visibility map.
>
> I think this is 9.7 material. This patch has already won the
> "scariest patch" tournament. Changing the logic more than necessary
> at this late date seems like it just increases the scariness. I think
> this is an opportunity for further optimization, not a defect.
>

I agree with you.
I'll submit this as a improvement for 9.7.
That patch also incorporates the following review comment.
We can push at least this fix.
>> /*
>> * Compute whether we actually scanned the whole relation. If we did, we
>> * can adjust relfrozenxid and relminmxid.
>> *
>> * NB: We need to check this before truncating the relation, because that
>> * will change ->rel_pages.
>> */
>>
>> Comment is out-of-date now.

I'm address the review comment of 7087166 commit, and will post the patch.
And testing feature for freeze map is under the discussion.

Regards,

--
Masahiko Sawada

On Fri, Jun 3, 2016 at 10:49 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> That patch also incorporates the following review comment.
> We can push at least this fix.

Can you submit that part as a separate patch?

> I'm address the review comment of 7087166 commit, and will post the patch.

When?

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

On Sat, Jun 4, 2016 at 12:08 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jun 3, 2016 at 10:49 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> That patch also incorporates the following review comment.
>> We can push at least this fix.
>
> Can you submit that part as a separate patch?

Attached.

>> I'm address the review comment of 7087166 commit, and will post the patch.
>
> When?
>

On Saturday.

Regards,

--
Masahiko Sawada

On Fri, Jun 3, 2016 at 11:21 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> Can you submit that part as a separate patch?
>
> Attached.

Thanks, committed.

>>> I'm address the review comment of 7087166 commit, and will post the patch.
>>
>> When?
>
> On Saturday.

Great. Will that address everything for this open item, then?

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

On Sat, May 7, 2016 at 5:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, May 5, 2016 at 2:20 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
>>> 7087166 pg_upgrade: Convert old visibility map format to new format.
>>
>> +const char *
>> +rewriteVisibilityMap(const char *fromfile, const char *tofile, bool force)
>> ...
>>
>> + while ((bytesRead = read(src_fd, buffer, BLCKSZ)) == BLCKSZ)
>> + {
>> ..
>>
>> Uh, shouldn't we actually fail if we read incompletely? Rather than
>> silently ignoring the problem? Ok, this causes no corruption, but it
>> indicates that something went significantly wrong.
>
> Sure, that's reasonable.
>

Fixed.

>> + char new_vmbuf[BLCKSZ];
>> + char *new_cur = new_vmbuf;
>> + bool empty = true;
>> + bool old_lastpart;
>> +
>> + /* Copy page header in advance */
>> + memcpy(new_vmbuf, &pageheader, SizeOfPageHeaderData);
>>
>> Shouldn't we zero out new_vmbuf? Afaics we're not necessarily zeroing it
>> with old_lastpart && !empty, right?
>
> Oh, dear. That seems like a possible data corruption bug. Maybe we'd
> better fix that right away (although I don't actually have time before
> the wrap).

Since the force is always set true, I removed the force from argument
of copyFile() and rewriteVisibilityMap().
And destination file is always opened with O_RDWR, O_CREAT, O_TRUNC flags .

>> + if ((dst_fd = open(tofile, O_RDWR | O_CREAT | (force ? 0 : O_EXCL), S_IRUSR | S_IWUSR)) < 0)
>> + {
>> + close(src_fd);
>> + return getErrorText();
>> + }
>>
>> I know you guys copied this, but what's the force thing about?
>> Expecially as it's always set to true by the callers (i.e. what is the
>> parameter even about?)? Wouldn't we at least have to specify O_TRUNC in
>> the force case?
>
> I just work here.
>
>> + old_cur += BITS_PER_HEAPBLOCK_OLD;
>> + new_cur += BITS_PER_HEAPBLOCK;
>>
>> I'm not sure I'm understanding the point of the BITS_PER_HEAPBLOCK_OLD
>> stuff - as long as it's hardcoded into rewriteVisibilityMap() we'll not
>> be able to have differing ones anyway, should we decide to add a third
>> bit?
>
> I think that's just a matter of style.

So this comments is not incorporated.

Attached patch, please review it.

Regards,

--
Masahiko Sawada

On Fri, Jun 3, 2016 at 10:25 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> + char new_vmbuf[BLCKSZ];
>>> + char *new_cur = new_vmbuf;
>>> + bool empty = true;
>>> + bool old_lastpart;
>>> +
>>> + /* Copy page header in advance */
>>> + memcpy(new_vmbuf, &pageheader, SizeOfPageHeaderData);
>>>
>>> Shouldn't we zero out new_vmbuf? Afaics we're not necessarily zeroing it
>>> with old_lastpart && !empty, right?
>>
>> Oh, dear. That seems like a possible data corruption bug. Maybe we'd
>> better fix that right away (although I don't actually have time before
>> the wrap).

Actually, on second thought, I'm not seeing the bug here. It seems to
me that the loop commented this way:

/* Process old page bytes one by one, and turn it into new page. */

...should always write to every byte in new_vmbuf, because we process
exactly half the bytes in the old block at a time, and so that's going
to generate exactly one full page of new bytes. Am I missing
something?

> Since the force is always set true, I removed the force from argument
> of copyFile() and rewriteVisibilityMap().
> And destination file is always opened with O_RDWR, O_CREAT, O_TRUNC flags .

I'm not happy with this. I think we should always open with O_EXCL,
because the new file is not expected to exist and if it does,
something's probably broken. I think we should default to the safe
behavior (which is failing) rather than the unsafe behavior (which is
clobbering data).

(Status update for Noah: I expect Masahiko Sawada will respond
quickly, but if not I'll give some kind of update by Monday COB
anyhow.)

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

On Sat, Jun 4, 2016 at 12:41 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jun 3, 2016 at 10:25 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>> + char new_vmbuf[BLCKSZ];
>>>> + char *new_cur = new_vmbuf;
>>>> + bool empty = true;
>>>> + bool old_lastpart;
>>>> +
>>>> + /* Copy page header in advance */
>>>> + memcpy(new_vmbuf, &pageheader, SizeOfPageHeaderData);
>>>>
>>>> Shouldn't we zero out new_vmbuf? Afaics we're not necessarily zeroing it
>>>> with old_lastpart && !empty, right?
>>>
>>> Oh, dear. That seems like a possible data corruption bug. Maybe we'd
>>> better fix that right away (although I don't actually have time before
>>> the wrap).
>
> Actually, on second thought, I'm not seeing the bug here. It seems to
> me that the loop commented this way:
>
> /* Process old page bytes one by one, and turn it into new page. */
>
> ...should always write to every byte in new_vmbuf, because we process
> exactly half the bytes in the old block at a time, and so that's going
> to generate exactly one full page of new bytes. Am I missing
> something?

Yeah, you're right.
the rewriteVisibilityMap() always exactly writes whole new_vmbuf.

>
>> Since the force is always set true, I removed the force from argument
>> of copyFile() and rewriteVisibilityMap().
>> And destination file is always opened with O_RDWR, O_CREAT, O_TRUNC flags .
>
> I'm not happy with this. I think we should always open with O_EXCL,
> because the new file is not expected to exist and if it does,
> something's probably broken. I think we should default to the safe
> behavior (which is failing) rather than the unsafe behavior (which is
> clobbering data).

I specified O_EXCL instead of O_TRUNC.

Attached updated patch.

Regards,

--
Masahiko Sawada

On Sat, Jun 4, 2016 at 12:59 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jun 3, 2016 at 11:21 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> Can you submit that part as a separate patch?
>>
>> Attached.
>
> Thanks, committed.
>
>>>> I'm address the review comment of 7087166 commit, and will post the patch.
>>>
>>> When?
>>
>> On Saturday.
>
> Great. Will that address everything for this open item, then?
>

Attached patch for commit 7087166 on another mail.
I think that only the test tool for visibility map is remaining and
under the discussion.
Even if we have verification tool or function for visibility map, we
cannot repair the contents of visibility map if we turned out that
contents of visibility map is something wrong.
So I think we should have the way that re-generates the visibility map.
For this purpose, doing vacuum while ignoring visibility map by a new
option or new function is one idea.
But IMHO, it's not good idea to allow a function to do vacuum, and
expanding the VACUUM syntax might be somewhat overkill.

So other idea is to have GUC parameter, vacuum_even_frozen_page for example.
If this parameter is set true (false by default), we do vacuum whole
table forcibly and re-generate visibility map.
The advantage of this idea is that we don't necessary to expand VACUUM
syntax and relatively easily can remove this parameter if it's not
necessary anymore.

Thought?

Regards,

--
Masahiko Sawada

On Sat, Jun 4, 2016 at 1:46 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Sat, Jun 4, 2016 at 12:59 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Jun 3, 2016 at 11:21 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>> Can you submit that part as a separate patch?
>>>
>>> Attached.
>>
>> Thanks, committed.
>>
>>>>> I'm address the review comment of 7087166 commit, and will post the patch.
>>>>
>>>> When?
>>>
>>> On Saturday.
>>
>> Great. Will that address everything for this open item, then?
>>
>
> Attached patch for commit 7087166 on another mail.
> I think that only the test tool for visibility map is remaining and
> under the discussion.
> Even if we have verification tool or function for visibility map, we
> cannot repair the contents of visibility map if we turned out that
> contents of visibility map is something wrong.
> So I think we should have the way that re-generates the visibility map.
> For this purpose, doing vacuum while ignoring visibility map by a new
> option or new function is one idea.
> But IMHO, it's not good idea to allow a function to do vacuum, and
> expanding the VACUUM syntax might be somewhat overkill.
>
> So other idea is to have GUC parameter, vacuum_even_frozen_page for example.
> If this parameter is set true (false by default), we do vacuum whole
> table forcibly and re-generate visibility map.
> The advantage of this idea is that we don't necessary to expand VACUUM
> syntax and relatively easily can remove this parameter if it's not
> necessary anymore.
>

Attached is a sample patch that controls full page vacuum by new GUC parameter.

Regards,

--
Masahiko Sawada

On Mon, Jun 6, 2016 at 5:44 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Sat, Jun 4, 2016 at 1:46 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Sat, Jun 4, 2016 at 12:59 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Fri, Jun 3, 2016 at 11:21 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>>> Can you submit that part as a separate patch?
>>>>
>>>> Attached.
>>>
>>> Thanks, committed.
>>>
>>>>>> I'm address the review comment of 7087166 commit, and will post the patch.
>>>>>
>>>>> When?
>>>>
>>>> On Saturday.
>>>
>>> Great. Will that address everything for this open item, then?
>>>
>>
>> Attached patch for commit 7087166 on another mail.
>> I think that only the test tool for visibility map is remaining and
>> under the discussion.
>> Even if we have verification tool or function for visibility map, we
>> cannot repair the contents of visibility map if we turned out that
>> contents of visibility map is something wrong.
>> So I think we should have the way that re-generates the visibility map.
>> For this purpose, doing vacuum while ignoring visibility map by a new
>> option or new function is one idea.
>> But IMHO, it's not good idea to allow a function to do vacuum, and
>> expanding the VACUUM syntax might be somewhat overkill.
>>
>> So other idea is to have GUC parameter, vacuum_even_frozen_page for example.
>> If this parameter is set true (false by default), we do vacuum whole
>> table forcibly and re-generate visibility map.
>> The advantage of this idea is that we don't necessary to expand VACUUM
>> syntax and relatively easily can remove this parameter if it's not
>> necessary anymore.
>>
>
> Attached is a sample patch that controls full page vacuum by new GUC parameter.

Don't we want a reloption for that? Just wondering...
--
Michael

On Mon, Jun 6, 2016 at 5:11 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>> Attached is a sample patch that controls full page vacuum by new GUC parameter.
>
> Don't we want a reloption for that? Just wondering...

Why? Just for consistency? I think the bigger question here is
whether we need to do anything at all. It's true that, without some
new option, we'll lose the ability to forcibly vacuum every page in
the relation, even if all-frozen. But there's not much use case for
that in the first place. It will be potentially helpful if it turns
out that we have a bug that sets the all-frozen bit on pages that are
not, in fact, all-frozen. Otherwise, what's the use?

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

On Mon, Jun 6, 2016 at 6:34 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jun 6, 2016 at 5:11 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> Attached is a sample patch that controls full page vacuum by new GUC parameter.
>>
>> Don't we want a reloption for that? Just wondering...
>
> Why? Just for consistency? I think the bigger question here is
> whether we need to do anything at all. It's true that, without some
> new option, we'll lose the ability to forcibly vacuum every page in
> the relation, even if all-frozen. But there's not much use case for
> that in the first place. It will be potentially helpful if it turns
> out that we have a bug that sets the all-frozen bit on pages that are
> not, in fact, all-frozen. Otherwise, what's the use?
>

I cannot agree with using this parameter as a reloption.
We set it true only when the serious bug is discovered and we want to
re-generate the visibility maps of specific tables.
I thought that control by GUC parameter would be convenient rather
than adding the new option.

Regards,

--
Masahiko Sawada

On Sat, Jun 4, 2016 at 12:18 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> Attached updated patch.

The error-checking enhancements here look good to me, except that you
forgot to initialize totalBytesRead. I've committed those changes
with a fix for that problem and will look at the rest of this
separately.

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

Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> writes:
> On Sat, Jun 4, 2016 at 1:46 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> So other idea is to have GUC parameter, vacuum_even_frozen_page for example.
>> If this parameter is set true (false by default), we do vacuum whole
>> table forcibly and re-generate visibility map.
>> The advantage of this idea is that we don't necessary to expand VACUUM
>> syntax and relatively easily can remove this parameter if it's not
>> necessary anymore.

> Attached is a sample patch that controls full page vacuum by new GUC parameter.

I find this approach fairly ugly ... it's randomly inconsistent with other
VACUUM parameters for no very defensible reason. Taking out GUCs is not
easier than taking out statement parameters; you risk breaking
applications either way.

regards, tom lane

On Mon, Jun 6, 2016 at 7:46 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Jun 4, 2016 at 12:18 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> Attached updated patch.
>
> The error-checking enhancements here look good to me, except that you
> forgot to initialize totalBytesRead. I've committed those changes
> with a fix for that problem and will look at the rest of this
> separately.

Committed that now, too.

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

On Mon, Jun 6, 2016 at 9:53 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> writes:
>> On Sat, Jun 4, 2016 at 1:46 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> So other idea is to have GUC parameter, vacuum_even_frozen_page for example.
>>> If this parameter is set true (false by default), we do vacuum whole
>>> table forcibly and re-generate visibility map.
>>> The advantage of this idea is that we don't necessary to expand VACUUM
>>> syntax and relatively easily can remove this parameter if it's not
>>> necessary anymore.
>
>> Attached is a sample patch that controls full page vacuum by new GUC parameter.
>
> I find this approach fairly ugly ... it's randomly inconsistent with other
> VACUUM parameters for no very defensible reason.

Just to be sure I understand, in what way is it inconsistent?

> Taking out GUCs is not
> easier than taking out statement parameters; you risk breaking
> applications either way.

Agreed, but that doesn't really answer the question of which one we
should have, if either. My gut feeling on this is to either do
nothing or add a VACUUM option (not a GUC, not a reloption) called
even_frozen_pages, default false. What is your opinion?

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jun 6, 2016 at 9:53 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Taking out GUCs is not
>> easier than taking out statement parameters; you risk breaking
>> applications either way.

> Agreed, but that doesn't really answer the question of which one we
> should have, if either. My gut feeling on this is to either do
> nothing or add a VACUUM option (not a GUC, not a reloption) called
> even_frozen_pages, default false. What is your opinion?

That's about where I stand, with some preference for "do nothing".
I'm not convinced we need this.

regards, tom lane

On Fri, Jun 3, 2016 at 11:41 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> (Status update for Noah: I expect Masahiko Sawada will respond
> quickly, but if not I'll give some kind of update by Monday COB
> anyhow.)

I believe this open item is now closed, unless Andres has more
comments or wishes to discuss any point further, with the exception
that we still need to decide whether to add VACUUM (even_frozen_pages)
or some variant of that. I have added a new open item for that issue
and marked this one as resolved.

My intended strategy as the presumptive owner of the new items is to
do nothing unless more of a consensus emerges than we have presently.
We do not seem to have clear agreement on whether to add the new
option; whether to make it a GUC, a reloption, a VACUUM syntax option,
or some combination of those things; and whether it should blow up the
existing VM and rebuild it (as proposed by Sawada-san) or just force
frozen pages to be scanned in the hope that something good will happen
(as proposed by Andres). In the absence of consensus, doing nothing
is a reasonable choice here.

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

Robert Haas wrote:

> My gut feeling on this is to either do nothing or add a VACUUM option
> (not a GUC, not a reloption) called even_frozen_pages, default false.
> What is your opinion?

+1 for that approach -- I thought that was already agreed weeks ago and
the only question was what to name that option. even_frozen_pages
sounds better than SCANALL, SCAN_ALL, FREEZE, FORCE (the other
options I saw proposed in that subthread), so +1 for that naming
too.

I don't like doing nothing; that means that when we discover a bug we'll
have to tell users to rm a file whose name requires a complicated
catalog query to find out, so -1 for that.

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

On Mon, Jun 6, 2016 at 10:18 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas wrote:
>> My gut feeling on this is to either do nothing or add a VACUUM option
>> (not a GUC, not a reloption) called even_frozen_pages, default false.
>> What is your opinion?
>
> +1 for that approach -- I thought that was already agreed weeks ago and
> the only question was what to name that option. even_frozen_pages
> sounds better than SCANALL, SCAN_ALL, FREEZE, FORCE (the other
> options I saw proposed in that subthread), so +1 for that naming
> too.
>
> I don't like doing nothing; that means that when we discover a bug we'll
> have to tell users to rm a file whose name requires a complicated
> catalog query to find out, so -1 for that.

So... I agree that it is definitely not good if we have to tell users
to rm a file, but I am not quite sure how this new option would
prevent us from having to say that? Here are some potential kinds of
bugs we might have:

1. Sometimes, the all-frozen bit doesn't get set when it should.
2. Sometimes, the all-frozen bit gets sit when it shouldn't.
3. Some combination of (1) and (2), so that the VM fork can't be
trusted in either direction.

If (1) happens, removing the VM fork is not a good idea; what people
will want to do is re-run a VACUUM FREEZE.

If (2) or (3) happens, removing the VM fork might be a good idea, but
it's not really clear that VACUUM (even_frozen_pages) will help much.
For one thing, if there are actually unfrozen tuples on those pages
and the clog pages which they reference are already gone or recycled,
rerunning VACUUM on the table in any form might permanently lose data,
or maybe it will just fail.

If because of the nature of the bug you somehow know that case doesn't
pertain, then I suppose the bug is that the tuple-level and page-level
state is out of sync. VACUUM (even_frozen_pages) probably won't help
with that much either, because VACUUM never clears the all-frozen bit
without also clearing the all-visible bit, and that only if the page
contains dead tuples, which in this case it probably doesn't.

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.

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

Robert Haas <robertmhaas(at)gmail(dot)com> 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.

regards, tom lane

On 2016-06-06 05:34:32 -0400, Robert Haas wrote:
> On Mon, Jun 6, 2016 at 5:11 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> >> Attached is a sample patch that controls full page vacuum by new GUC parameter.
> >
> > Don't we want a reloption for that? Just wondering...
>
> Why? Just for consistency? I think the bigger question here is
> whether we need to do anything at all. It's true that, without some
> new option, we'll lose the ability to forcibly vacuum every page in
> the relation, even if all-frozen. But there's not much use case for
> that in the first place. It will be potentially helpful if it turns
> out that we have a bug that sets the all-frozen bit on pages that are
> not, in fact, all-frozen. Otherwise, what's the use?

Except that we right now don't have any realistic way to figure out
whether this new feature actually does the right thing. Which makes
testing this *considerably* harder than just VACUUM (dwim). I think it's
unacceptable to release this feature without a way that'll tell that it
so far has/has not corrupted the database. Would that, in a perfect
world, be vacuum? No, probably not. But since we're not in a perfect world...

Andres

On Mon, Jun 6, 2016 at 11:28 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-06-06 05:34:32 -0400, Robert Haas wrote:
>> On Mon, Jun 6, 2016 at 5:11 AM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> >> Attached is a sample patch that controls full page vacuum by new GUC parameter.
>> >
>> > Don't we want a reloption for that? Just wondering...
>>
>> Why? Just for consistency? I think the bigger question here is
>> whether we need to do anything at all. It's true that, without some
>> new option, we'll lose the ability to forcibly vacuum every page in
>> the relation, even if all-frozen. But there's not much use case for
>> that in the first place. It will be potentially helpful if it turns
>> out that we have a bug that sets the all-frozen bit on pages that are
>> not, in fact, all-frozen. Otherwise, what's the use?
>
> Except that we right now don't have any realistic way to figure out
> whether this new feature actually does the right thing. Which makes
> testing this *considerably* harder than just VACUUM (dwim). I think it's
> unacceptable to release this feature without a way that'll tell that it
> so far has/has not corrupted the database. Would that, in a perfect
> world, be vacuum? No, probably not. But since we're not in a perfect world...

I just don't see how running VACUUM on the all-frozen pages is going
to help. In terms of diagnostic tools, you can get the VM bits and
page-level bits using the pg_visibility extension; I wrote it
precisely because of concerns like the ones you raise here. If you
want to cross-check the page-level bits against the tuple-level bits,
you can do that with the pageinspect extension. And if you do those
things, you can actually find out whether stuff is broken. Vacuuming
the all-frozen pages won't tell you that. It will either do nothing
(which doesn't tell you that things are OK) or it will change
something (possibly without reporting any message, and possibly making
a bad situation worse instead of better).

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

On 2016-06-06 11:37:25 -0400, Robert Haas wrote:
> On Mon, Jun 6, 2016 at 11:28 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > Except that we right now don't have any realistic way to figure out
> > whether this new feature actually does the right thing. Which makes
> > testing this *considerably* harder than just VACUUM (dwim). I think it's
> > unacceptable to release this feature without a way that'll tell that it
> > so far has/has not corrupted the database. Would that, in a perfect
> > world, be vacuum? No, probably not. But since we're not in a perfect world...
>
> I just don't see how running VACUUM on the all-frozen pages is going
> to help.

Because we can tell people in the beta2 announcement or some wiki page
"please run VACUUM(scan_all)" and check whether it emits WARNINGs. And
if we suspect freeze map in bug reports, we can just ask reporters to
run a VACUUM (scan_all).

> In terms of diagnostic tools, you can get the VM bits and
> page-level bits using the pg_visibility extension; I wrote it
> precisely because of concerns like the ones you raise here. If you
> want to cross-check the page-level bits against the tuple-level bits,
> you can do that with the pageinspect extension. And if you do those
> things, you can actually find out whether stuff is broken.

That's WAY out ouf reach of any "normal users". Adding a vacuum option
is doable, writing complex queries is not.

> Vacuuming the all-frozen pages won't tell you that. It will either do
> nothing (which doesn't tell you that things are OK) or it will change
> something (possibly without reporting any message, and possibly making
> a bad situation worse instead of better).

We found a number of bugs for the equivalent issues in all-visible
handling via the vacuum error reporting around those.

Greetings,

Andres Freund

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jun 6, 2016 at 11:28 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> Except that we right now don't have any realistic way to figure out
>> whether this new feature actually does the right thing.

> I just don't see how running VACUUM on the all-frozen pages is going
> to help.

Yes. I don't see that any of the proposed features would be very useful
for answering the question "is my VM incorrect". Maybe they would fix
problems, and maybe not, but in any case you couldn't rely on VACUUM
to tell you about a problem. (Even if you've got warning messages in
there, they might disappear into the postmaster log during an
auto-vacuum. Warning messages in VACUUM are not a good debugging
technology.)

regards, tom lane

On Mon, Jun 6, 2016 at 11:44 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> In terms of diagnostic tools, you can get the VM bits and
>> page-level bits using the pg_visibility extension; I wrote it
>> precisely because of concerns like the ones you raise here. If you
>> want to cross-check the page-level bits against the tuple-level bits,
>> you can do that with the pageinspect extension. And if you do those
>> things, you can actually find out whether stuff is broken.
>
> That's WAY out ouf reach of any "normal users". Adding a vacuum option
> is doable, writing complex queries is not.

Why would they have to write the complex query? Wouldn't they just
need to run that we wrote for them?

I mean, I'm not 100% dead set against this option you want, but in all
honestly, I would never, ever tell anyone to use it. Unleashing
VACUUM on possibly-damaged data is just asking it to decide to prune
away tuples you don't want gone. I would try very hard to come up
with something to give that user that was only going to *read* the
possibly-damaged data with as little chance of modifying or erasing it
as possible.

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Mon, Jun 6, 2016 at 11:44 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >> In terms of diagnostic tools, you can get the VM bits and
> >> page-level bits using the pg_visibility extension; I wrote it
> >> precisely because of concerns like the ones you raise here. If you
> >> want to cross-check the page-level bits against the tuple-level bits,
> >> you can do that with the pageinspect extension. And if you do those
> >> things, you can actually find out whether stuff is broken.
> >
> > That's WAY out ouf reach of any "normal users". Adding a vacuum option
> > is doable, writing complex queries is not.
>
> Why would they have to write the complex query? Wouldn't they just
> need to run that we wrote for them?
>
> I mean, I'm not 100% dead set against this option you want, but in all
> honestly, I would never, ever tell anyone to use it. Unleashing
> VACUUM on possibly-damaged data is just asking it to decide to prune
> away tuples you don't want gone. I would try very hard to come up
> with something to give that user that was only going to *read* the
> possibly-damaged data with as little chance of modifying or erasing it
> as possible.

I certainly agree with this.

We need a read-only utility which checks that the system is in a correct
and valid state. There are a few of those which have been built for
different pieces, I believe, and we really should have one for the
visibility map, but I don't think it makes sense to imply in any way
that VACUUM can or should be used for that.

Thanks!

Stephen

On 2016-06-06 14:24:14 -0400, Stephen Frost wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > On Mon, Jun 6, 2016 at 11:44 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > >> In terms of diagnostic tools, you can get the VM bits and
> > >> page-level bits using the pg_visibility extension; I wrote it
> > >> precisely because of concerns like the ones you raise here. If you
> > >> want to cross-check the page-level bits against the tuple-level bits,
> > >> you can do that with the pageinspect extension. And if you do those
> > >> things, you can actually find out whether stuff is broken.
> > >
> > > That's WAY out ouf reach of any "normal users". Adding a vacuum option
> > > is doable, writing complex queries is not.
> >
> > Why would they have to write the complex query? Wouldn't they just
> > need to run that we wrote for them?

Then write that query. Verify that that query performs halfway
reasonably fast. Document that it should be run against databases after
subjecting them to tests. That'd address my concern as well.

> > I mean, I'm not 100% dead set against this option you want, but in all
> > honestly, I would never, ever tell anyone to use it. Unleashing
> > VACUUM on possibly-damaged data is just asking it to decide to prune
> > away tuples you don't want gone. I would try very hard to come up
> > with something to give that user that was only going to *read* the
> > possibly-damaged data with as little chance of modifying or erasing it
> > as possible.

I'm more concerned about actually being able to verify that the freeze
logic does actually something meaningful, in situation where we'd *NOT*
expect any problems. If we're not trusting vacuum in that situation,
well ...

> I certainly agree with this.
>
> We need a read-only utility which checks that the system is in a correct
> and valid state. There are a few of those which have been built for
> different pieces, I believe, and we really should have one for the
> visibility map, but I don't think it makes sense to imply in any way
> that VACUUM can or should be used for that.

Meh. This is vacuum behaviour that *has existed* up to this point. You
essentially removed it. Sure, I'm all for adding a verification
tool. But that's just pie in the skie at this point. We have a complex,
data loss threatening feature, which just about nobody can verify at
this point. That's crazy.

Greetings,

Andres Freund

On Mon, Jun 6, 2016 at 2:35 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> > Why would they have to write the complex query? Wouldn't they just
>> > need to run that we wrote for them?
>
> Then write that query. Verify that that query performs halfway
> reasonably fast. Document that it should be run against databases after
> subjecting them to tests. That'd address my concern as well.

You know, I am starting to lose a teeny bit of patience here. I do
appreciate you reviewing this code, very much, and genuinely, and it
would be great if more people wanted to review it. But this kind of
reads like you think that I'm being a jerk, which I'm trying pretty
hard not to be, and like you have the right to tell assign me
arbitrary work, which I think you don't. If you want to have a
reasonable conversation about what the options are for making this
better, great. If you want to me to do some work to help improve
things on a patch I committed, that is 100% fair. But I don't know
what I did to earn this response which, to me, reads as rather
demanding and rather exasperated.

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

Hi,

On 2016-06-06 15:16:10 -0400, Robert Haas wrote:
> On Mon, Jun 6, 2016 at 2:35 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >> > Why would they have to write the complex query? Wouldn't they just
> >> > need to run that we wrote for them?
> >
> > Then write that query. Verify that that query performs halfway
> > reasonably fast. Document that it should be run against databases after
> > subjecting them to tests. That'd address my concern as well.
>
> You know, I am starting to lose a teeny bit of patience here.

Same here.

> I do appreciate you reviewing this code, very much, and genuinely, and
> it would be great if more people wanted to review it.

> But this kind of reads like you think that I'm being a jerk, which I'm
> trying pretty hard not to be

I don't think you're a jerk. But I am loosing a good bit of my patience
here. I've posted these issues a month ago, and for a long while the
only thing that happened was bikeshedding about the name of something
that wasn't even decided to happen yet (obviously said bikeshedding
isn't your fault).

> and like you have the right to tell assign me arbitrary work, which I
> think you don't.

It's not like adding a parameter for this would be a lot of work,
there's even a patch out there. I'm getting impatient because I feel
the issue of this critical feature not being testable is getting ignored
and/or played down. And then sidetracked into a general "let's add a
database consistency checker" type discussion. Which we need, but won't
get in 9.6.

If you say: "I agree with the feature in principle, but I don't want to
spend time to review/commit it." - ok, that's fair enough. But at the
moment that isn't what I'm reading between the lines.

> If you want to have a
> reasonable conversation about what the options are for making this
> better, great.

Yes, I want that.

> If you want to me to do some work to help improve things on a patch I
> committed, that is 100% fair. But I don't know what I did to earn
> this response which, to me, reads as rather demanding and rather
> exasperated.

I don't think it's absurd to make some demands on the committer of a
impact-heavy feature, about at least finding a realistic path towards
the new feature being realistically testable. This is a scary (but
*REALLY IMPORTANT*) patch, and I don't understand why it's ok that we
can't push a it through a couple wraparounds under high concurrency, and
easily verify that the freeze map is in sync with the actual data.

And yes, I *am* exasperated, that I'm the only one that appears to be
scared by the lack of that capability. I think the feature is in a
*lot* better shape than multixacts, but it certainly has the potential
to do even more damage in ways that'll essentially be unrecoverable.

Andres

Andres, all,

* Andres Freund (andres(at)anarazel(dot)de) wrote:
> On 2016-06-06 15:16:10 -0400, Robert Haas wrote:
> > On Mon, Jun 6, 2016 at 2:35 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > and like you have the right to tell assign me arbitrary work, which I
> > think you don't.
>
> It's not like adding a parameter for this would be a lot of work,
> there's even a patch out there. I'm getting impatient because I feel
> the issue of this critical feature not being testable is getting ignored
> and/or played down. And then sidetracked into a general "let's add a
> database consistency checker" type discussion. Which we need, but won't
> get in 9.6.

To be clear, I was pointing out that we've had similar types of
consistency checkers implemented for other big features (eg: Heikki's
work on checking that WAL works) and that it'd be good to have one here
also.

That could be as simple as a query with the right things installed, or
it might be an independent tool, but not having any way to check isn't
good. That said, trying to make VACUUM do that doesn't make sense to me
either.

Perhaps that's not an option due to the lateness of the hour or the lack
of manpower behind it, but that doesn't seem to be what has been said so
far.

> > If you want to me to do some work to help improve things on a patch I
> > committed, that is 100% fair. But I don't know what I did to earn
> > this response which, to me, reads as rather demanding and rather
> > exasperated.
>
> I don't think it's absurd to make some demands on the committer of a
> impact-heavy feature, about at least finding a realistic path towards
> the new feature being realistically testable. This is a scary (but
> *REALLY IMPORTANT*) patch, and I don't understand why it's ok that we
> can't push a it through a couple wraparounds under high concurrency, and
> easily verify that the freeze map is in sync with the actual data.
>
> And yes, I *am* exasperated, that I'm the only one that appears to be
> scared by the lack of that capability. I think the feature is in a
> *lot* better shape than multixacts, but it certainly has the potential
> to do even more damage in ways that'll essentially be unrecoverable.

Not having a straightforward way to ensure that it's working properly is
certainly concerning to me as well.

Thanks!

Stephen

On 2016-06-06 16:18:19 -0400, Stephen Frost wrote:
> That could be as simple as a query with the right things installed, or
> it might be an independent tool, but not having any way to check isn't
> good. That said, trying to make VACUUM do that doesn't make sense to me
> either.

The point is that VACUUM *has* these types of checks. And had so for
many years:
else if (all_visible_according_to_vm && !PageIsAllVisible(page)
&& VM_ALL_VISIBLE(onerel, blkno, &vmbuffer))
{
elog(WARNING, "page is not marked all-visible but visibility map bit is set in relation \"%s\" page %u",
relname, blkno);
visibilitymap_clear(onerel, blkno, vmbuffer);
}
...
else if (PageIsAllVisible(page) && has_dead_tuples)
{
elog(WARNING, "page containing dead tuples is marked as all-visible in relation \"%s\" page %u",
relname, blkno);
PageClearAllVisible(page);
MarkBufferDirty(buf);
visibilitymap_clear(onerel, blkno, vmbuffer);
}

the point is that, after the introduction of the freeze bit, there's no
way to reach them anymore (and they're missing a useful extension of
these warnings, but ...); these warnings have caught bugs. I don't
think it'd advocate for the vacuum option otherwise.

Greetings,

Andres Freund