Reviewing freeze map code

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

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

On Tue, May 3, 2016 at 6:48 AM, Andres Freund <andres@anarazel.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
/messages/by-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

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

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
/messages/by-id/20160331103739.8956.94469@wrigleys.postgresql.org
but I don't see it being more relevant here.

Andres

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

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

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

On Mon, May 2, 2016 at 8:25 PM, Andres Freund <andres@anarazel.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

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

On Wed, May 4, 2016 at 8:08 PM, Andres Freund <andres@anarazel.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
/messages/by-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

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

On Thu, May 5, 2016 at 2:20 PM, Andres Freund <andres@anarazel.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

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

On 05/06/2016 01:40 PM, Robert Haas wrote:

On Wed, May 4, 2016 at 8:08 PM, Andres Freund <andres@anarazel.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.

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

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@anarazel.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

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

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.

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

* Joshua D. Drake (jd@commandprompt.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?

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

On 05/06/2016 01:58 PM, Stephen Frost wrote:

* Joshua D. Drake (jd@commandprompt.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.

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

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)

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

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.

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

* Josh berkus (josh@agliodbs.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...

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

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.

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

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)

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