pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  
help / color / mirror / Atom feed
9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
22+ messages / 11 participants
[nested] [flat]

* 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2013-01-09 23:07  Bruce Momjian <bruce@momjian.us>
  0 siblings, 1 reply; 22+ messages in thread

From: Bruce Momjian @ 2013-01-09 23:07 UTC (permalink / raw)
  To: pgsql-hackers; +Cc: Guillaume Cottenceau <gc@mnc.ch>

How do we want to handle the case where VACUUM FULL clears the
visibility map, causing loss of index-only scans?

	http://archives.postgresql.org/pgsql-performance/2012-11/msg00317.php

Do we document this behavior or add a TODO item?

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2013-01-10 00:38  Josh Berkus <josh@agliodbs.com>
  parent: Bruce Momjian <bruce@momjian.us>
  0 siblings, 2 replies; 22+ messages in thread

From: Josh Berkus @ 2013-01-10 00:38 UTC (permalink / raw)
  To: pgsql-hackers

On 01/09/2013 03:07 PM, Bruce Momjian wrote:
> How do we want to handle the case where VACUUM FULL clears the
> visibility map, causing loss of index-only scans?
> 
> 	http://archives.postgresql.org/pgsql-performance/2012-11/msg00317.php
> 
> Do we document this behavior or add a TODO item?
> 

Both?

Surely VACUUM FULL should rebuild the visibility map, and make tuples in
the new relation all-visible, no?

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2013-01-10 06:15  Amit Kapila <amit.kapila@huawei.com>
  parent: Josh Berkus <josh@agliodbs.com>
  1 sibling, 0 replies; 22+ messages in thread

From: Amit Kapila @ 2013-01-10 06:15 UTC (permalink / raw)
  To: 'Josh Berkus' <josh@agliodbs.com>; pgsql-hackers

On Thursday, January 10, 2013 6:09 AM Josh Berkus wrote:
> On 01/09/2013 03:07 PM, Bruce Momjian wrote:
> > How do we want to handle the case where VACUUM FULL clears the
> > visibility map, causing loss of index-only scans?
> >
> > 	http://archives.postgresql.org/pgsql-performance/2012-
> 11/msg00317.php
> >
> > Do we document this behavior or add a TODO item?
> >
> 
> Both?
> 
> Surely VACUUM FULL should rebuild the visibility map, and make tuples
> in
> the new relation all-visible, no?

I think it cannot made all visible.
How about if any transaction in SSI mode is started before Vacuum Full, should it see all tuples.

With Regards,
Amit Kapila.



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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2013-01-10 06:31  Pavan Deolasee <pavan.deolasee@gmail.com>
  parent: Josh Berkus <josh@agliodbs.com>
  1 sibling, 1 reply; 22+ messages in thread

From: Pavan Deolasee @ 2013-01-10 06:31 UTC (permalink / raw)
  To: Amit Kapila <amit.kapila@huawei.com>; +Cc: Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On Thu, Jan 10, 2013 at 11:45 AM, Amit Kapila <amit.kapila@huawei.com> wrote:
> On Thursday, January 10, 2013 6:09 AM Josh Berkus wrote:

>>
>> Surely VACUUM FULL should rebuild the visibility map, and make tuples
>> in
>> the new relation all-visible, no?
>
> I think it cannot made all visible.
> How about if any transaction in SSI mode is started before Vacuum Full, should it see all tuples.
>

We can definitely do better than what we are doing today and that
should fix many use cases and rebuild the VM for large part of the
table if not all. More precisely, in cluster.c we can see what does
HeapTupleSatisfiesVacuum() returns for every tuple in a page. If there
are only DEAD or LIVE tuples in a page, we can set the VM bit. We may
need similar additional checks for LIVE tuples like we have in vacuum
code path. But its certainly doable.

I'd also suggested doing something similar for cases when a table is
created and written in the same transaction. But that's more invasive.

Thanks,
Pavan

-- 
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2013-01-10 09:18  Amit Kapila <amit.kapila@huawei.com>
  parent: Pavan Deolasee <pavan.deolasee@gmail.com>
  0 siblings, 0 replies; 22+ messages in thread

From: Amit Kapila @ 2013-01-10 09:18 UTC (permalink / raw)
  To: 'Pavan Deolasee' <pavan.deolasee@gmail.com>; 'Bruce Momjian' <bruce@momjian.us>; +Cc: 'Josh Berkus' <josh@agliodbs.com>; pgsql-hackers

On Thursday, January 10, 2013 12:01 PM Pavan Deolasee wrote:
> On Thu, Jan 10, 2013 at 11:45 AM, Amit Kapila <amit.kapila@huawei.com>
> wrote:
> > On Thursday, January 10, 2013 6:09 AM Josh Berkus wrote:
> 
> >>
> >> Surely VACUUM FULL should rebuild the visibility map, and make
> tuples
> >> in
> >> the new relation all-visible, no?
> >
> > I think it cannot made all visible.
> > How about if any transaction in SSI mode is started before Vacuum
> Full, should it see all tuples.
> >
> 
> We can definitely do better than what we are doing today and that
> should fix many use cases and rebuild the VM for large part of the
> table if not all. More precisely, in cluster.c we can see what does
> HeapTupleSatisfiesVacuum() returns for every tuple in a page. If there
> are only DEAD or LIVE tuples in a page, we can set the VM bit. We may
> need similar additional checks for LIVE tuples like we have in vacuum
> code path. But its certainly doable.

>> Do we document this behavior or add a TODO item?
 

> Both?

IMO, we should do both.

With Regards,
Amit Kapila.



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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2013-01-12 19:14  Kevin Grittner <kgrittn@mail.com>
  0 siblings, 1 reply; 22+ messages in thread

From: Kevin Grittner @ 2013-01-12 19:14 UTC (permalink / raw)
  To: Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

Amit Kapila wrote:
> On Thursday, January 10, 2013 6:09 AM Josh Berkus wrote:

>> Surely VACUUM FULL should rebuild the visibility map, and make
>> tuples in the new relation all-visible, no?

Certainly it seems odd to me that VACUUM FULL leaves the the table
in a less-well maintained state in terms of visibility than a
"normal" vacuum. VACUUM FULL should not need to be followed by
another VACUUM.

> I think it cannot made all visible.

I don't think all tuples in the relation are necessarily visible to
all transactions, but the ones which are should probably be flagged
that way.

> How about if any transaction in SSI mode is started before Vacuum
> Full, should it see all tuples.

There are no differences between visibility rules for serializable
transactions (SSI) and repeatable read transactions. It should be
based on whether any snapshots exist which can still see the tuple.

-Kevin


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2013-11-27 21:33  Bruce Momjian <bruce@momjian.us>
  parent: Kevin Grittner <kgrittn@mail.com>
  0 siblings, 1 reply; 22+ messages in thread

From: Bruce Momjian @ 2013-11-27 21:33 UTC (permalink / raw)
  To: Kevin Grittner <kgrittn@mail.com>; +Cc: Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On Sat, Jan 12, 2013 at 02:14:03PM -0500, Kevin Grittner wrote:
> Amit Kapila wrote:
> > On Thursday, January 10, 2013 6:09 AM Josh Berkus wrote:
> 
> >> Surely VACUUM FULL should rebuild the visibility map, and make
> >> tuples in the new relation all-visible, no?
> 
> Certainly it seems odd to me that VACUUM FULL leaves the the table
> in a less-well maintained state in terms of visibility than a
> "normal" vacuum. VACUUM FULL should not need to be followed by
> another VACUUM.
> 
> > I think it cannot made all visible.
> 
> I don't think all tuples in the relation are necessarily visible to
> all transactions, but the ones which are should probably be flagged
> that way.

I have developed the attached proof-of-concept patch to fix the problem
of having no visibility map after CLUSTER or VACUUM FULL.  I tested with
these queries:

	CREATE TABLE test(x INT PRIMARY KEY);
	INSERT INTO test VALUES (1);
	VACUUM FULL test; -- or CLUSTER
	SELECT relfilenode FROM pg_class WHERE relname = 'test';
	 relfilenode
	-------------
	       16399

Then 'ls -l data/base/16384/16399*' to see the *_vm file.  I am not sure
how to test that the vm contents are valid.

This patch is fairly tricky because our CLUSTER/VACUUM FULL behavior
does not do writes through the shared buffer cache, as outlined in this
C comment block:

 * We can't use the normal heap_insert function to insert into the new
 * heap, because heap_insert overwrites the visibility information.
 * We use a special-purpose raw_heap_insert function instead, which
 * is optimized for bulk inserting a lot of tuples, knowing that we have
 * exclusive access to the heap.  raw_heap_insert builds new pages in
 * local storage.  When a page is full, or at the end of the process,
 * we insert it to WAL as a single record and then write it to disk
 * directly through smgr.  Note, however, that any data sent to the new
 * heap's TOAST table will go through the normal bufmgr.

I originally tried to do this higher up in the stack but ran into
problems because I couldn't access the new heap page so I had to do it
at the non-shared-buffer page level.  I reused the lazy vacuum routines.

I need to know this is the right approach, and need to know what things
are wrong or missing.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

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

Attachments:

  [text/x-diff] vacuum.diff (7.0K, ../../20131127213320.GA3785@momjian.us/2-vacuum.diff)
  download | inline diff:
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
new file mode 100644
index 951894c..c01a6a8
*** a/src/backend/access/heap/rewriteheap.c
--- b/src/backend/access/heap/rewriteheap.c
***************
*** 107,112 ****
--- 107,114 ----
  #include "access/rewriteheap.h"
  #include "access/transam.h"
  #include "access/tuptoaster.h"
+ #include "access/visibilitymap.h"
+ #include "commands/vacuum.h"
  #include "storage/bufmgr.h"
  #include "storage/smgr.h"
  #include "utils/memutils.h"
*************** typedef OldToNewMappingData *OldToNewMap
*** 172,177 ****
--- 174,180 ----
  
  /* prototypes for internal functions */
  static void raw_heap_insert(RewriteState state, HeapTuple tup);
+ static void update_page_vm(Relation relation, Page page, BlockNumber blkno);
  
  
  /*
*************** end_heap_rewrite(RewriteState state)
*** 281,286 ****
--- 284,290 ----
  		RelationOpenSmgr(state->rs_new_rel);
  
  		PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
+ 		update_page_vm(state->rs_new_rel, state->rs_buffer, state->rs_blockno);
  
  		smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, state->rs_blockno,
  				   (char *) state->rs_buffer, true);
*************** raw_heap_insert(RewriteState state, Heap
*** 633,638 ****
--- 637,643 ----
  			RelationOpenSmgr(state->rs_new_rel);
  
  			PageSetChecksumInplace(page, state->rs_blockno);
+ 			update_page_vm(state->rs_new_rel, page, state->rs_blockno);
  
  			smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM,
  					   state->rs_blockno, (char *) page, true);
*************** raw_heap_insert(RewriteState state, Heap
*** 677,679 ****
--- 682,704 ----
  	if (heaptup != tup)
  		heap_freetuple(heaptup);
  }
+ 
+ static void
+ update_page_vm(Relation relation, Page page, BlockNumber blkno)
+ {
+ 	Buffer		vmbuffer = InvalidBuffer;
+ 	TransactionId visibility_cutoff_xid;
+ 
+ 	visibilitymap_pin(relation, blkno, &vmbuffer);
+ 	Assert(BufferIsValid(vmbuffer));
+ 
+ 	if (!visibilitymap_test(relation, blkno, &vmbuffer) &&
+ 		heap_page_is_all_visible(relation, InvalidBuffer, page,
+ 								 &visibility_cutoff_xid))
+ 	{
+ 		PageSetAllVisible(page);
+ 		visibilitymap_set(relation, blkno, InvalidBlockNumber,
+ 						  InvalidXLogRecPtr, vmbuffer, visibility_cutoff_xid);
+ 	}
+ 	ReleaseBuffer(vmbuffer);
+ }
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
new file mode 100644
index 7f40d89..a42511b
*** a/src/backend/access/heap/visibilitymap.c
--- b/src/backend/access/heap/visibilitymap.c
*************** visibilitymap_set(Relation rel, BlockNum
*** 278,284 ****
  		map[mapByte] |= (1 << mapBit);
  		MarkBufferDirty(vmBuf);
  
! 		if (RelationNeedsWAL(rel))
  		{
  			if (XLogRecPtrIsInvalid(recptr))
  			{
--- 278,284 ----
  		map[mapByte] |= (1 << mapBit);
  		MarkBufferDirty(vmBuf);
  
! 		if (RelationNeedsWAL(rel) && BufferIsValid(heapBuf))
  		{
  			if (XLogRecPtrIsInvalid(recptr))
  			{
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
new file mode 100644
index 6688ab3..ea349a5
*** a/src/backend/commands/vacuumlazy.c
--- b/src/backend/commands/vacuumlazy.c
*************** static void lazy_record_dead_tuple(LVRel
*** 151,158 ****
  					   ItemPointer itemptr);
  static bool lazy_tid_reaped(ItemPointer itemptr, void *state);
  static int	vac_cmp_itemptr(const void *left, const void *right);
- static bool heap_page_is_all_visible(Relation rel, Buffer buf,
- 						 TransactionId *visibility_cutoff_xid);
  
  
  /*
--- 151,156 ----
*************** lazy_vacuum_page(Relation onerel, BlockN
*** 1187,1193 ****
  	 * check if the page has become all-visible.
  	 */
  	if (!visibilitymap_test(onerel, blkno, vmbuffer) &&
! 		heap_page_is_all_visible(onerel, buffer, &visibility_cutoff_xid))
  	{
  		Assert(BufferIsValid(*vmbuffer));
  		PageSetAllVisible(page);
--- 1185,1191 ----
  	 * check if the page has become all-visible.
  	 */
  	if (!visibilitymap_test(onerel, blkno, vmbuffer) &&
! 		heap_page_is_all_visible(onerel, buffer, NULL, &visibility_cutoff_xid))
  	{
  		Assert(BufferIsValid(*vmbuffer));
  		PageSetAllVisible(page);
*************** vac_cmp_itemptr(const void *left, const
*** 1694,1707 ****
   * transactions. Also return the visibility_cutoff_xid which is the highest
   * xmin amongst the visible tuples.
   */
! static bool
! heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId *visibility_cutoff_xid)
  {
- 	Page		page = BufferGetPage(buf);
  	OffsetNumber offnum,
  				maxoff;
  	bool		all_visible = true;
  
  	*visibility_cutoff_xid = InvalidTransactionId;
  
  	/*
--- 1692,1708 ----
   * transactions. Also return the visibility_cutoff_xid which is the highest
   * xmin amongst the visible tuples.
   */
! bool
! heap_page_is_all_visible(Relation rel, Buffer buf, Page page,
! 						 TransactionId *visibility_cutoff_xid)
  {
  	OffsetNumber offnum,
  				maxoff;
  	bool		all_visible = true;
  
+ 	if (BufferIsValid(buf))
+ 		page = BufferGetPage(buf);
+ 
  	*visibility_cutoff_xid = InvalidTransactionId;
  
  	/*
*************** heap_page_is_all_visible(Relation rel, B
*** 1722,1728 ****
  		if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
  			continue;
  
! 		ItemPointerSet(&(tuple.t_self), BufferGetBlockNumber(buf), offnum);
  
  		/*
  		 * Dead line pointers can have index pointers pointing to them. So
--- 1723,1731 ----
  		if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
  			continue;
  
! 		/* XXX use 0 or real offset? */
! 		ItemPointerSet(&(tuple.t_self), BufferIsValid(buf) ?
! 					   BufferGetBlockNumber(buf) : 0, offnum);
  
  		/*
  		 * Dead line pointers can have index pointers pointing to them. So
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
new file mode 100644
index ed66c49..7ad8b58
*** a/src/backend/utils/time/tqual.c
--- b/src/backend/utils/time/tqual.c
*************** static inline void
*** 108,113 ****
--- 108,117 ----
  SetHintBits(HeapTupleHeader tuple, Buffer buffer,
  			uint16 infomask, TransactionId xid)
  {
+ 	/* we might not have a buffer if we are doing raw_heap_insert() */
+ 	if (!BufferIsValid(buffer))
+ 		return;
+ 
  	if (TransactionIdIsValid(xid))
  	{
  		/* NB: xid must be known committed here! */
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
new file mode 100644
index 08bec25..04cd4b7
*** a/src/include/commands/vacuum.h
--- b/src/include/commands/vacuum.h
***************
*** 19,24 ****
--- 19,25 ----
  #include "catalog/pg_type.h"
  #include "nodes/parsenodes.h"
  #include "storage/buf.h"
+ #include "storage/bufpage.h"
  #include "storage/lock.h"
  #include "utils/relcache.h"
  
*************** extern void vacuum_delay_point(void);
*** 167,172 ****
--- 168,175 ----
  /* in commands/vacuumlazy.c */
  extern void lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
  				BufferAccessStrategy bstrategy);
+ extern bool heap_page_is_all_visible(Relation rel, Buffer buf, Page page,
+ 						 TransactionId *visibility_cutoff_xid);
  
  /* in commands/analyze.c */
  extern void analyze_rel(Oid relid, VacuumStmt *vacstmt,

^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2013-11-28 22:17  Robert Haas <robertmhaas@gmail.com>
  parent: Bruce Momjian <bruce@momjian.us>
  0 siblings, 2 replies; 22+ messages in thread

From: Robert Haas @ 2013-11-28 22:17 UTC (permalink / raw)
  To: Bruce Momjian <bruce@momjian.us>; +Cc: Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On Wed, Nov 27, 2013 at 4:33 PM, Bruce Momjian <bruce@momjian.us> wrote:
> On Sat, Jan 12, 2013 at 02:14:03PM -0500, Kevin Grittner wrote:
>> Amit Kapila wrote:
>> > On Thursday, January 10, 2013 6:09 AM Josh Berkus wrote:
>>
>> >> Surely VACUUM FULL should rebuild the visibility map, and make
>> >> tuples in the new relation all-visible, no?
>>
>> Certainly it seems odd to me that VACUUM FULL leaves the the table
>> in a less-well maintained state in terms of visibility than a
>> "normal" vacuum. VACUUM FULL should not need to be followed by
>> another VACUUM.
>>
>> > I think it cannot made all visible.
>>
>> I don't think all tuples in the relation are necessarily visible to
>> all transactions, but the ones which are should probably be flagged
>> that way.
>
> I have developed the attached proof-of-concept patch to fix the problem
> of having no visibility map after CLUSTER or VACUUM FULL.  I tested with
> these queries:
>
>         CREATE TABLE test(x INT PRIMARY KEY);
>         INSERT INTO test VALUES (1);
>         VACUUM FULL test; -- or CLUSTER
>         SELECT relfilenode FROM pg_class WHERE relname = 'test';
>          relfilenode
>         -------------
>                16399
>
> Then 'ls -l data/base/16384/16399*' to see the *_vm file.  I am not sure
> how to test that the vm contents are valid.
>
> This patch is fairly tricky because our CLUSTER/VACUUM FULL behavior
> does not do writes through the shared buffer cache, as outlined in this
> C comment block:
>
>  * We can't use the normal heap_insert function to insert into the new
>  * heap, because heap_insert overwrites the visibility information.
>  * We use a special-purpose raw_heap_insert function instead, which
>  * is optimized for bulk inserting a lot of tuples, knowing that we have
>  * exclusive access to the heap.  raw_heap_insert builds new pages in
>  * local storage.  When a page is full, or at the end of the process,
>  * we insert it to WAL as a single record and then write it to disk
>  * directly through smgr.  Note, however, that any data sent to the new
>  * heap's TOAST table will go through the normal bufmgr.
>
> I originally tried to do this higher up in the stack but ran into
> problems because I couldn't access the new heap page so I had to do it
> at the non-shared-buffer page level.  I reused the lazy vacuum routines.
>
> I need to know this is the right approach, and need to know what things
> are wrong or missing.

The fact that you've needed to modify SetHintBits() to make this work
is pretty nasty.  I'm not exactly sure what to do about that, but it
doesn't seem good.

This patch also has the desirable but surprising consequence that hint
bits will be set as a side effect of update_page_vm(), which means
that you'd better do that BEFORE checksumming the page.

I wonder if we ought to mark each page as all-visible in
raw_heap_insert() when we first initialize it, and then clear the flag
when we come across a tuple that isn't all-visible.  We could try to
set hint bits on the tuple before placing it on the page, too, though
I'm not sure of the details.

-- 
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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2013-11-28 22:38  Bruce Momjian <bruce@momjian.us>
  parent: Robert Haas <robertmhaas@gmail.com>
  1 sibling, 1 reply; 22+ messages in thread

From: Bruce Momjian @ 2013-11-28 22:38 UTC (permalink / raw)
  To: Robert Haas <robertmhaas@gmail.com>; +Cc: Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On Thu, Nov 28, 2013 at 05:17:07PM -0500, Robert Haas wrote:
> > I need to know this is the right approach, and need to know what things
> > are wrong or missing.
> 
> The fact that you've needed to modify SetHintBits() to make this work
> is pretty nasty.  I'm not exactly sure what to do about that, but it
> doesn't seem good.

Hey, if that's the worst of the problems, I will be very happy.  There
is a sense that because we are not using the shared buffer cache and not
WAL logging, we have to skip some stuff.

> This patch also has the desirable but surprising consequence that hint
> bits will be set as a side effect of update_page_vm(), which means
> that you'd better do that BEFORE checksumming the page.

Yes, I already saw that and fixed it in my git tree.

> I wonder if we ought to mark each page as all-visible in
> raw_heap_insert() when we first initialize it, and then clear the flag
> when we come across a tuple that isn't all-visible.  We could try to
> set hint bits on the tuple before placing it on the page, too, though
> I'm not sure of the details.

I went with the per-page approach because I wanted to re-use the vacuum
lazy function.  Is there some other code that does this already?  I am
trying to avoid yet-another set of routines that would need to be
maintained or could be buggy.  This hit bit setting is tricky.

And thanks much for the review!

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2013-12-03 16:25  Bruce Momjian <bruce@momjian.us>
  parent: Bruce Momjian <bruce@momjian.us>
  0 siblings, 2 replies; 22+ messages in thread

From: Bruce Momjian @ 2013-12-03 16:25 UTC (permalink / raw)
  To: Robert Haas <robertmhaas@gmail.com>; +Cc: Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On Thu, Nov 28, 2013 at 05:38:05PM -0500, Bruce Momjian wrote:
> > I wonder if we ought to mark each page as all-visible in
> > raw_heap_insert() when we first initialize it, and then clear the flag
> > when we come across a tuple that isn't all-visible.  We could try to
> > set hint bits on the tuple before placing it on the page, too, though
> > I'm not sure of the details.
> 
> I went with the per-page approach because I wanted to re-use the vacuum
> lazy function.  Is there some other code that does this already?  I am
> trying to avoid yet-another set of routines that would need to be
> maintained or could be buggy.  This hit bit setting is tricky.
> 
> And thanks much for the review!

So, should I put this in the next commit fest?  I still have an unknown
about the buffer number to use here:

!       /* XXX use 0 or real offset? */
!       ItemPointerSet(&(tuple.t_self), BufferIsValid(buf) ?
!                      BufferGetBlockNumber(buf) : 0, offnum);

Is everyone else OK with this approach?  Updated patch attached.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

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

Attachments:

  [text/x-diff] vacuum.diff (6.9K, ../../20131203162556.GC27105@momjian.us/2-vacuum.diff)
  download | inline diff:
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
new file mode 100644
index 951894c..44ae5d8
*** a/src/backend/access/heap/rewriteheap.c
--- b/src/backend/access/heap/rewriteheap.c
***************
*** 107,112 ****
--- 107,114 ----
  #include "access/rewriteheap.h"
  #include "access/transam.h"
  #include "access/tuptoaster.h"
+ #include "access/visibilitymap.h"
+ #include "commands/vacuum.h"
  #include "storage/bufmgr.h"
  #include "storage/smgr.h"
  #include "utils/memutils.h"
*************** typedef OldToNewMappingData *OldToNewMap
*** 172,177 ****
--- 174,180 ----
  
  /* prototypes for internal functions */
  static void raw_heap_insert(RewriteState state, HeapTuple tup);
+ static void update_page_vm(Relation relation, Page page, BlockNumber blkno);
  
  
  /*
*************** end_heap_rewrite(RewriteState state)
*** 280,285 ****
--- 283,289 ----
  						state->rs_buffer);
  		RelationOpenSmgr(state->rs_new_rel);
  
+ 		update_page_vm(state->rs_new_rel, state->rs_buffer, state->rs_blockno);
  		PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
  
  		smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, state->rs_blockno,
*************** raw_heap_insert(RewriteState state, Heap
*** 632,637 ****
--- 636,642 ----
  			 */
  			RelationOpenSmgr(state->rs_new_rel);
  
+ 			update_page_vm(state->rs_new_rel, page, state->rs_blockno);
  			PageSetChecksumInplace(page, state->rs_blockno);
  
  			smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM,
*************** raw_heap_insert(RewriteState state, Heap
*** 677,679 ****
--- 682,704 ----
  	if (heaptup != tup)
  		heap_freetuple(heaptup);
  }
+ 
+ static void
+ update_page_vm(Relation relation, Page page, BlockNumber blkno)
+ {
+ 	Buffer		vmbuffer = InvalidBuffer;
+ 	TransactionId visibility_cutoff_xid;
+ 
+ 	visibilitymap_pin(relation, blkno, &vmbuffer);
+ 	Assert(BufferIsValid(vmbuffer));
+ 
+ 	if (!visibilitymap_test(relation, blkno, &vmbuffer) &&
+ 		heap_page_is_all_visible(relation, InvalidBuffer, page,
+ 								 &visibility_cutoff_xid))
+ 	{
+ 		PageSetAllVisible(page);
+ 		visibilitymap_set(relation, blkno, InvalidBlockNumber,
+ 						  InvalidXLogRecPtr, vmbuffer, visibility_cutoff_xid);
+ 	}
+ 	ReleaseBuffer(vmbuffer);
+ }
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
new file mode 100644
index 7f40d89..a42511b
*** a/src/backend/access/heap/visibilitymap.c
--- b/src/backend/access/heap/visibilitymap.c
*************** visibilitymap_set(Relation rel, BlockNum
*** 278,284 ****
  		map[mapByte] |= (1 << mapBit);
  		MarkBufferDirty(vmBuf);
  
! 		if (RelationNeedsWAL(rel))
  		{
  			if (XLogRecPtrIsInvalid(recptr))
  			{
--- 278,284 ----
  		map[mapByte] |= (1 << mapBit);
  		MarkBufferDirty(vmBuf);
  
! 		if (RelationNeedsWAL(rel) && BufferIsValid(heapBuf))
  		{
  			if (XLogRecPtrIsInvalid(recptr))
  			{
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
new file mode 100644
index fe2d9e7..4f6578f
*** a/src/backend/commands/vacuumlazy.c
--- b/src/backend/commands/vacuumlazy.c
*************** static void lazy_record_dead_tuple(LVRel
*** 151,158 ****
  					   ItemPointer itemptr);
  static bool lazy_tid_reaped(ItemPointer itemptr, void *state);
  static int	vac_cmp_itemptr(const void *left, const void *right);
- static bool heap_page_is_all_visible(Relation rel, Buffer buf,
- 						 TransactionId *visibility_cutoff_xid);
  
  
  /*
--- 151,156 ----
*************** lazy_vacuum_page(Relation onerel, BlockN
*** 1197,1203 ****
  	 * check if the page has become all-visible.
  	 */
  	if (!visibilitymap_test(onerel, blkno, vmbuffer) &&
! 		heap_page_is_all_visible(onerel, buffer, &visibility_cutoff_xid))
  	{
  		Assert(BufferIsValid(*vmbuffer));
  		PageSetAllVisible(page);
--- 1195,1201 ----
  	 * check if the page has become all-visible.
  	 */
  	if (!visibilitymap_test(onerel, blkno, vmbuffer) &&
! 		heap_page_is_all_visible(onerel, buffer, NULL, &visibility_cutoff_xid))
  	{
  		Assert(BufferIsValid(*vmbuffer));
  		PageSetAllVisible(page);
*************** vac_cmp_itemptr(const void *left, const
*** 1704,1717 ****
   * transactions. Also return the visibility_cutoff_xid which is the highest
   * xmin amongst the visible tuples.
   */
! static bool
! heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId *visibility_cutoff_xid)
  {
- 	Page		page = BufferGetPage(buf);
  	OffsetNumber offnum,
  				maxoff;
  	bool		all_visible = true;
  
  	*visibility_cutoff_xid = InvalidTransactionId;
  
  	/*
--- 1702,1718 ----
   * transactions. Also return the visibility_cutoff_xid which is the highest
   * xmin amongst the visible tuples.
   */
! bool
! heap_page_is_all_visible(Relation rel, Buffer buf, Page page,
! 						 TransactionId *visibility_cutoff_xid)
  {
  	OffsetNumber offnum,
  				maxoff;
  	bool		all_visible = true;
  
+ 	if (BufferIsValid(buf))
+ 		page = BufferGetPage(buf);
+ 
  	*visibility_cutoff_xid = InvalidTransactionId;
  
  	/*
*************** heap_page_is_all_visible(Relation rel, B
*** 1732,1738 ****
  		if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
  			continue;
  
! 		ItemPointerSet(&(tuple.t_self), BufferGetBlockNumber(buf), offnum);
  
  		/*
  		 * Dead line pointers can have index pointers pointing to them. So
--- 1733,1741 ----
  		if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
  			continue;
  
! 		/* XXX use 0 or real offset? */
! 		ItemPointerSet(&(tuple.t_self), BufferIsValid(buf) ?
! 					   BufferGetBlockNumber(buf) : 0, offnum);
  
  		/*
  		 * Dead line pointers can have index pointers pointing to them. So
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
new file mode 100644
index 1ebc5ff..c53e2fb
*** a/src/backend/utils/time/tqual.c
--- b/src/backend/utils/time/tqual.c
*************** static inline void
*** 108,113 ****
--- 108,117 ----
  SetHintBits(HeapTupleHeader tuple, Buffer buffer,
  			uint16 infomask, TransactionId xid)
  {
+ 	/* we might not have a buffer if we are doing raw_heap_insert() */
+ 	if (!BufferIsValid(buffer))
+ 		return;
+ 
  	if (TransactionIdIsValid(xid))
  	{
  		/* NB: xid must be known committed here! */
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
new file mode 100644
index 44a3c3b..7e3ff9c
*** a/src/include/commands/vacuum.h
--- b/src/include/commands/vacuum.h
***************
*** 19,24 ****
--- 19,25 ----
  #include "catalog/pg_type.h"
  #include "nodes/parsenodes.h"
  #include "storage/buf.h"
+ #include "storage/bufpage.h"
  #include "storage/lock.h"
  #include "utils/relcache.h"
  
*************** extern void vacuum_delay_point(void);
*** 168,173 ****
--- 169,176 ----
  /* in commands/vacuumlazy.c */
  extern void lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
  				BufferAccessStrategy bstrategy);
+ extern bool heap_page_is_all_visible(Relation rel, Buffer buf, Page page,
+ 						 TransactionId *visibility_cutoff_xid);
  
  /* in commands/analyze.c */
  extern void analyze_rel(Oid relid, VacuumStmt *vacstmt,

^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2013-12-03 19:01  Robert Haas <robertmhaas@gmail.com>
  parent: Bruce Momjian <bruce@momjian.us>
  1 sibling, 1 reply; 22+ messages in thread

From: Robert Haas @ 2013-12-03 19:01 UTC (permalink / raw)
  To: Bruce Momjian <bruce@momjian.us>; +Cc: Kevin Grittner <kgrittn@mail.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On Tue, Dec 3, 2013 at 11:25 AM, Bruce Momjian <bruce@momjian.us> wrote:
> On Thu, Nov 28, 2013 at 05:38:05PM -0500, Bruce Momjian wrote:
>> > I wonder if we ought to mark each page as all-visible in
>> > raw_heap_insert() when we first initialize it, and then clear the flag
>> > when we come across a tuple that isn't all-visible.  We could try to
>> > set hint bits on the tuple before placing it on the page, too, though
>> > I'm not sure of the details.
>>
>> I went with the per-page approach because I wanted to re-use the vacuum
>> lazy function.  Is there some other code that does this already?  I am
>> trying to avoid yet-another set of routines that would need to be
>> maintained or could be buggy.  This hit bit setting is tricky.
>>
>> And thanks much for the review!
>
> So, should I put this in the next commit fest?

+1.

> I still have an unknown
> about the buffer number to use here:
>
> !       /* XXX use 0 or real offset? */
> !       ItemPointerSet(&(tuple.t_self), BufferIsValid(buf) ?
> !                      BufferGetBlockNumber(buf) : 0, offnum);
>
> Is everyone else OK with this approach?  Updated patch attached.

I started looking at this a little more the other day but got bogged
down in other things before I got through it all.  I think we're going
to want to revise this so that it doesn't go through the functions
that current assume that they're always deal with a shared_buffer, but
I haven't figured out exactly what the most elegant way of doing that
is yet.

-- 
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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2013-12-03 19:34  Bruce Momjian <bruce@momjian.us>
  parent: Robert Haas <robertmhaas@gmail.com>
  0 siblings, 0 replies; 22+ messages in thread

From: Bruce Momjian @ 2013-12-03 19:34 UTC (permalink / raw)
  To: Robert Haas <robertmhaas@gmail.com>; +Cc: Kevin Grittner <kgrittn@mail.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On Tue, Dec  3, 2013 at 02:01:52PM -0500, Robert Haas wrote:
> On Tue, Dec 3, 2013 at 11:25 AM, Bruce Momjian <bruce@momjian.us> wrote:
> > On Thu, Nov 28, 2013 at 05:38:05PM -0500, Bruce Momjian wrote:
> >> > I wonder if we ought to mark each page as all-visible in
> >> > raw_heap_insert() when we first initialize it, and then clear the flag
> >> > when we come across a tuple that isn't all-visible.  We could try to
> >> > set hint bits on the tuple before placing it on the page, too, though
> >> > I'm not sure of the details.
> >>
> >> I went with the per-page approach because I wanted to re-use the vacuum
> >> lazy function.  Is there some other code that does this already?  I am
> >> trying to avoid yet-another set of routines that would need to be
> >> maintained or could be buggy.  This hit bit setting is tricky.
> >>
> >> And thanks much for the review!
> >
> > So, should I put this in the next commit fest?
> 
> +1.

OK, I added it to the January commit fest.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2014-01-16 01:29  Simon Riggs <simon@2ndQuadrant.com>
  parent: Robert Haas <robertmhaas@gmail.com>
  1 sibling, 0 replies; 22+ messages in thread

From: Simon Riggs @ 2014-01-16 01:29 UTC (permalink / raw)
  To: Robert Haas <robertmhaas@gmail.com>; +Cc: Bruce Momjian <bruce@momjian.us>; Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On 28 November 2013 22:17, Robert Haas <robertmhaas@gmail.com> wrote:

> The fact that you've needed to modify SetHintBits() to make this work
> is pretty nasty.  I'm not exactly sure what to do about that, but it
> doesn't seem good.

That makes me feel bad also.

I think we should be looking for some special case routines rather
than piggybacking there.

Wonderful to see you personally and directly addressing this concern
and very supportive of this solution for 9.4.

I wonder about a user option to let these routines wait until they are
the oldest snapshot, so they can then set every page as all-visible
without even bothering to check individual items. Or maybe a user
option to set them all-visible even without actually being the oldest,
as is possible with COPY FREEZE. Full lock on the table is a big
thing, so a shame to waste the opportunity to set everything visible
just because some long running task is still executing somewhere (but
clearly not here otherwise we wouldn't have the lock).

-- 
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2014-01-24 21:52  Jaime Casanova <jaime@2ndquadrant.com>
  parent: Bruce Momjian <bruce@momjian.us>
  1 sibling, 2 replies; 22+ messages in thread

From: Jaime Casanova @ 2014-01-24 21:52 UTC (permalink / raw)
  To: Bruce Momjian <bruce@momjian.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On Tue, Dec 3, 2013 at 11:25 AM, Bruce Momjian <bruce@momjian.us> wrote:
>
> Is everyone else OK with this approach?  Updated patch attached.
>

Hi,

I started to look at this patch and i found that it fails an assertion
as soon as you run a VACUUM FULL after a lazy VACUUM even if those are
on unrelated relations. For example in an assert-enabled build with
the regression database run:

VACUUM customer;
[... insert here whatever commands you like or nothing at all ...]
VACUUM FULL tenk1;

TRAP: FailedAssertion("!(InRecovery || ( ((void) ((bool) ((!
assert_enabled) || ! (!((heapBuf) <= NBuffers && (heapBuf) >=
-NLocBuffer)) || (ExceptionalCondition("!((heapBuf) <= NBuffers &&
(heapBuf) >= -NLocBuffer)", ("FailedAssertion"), "visibilitymap.c",
260), 0)))), (heapBuf) != 0 ))", File: "visibilitymap.c", Line: 260)
LOG:  server process (PID 25842) was terminated by signal 6: Aborted
DETAIL:  Failed process was running: vacuum FULL customer;
LOG:  terminating any other active server processes

trace:
(gdb) bt
#0  0x00007f9a3d00d475 in *__GI_raise (sig=<optimized out>) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x00007f9a3d0106f0 in *__GI_abort () at abort.c:92
#2  0x0000000000777597 in ExceptionalCondition (
    conditionName=conditionName@entry=0x7cd3b8 "!(InRecovery || (
((void) ((bool) ((! assert_enabled) || ! (!((heapBuf) <= NBuffers &&
(heapBuf) >= -NLocBuffer)) || (ExceptionalCondition(\"!((heapBuf) <=
NBuffers && (heapBuf) >= -NLocBuffer)\", (\"Fai"...,
errorType=errorType@entry=0x7b0730 "FailedAssertion",
    fileName=fileName@entry=0x7cd105 "visibilitymap.c",
lineNumber=lineNumber@entry=260) at assert.c:54
#3  0x00000000004a7d99 in visibilitymap_set
(rel=rel@entry=0x7f9a3da56a00, heapBlk=heapBlk@entry=0,
heapBuf=heapBuf@entry=0, recptr=recptr@entry=0, vmBuf=220,
    cutoff_xid=2) at visibilitymap.c:260
#4  0x00000000004a33e5 in update_page_vm (relation=0x7f9a3da56a00,
page=page@entry=0x1868b18 "", blkno=0) at rewriteheap.c:702
#5  0x00000000004a3668 in raw_heap_insert
(state=state@entry=0x1849e98, tup=tup@entry=0x184f208) at
rewriteheap.c:641
#6  0x00000000004a3b8b in rewrite_heap_tuple
(state=state@entry=0x1849e98, old_tuple=old_tuple@entry=0x1852a50,
new_tuple=new_tuple@entry=0x184f208)
    at rewriteheap.c:433
#7  0x000000000055c373 in reform_and_rewrite_tuple
(tuple=tuple@entry=0x1852a50,
oldTupDesc=oldTupDesc@entry=0x7f9a3da4d350,
    newTupDesc=newTupDesc@entry=0x7f9a3da599a8,
values=values@entry=0x1852f40, isnull=isnull@entry=0x184f920 "",
newRelHasOids=1 '\001',
    rwstate=rwstate@entry=0x1849e98) at cluster.c:1670

-- 
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566         Cell: +593 987171157


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2014-01-25 02:09  Bruce Momjian <bruce@momjian.us>
  parent: Jaime Casanova <jaime@2ndquadrant.com>
  1 sibling, 1 reply; 22+ messages in thread

From: Bruce Momjian @ 2014-01-25 02:09 UTC (permalink / raw)
  To: Jaime Casanova <jaime@2ndquadrant.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On Fri, Jan 24, 2014 at 04:52:55PM -0500, Jaime Casanova wrote:
> On Tue, Dec 3, 2013 at 11:25 AM, Bruce Momjian <bruce@momjian.us> wrote:
> >
> > Is everyone else OK with this approach?  Updated patch attached.
> >
> 
> Hi,
> 
> I started to look at this patch and i found that it fails an assertion
> as soon as you run a VACUUM FULL after a lazy VACUUM even if those are
> on unrelated relations. For example in an assert-enabled build with
> the regression database run:
> 
> VACUUM customer;
> [... insert here whatever commands you like or nothing at all ...]
> VACUUM FULL customer;

Wow, thanks for the testing.  You are right that I had two bugs, both in
visibilitymap_set().  First, I made setting heapBuf optional, but forgot
to remove the Assert check now that it was optional.  Second, I passed
InvalidBlockNumber instead of InvalidBuffer when calling
visibilitymap_set().

Both are fixed in the attached patch.  Thanks for the report.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

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

Attachments:

  [text/x-diff] vacuum.diff (7.2K, ../../20140125020906.GA17657@momjian.us/2-vacuum.diff)
  download | inline diff:
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
new file mode 100644
index c34ab98..ff275fa
*** a/src/backend/access/heap/rewriteheap.c
--- b/src/backend/access/heap/rewriteheap.c
***************
*** 107,112 ****
--- 107,114 ----
  #include "access/rewriteheap.h"
  #include "access/transam.h"
  #include "access/tuptoaster.h"
+ #include "access/visibilitymap.h"
+ #include "commands/vacuum.h"
  #include "storage/bufmgr.h"
  #include "storage/smgr.h"
  #include "utils/memutils.h"
*************** typedef OldToNewMappingData *OldToNewMap
*** 172,177 ****
--- 174,180 ----
  
  /* prototypes for internal functions */
  static void raw_heap_insert(RewriteState state, HeapTuple tup);
+ static void update_page_vm(Relation relation, Page page, BlockNumber blkno);
  
  
  /*
*************** end_heap_rewrite(RewriteState state)
*** 281,286 ****
--- 284,290 ----
  						true);
  		RelationOpenSmgr(state->rs_new_rel);
  
+ 		update_page_vm(state->rs_new_rel, state->rs_buffer, state->rs_blockno);
  		PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
  
  		smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, state->rs_blockno,
*************** raw_heap_insert(RewriteState state, Heap
*** 633,638 ****
--- 637,643 ----
  			 */
  			RelationOpenSmgr(state->rs_new_rel);
  
+ 			update_page_vm(state->rs_new_rel, page, state->rs_blockno);
  			PageSetChecksumInplace(page, state->rs_blockno);
  
  			smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM,
*************** raw_heap_insert(RewriteState state, Heap
*** 678,680 ****
--- 683,705 ----
  	if (heaptup != tup)
  		heap_freetuple(heaptup);
  }
+ 
+ static void
+ update_page_vm(Relation relation, Page page, BlockNumber blkno)
+ {
+ 	Buffer		vmbuffer = InvalidBuffer;
+ 	TransactionId visibility_cutoff_xid;
+ 
+ 	visibilitymap_pin(relation, blkno, &vmbuffer);
+ 	Assert(BufferIsValid(vmbuffer));
+ 
+ 	if (!visibilitymap_test(relation, blkno, &vmbuffer) &&
+ 		heap_page_is_all_visible(relation, InvalidBuffer, page,
+ 								 &visibility_cutoff_xid))
+ 	{
+ 		PageSetAllVisible(page);
+ 		visibilitymap_set(relation, blkno, InvalidBuffer,
+ 						  InvalidXLogRecPtr, vmbuffer, visibility_cutoff_xid);
+ 	}
+ 	ReleaseBuffer(vmbuffer);
+ }
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
new file mode 100644
index 899ffac..3e7546b
*** a/src/backend/access/heap/visibilitymap.c
--- b/src/backend/access/heap/visibilitymap.c
*************** visibilitymap_set(Relation rel, BlockNum
*** 257,263 ****
  #endif
  
  	Assert(InRecovery || XLogRecPtrIsInvalid(recptr));
- 	Assert(InRecovery || BufferIsValid(heapBuf));
  
  	/* Check that we have the right heap page pinned, if present */
  	if (BufferIsValid(heapBuf) && BufferGetBlockNumber(heapBuf) != heapBlk)
--- 257,262 ----
*************** visibilitymap_set(Relation rel, BlockNum
*** 278,284 ****
  		map[mapByte] |= (1 << mapBit);
  		MarkBufferDirty(vmBuf);
  
! 		if (RelationNeedsWAL(rel))
  		{
  			if (XLogRecPtrIsInvalid(recptr))
  			{
--- 277,283 ----
  		map[mapByte] |= (1 << mapBit);
  		MarkBufferDirty(vmBuf);
  
! 		if (RelationNeedsWAL(rel) && BufferIsValid(heapBuf))
  		{
  			if (XLogRecPtrIsInvalid(recptr))
  			{
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
new file mode 100644
index 75e5f15..8d84bef
*** a/src/backend/commands/vacuumlazy.c
--- b/src/backend/commands/vacuumlazy.c
*************** static void lazy_record_dead_tuple(LVRel
*** 152,159 ****
  					   ItemPointer itemptr);
  static bool lazy_tid_reaped(ItemPointer itemptr, void *state);
  static int	vac_cmp_itemptr(const void *left, const void *right);
- static bool heap_page_is_all_visible(Relation rel, Buffer buf,
- 						 TransactionId *visibility_cutoff_xid);
  
  
  /*
--- 152,157 ----
*************** lazy_vacuum_page(Relation onerel, BlockN
*** 1232,1238 ****
  	 * check if the page has become all-visible.
  	 */
  	if (!visibilitymap_test(onerel, blkno, vmbuffer) &&
! 		heap_page_is_all_visible(onerel, buffer, &visibility_cutoff_xid))
  	{
  		Assert(BufferIsValid(*vmbuffer));
  		PageSetAllVisible(page);
--- 1230,1236 ----
  	 * check if the page has become all-visible.
  	 */
  	if (!visibilitymap_test(onerel, blkno, vmbuffer) &&
! 		heap_page_is_all_visible(onerel, buffer, NULL, &visibility_cutoff_xid))
  	{
  		Assert(BufferIsValid(*vmbuffer));
  		PageSetAllVisible(page);
*************** vac_cmp_itemptr(const void *left, const
*** 1730,1743 ****
   * transactions. Also return the visibility_cutoff_xid which is the highest
   * xmin amongst the visible tuples.
   */
! static bool
! heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId *visibility_cutoff_xid)
  {
- 	Page		page = BufferGetPage(buf);
  	OffsetNumber offnum,
  				maxoff;
  	bool		all_visible = true;
  
  	*visibility_cutoff_xid = InvalidTransactionId;
  
  	/*
--- 1728,1744 ----
   * transactions. Also return the visibility_cutoff_xid which is the highest
   * xmin amongst the visible tuples.
   */
! bool
! heap_page_is_all_visible(Relation rel, Buffer buf, Page page,
! 						 TransactionId *visibility_cutoff_xid)
  {
  	OffsetNumber offnum,
  				maxoff;
  	bool		all_visible = true;
  
+ 	if (BufferIsValid(buf))
+ 		page = BufferGetPage(buf);
+ 
  	*visibility_cutoff_xid = InvalidTransactionId;
  
  	/*
*************** heap_page_is_all_visible(Relation rel, B
*** 1758,1764 ****
  		if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
  			continue;
  
! 		ItemPointerSet(&(tuple.t_self), BufferGetBlockNumber(buf), offnum);
  
  		/*
  		 * Dead line pointers can have index pointers pointing to them. So
--- 1759,1767 ----
  		if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
  			continue;
  
! 		/* XXX use 0 or real offset? */
! 		ItemPointerSet(&(tuple.t_self), BufferIsValid(buf) ?
! 					   BufferGetBlockNumber(buf) : 0, offnum);
  
  		/*
  		 * Dead line pointers can have index pointers pointing to them. So
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
new file mode 100644
index f626755..b37ecc4
*** a/src/backend/utils/time/tqual.c
--- b/src/backend/utils/time/tqual.c
*************** static inline void
*** 108,113 ****
--- 108,117 ----
  SetHintBits(HeapTupleHeader tuple, Buffer buffer,
  			uint16 infomask, TransactionId xid)
  {
+ 	/* we might not have a buffer if we are doing raw_heap_insert() */
+ 	if (!BufferIsValid(buffer))
+ 		return;
+ 
  	if (TransactionIdIsValid(xid))
  	{
  		/* NB: xid must be known committed here! */
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
new file mode 100644
index 7c36890..1a35f6c
*** a/src/include/commands/vacuum.h
--- b/src/include/commands/vacuum.h
***************
*** 19,24 ****
--- 19,25 ----
  #include "catalog/pg_type.h"
  #include "nodes/parsenodes.h"
  #include "storage/buf.h"
+ #include "storage/bufpage.h"
  #include "storage/lock.h"
  #include "utils/relcache.h"
  
*************** extern void vacuum_delay_point(void);
*** 168,173 ****
--- 169,176 ----
  /* in commands/vacuumlazy.c */
  extern void lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
  				BufferAccessStrategy bstrategy);
+ extern bool heap_page_is_all_visible(Relation rel, Buffer buf, Page page,
+ 						 TransactionId *visibility_cutoff_xid);
  
  /* in commands/analyze.c */
  extern void analyze_rel(Oid relid, VacuumStmt *vacstmt,

^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2014-02-15 15:16  Andres Freund <andres@2ndquadrant.com>
  parent: Bruce Momjian <bruce@momjian.us>
  0 siblings, 1 reply; 22+ messages in thread

From: Andres Freund @ 2014-02-15 15:16 UTC (permalink / raw)
  To: Bruce Momjian <bruce@momjian.us>; +Cc: Jaime Casanova <jaime@2ndquadrant.com>; Robert Haas <robertmhaas@gmail.com>; Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

Hi,

> *************** end_heap_rewrite(RewriteState state)
> *** 281,286 ****
> --- 284,290 ----
>   						true);
>   		RelationOpenSmgr(state->rs_new_rel);
>   
> + 		update_page_vm(state->rs_new_rel, state->rs_buffer, state->rs_blockno);
>   		PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
>   
>   		smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, state->rs_blockno,
> *************** raw_heap_insert(RewriteState state, Heap
> *** 633,638 ****
> --- 637,643 ----
>   			 */
>   			RelationOpenSmgr(state->rs_new_rel);
>   
> + 			update_page_vm(state->rs_new_rel, page, state->rs_blockno);
>   			PageSetChecksumInplace(page, state->rs_blockno);
>   
>   			smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM,

I think those two cases should be combined.

> + static void
> + update_page_vm(Relation relation, Page page, BlockNumber blkno)
> + {
> + 	Buffer		vmbuffer = InvalidBuffer;
> + 	TransactionId visibility_cutoff_xid;
> + 
> + 	visibilitymap_pin(relation, blkno, &vmbuffer);
> + 	Assert(BufferIsValid(vmbuffer));
> + 
> + 	if (!visibilitymap_test(relation, blkno, &vmbuffer) &&
> + 		heap_page_is_all_visible(relation, InvalidBuffer, page,
> + 								 &visibility_cutoff_xid))
> + 	{
> + 		PageSetAllVisible(page);
> + 		visibilitymap_set(relation, blkno, InvalidBuffer,
> + 						  InvalidXLogRecPtr, vmbuffer, visibility_cutoff_xid);
> + 	}
> + 	ReleaseBuffer(vmbuffer);
> + }

How could visibilitymap_test() be true here?

> diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
> new file mode 100644
> index 899ffac..3e7546b
> *** a/src/backend/access/heap/visibilitymap.c
> --- b/src/backend/access/heap/visibilitymap.c
> *************** visibilitymap_set(Relation rel, BlockNum
> *** 257,263 ****
>   #endif
>   
>   	Assert(InRecovery || XLogRecPtrIsInvalid(recptr));
> - 	Assert(InRecovery || BufferIsValid(heapBuf));
>   
>   	/* Check that we have the right heap page pinned, if present */
>   	if (BufferIsValid(heapBuf) && BufferGetBlockNumber(heapBuf) != heapBlk)
> --- 257,262 ----
> *************** visibilitymap_set(Relation rel, BlockNum
> *** 278,284 ****
>   		map[mapByte] |= (1 << mapBit);
>   		MarkBufferDirty(vmBuf);
>   
> ! 		if (RelationNeedsWAL(rel))
>   		{
>   			if (XLogRecPtrIsInvalid(recptr))
>   			{
> --- 277,283 ----
>   		map[mapByte] |= (1 << mapBit);
>   		MarkBufferDirty(vmBuf);
>   
> ! 		if (RelationNeedsWAL(rel) && BufferIsValid(heapBuf))
>   		{
>   			if (XLogRecPtrIsInvalid(recptr))
>   			{

At the very least this needs to revise visibilitymap_set's comment about
the requirement of passing a buffer unless !InRecovery.

Also, how is this going to work with replication if you're explicitly
not WAL logging this?


> *************** vac_cmp_itemptr(const void *left, const
> *** 1730,1743 ****
>    * transactions. Also return the visibility_cutoff_xid which is the highest
>    * xmin amongst the visible tuples.
>    */
> ! static bool
> ! heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId *visibility_cutoff_xid)
>   {
> - 	Page		page = BufferGetPage(buf);
>   	OffsetNumber offnum,
>   				maxoff;
>   	bool		all_visible = true;
>   
>   	*visibility_cutoff_xid = InvalidTransactionId;
>   
>   	/*
> --- 1728,1744 ----
>    * transactions. Also return the visibility_cutoff_xid which is the highest
>    * xmin amongst the visible tuples.
>    */
> ! bool
> ! heap_page_is_all_visible(Relation rel, Buffer buf, Page page,
> ! 						 TransactionId *visibility_cutoff_xid)
>   {
>   	OffsetNumber offnum,
>   				maxoff;
>   	bool		all_visible = true;
>   
> + 	if (BufferIsValid(buf))
> + 		page = BufferGetPage(buf);
> + 
>   	*visibility_cutoff_xid = InvalidTransactionId;
>   
>   	/*
> *************** heap_page_is_all_visible(Relation rel, B
> *** 1758,1764 ****
>   		if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
>   			continue;
>   
> ! 		ItemPointerSet(&(tuple.t_self), BufferGetBlockNumber(buf), offnum);
>   
>   		/*
>   		 * Dead line pointers can have index pointers pointing to them. So
> --- 1759,1767 ----
>   		if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
>   			continue;
>   
> ! 		/* XXX use 0 or real offset? */
> ! 		ItemPointerSet(&(tuple.t_self), BufferIsValid(buf) ?
> ! 					   BufferGetBlockNumber(buf) : 0, offnum);

How about passing in the page and block number from the outside and not
dealing with a buffer in here at all?

>   		/*
>   		 * Dead line pointers can have index pointers pointing to them. So
> diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
> new file mode 100644
> index f626755..b37ecc4
> *** a/src/backend/utils/time/tqual.c
> --- b/src/backend/utils/time/tqual.c
> *************** static inline void
> *** 108,113 ****
> --- 108,117 ----
>   SetHintBits(HeapTupleHeader tuple, Buffer buffer,
>   			uint16 infomask, TransactionId xid)
>   {
> + 	/* we might not have a buffer if we are doing raw_heap_insert() */
> + 	if (!BufferIsValid(buffer))
> + 		return;
> + 
>   	if (TransactionIdIsValid(xid))
>   	{
>   		/* NB: xid must be known committed here! */

This looks seriously wrong to me.

I think the whole idea of doing this in private memory is bad. The
visibility routines aren't written for that, and the above is just one
instance of that, and I am far from convinced it's the only one. So you
either need to work out how to rely on the visibility checking done in
cluster.c, or you need to modify rewriteheap.c to write via
shared_buffers.

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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2014-02-15 17:50  Bruce Momjian <bruce@momjian.us>
  parent: Andres Freund <andres@2ndquadrant.com>
  0 siblings, 1 reply; 22+ messages in thread

From: Bruce Momjian @ 2014-02-15 17:50 UTC (permalink / raw)
  To: Andres Freund <andres@2ndquadrant.com>; +Cc: Jaime Casanova <jaime@2ndquadrant.com>; Robert Haas <robertmhaas@gmail.com>; Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On Sat, Feb 15, 2014 at 04:16:40PM +0100, Andres Freund wrote:
> Hi,
> 
> > *************** end_heap_rewrite(RewriteState state)
> > *** 281,286 ****
> > --- 284,290 ----
> >   						true);
> >   		RelationOpenSmgr(state->rs_new_rel);
> >   
> > + 		update_page_vm(state->rs_new_rel, state->rs_buffer, state->rs_blockno);
> >   		PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
> >   
> >   		smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, state->rs_blockno,
> > *************** raw_heap_insert(RewriteState state, Heap
> > *** 633,638 ****
> > --- 637,643 ----
> >   			 */
> >   			RelationOpenSmgr(state->rs_new_rel);
> >   
> > + 			update_page_vm(state->rs_new_rel, page, state->rs_blockno);
> >   			PageSetChecksumInplace(page, state->rs_blockno);
> >   
> >   			smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM,
> 
> I think those two cases should be combined.

Uh, what I did was to pair the new update_page_vm with the existing
PageSetChecksumInplace calls, figuring if we needed a checksum before we
wrote the page we would need a update_page_vm too.  Is that incorrect? 
It is that _last_ page write in the second instance.

> > + static void
> > + update_page_vm(Relation relation, Page page, BlockNumber blkno)
> > + {
> > + 	Buffer		vmbuffer = InvalidBuffer;
> > + 	TransactionId visibility_cutoff_xid;
> > + 
> > + 	visibilitymap_pin(relation, blkno, &vmbuffer);
> > + 	Assert(BufferIsValid(vmbuffer));
> > + 
> > + 	if (!visibilitymap_test(relation, blkno, &vmbuffer) &&
> > + 		heap_page_is_all_visible(relation, InvalidBuffer, page,
> > + 								 &visibility_cutoff_xid))
> > + 	{
> > + 		PageSetAllVisible(page);
> > + 		visibilitymap_set(relation, blkno, InvalidBuffer,
> > + 						  InvalidXLogRecPtr, vmbuffer, visibility_cutoff_xid);
> > + 	}
> > + 	ReleaseBuffer(vmbuffer);
> > + }
> 
> How could visibilitymap_test() be true here?

Oh, you are right that I can only hit that once per page;  test removed.

> > diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
> > new file mode 100644
> > index 899ffac..3e7546b
> > *** a/src/backend/access/heap/visibilitymap.c
> > --- b/src/backend/access/heap/visibilitymap.c
> > *************** visibilitymap_set(Relation rel, BlockNum
> > *** 257,263 ****
> >   #endif
> >   
> >   	Assert(InRecovery || XLogRecPtrIsInvalid(recptr));
> > - 	Assert(InRecovery || BufferIsValid(heapBuf));
> >   
> >   	/* Check that we have the right heap page pinned, if present */
> >   	if (BufferIsValid(heapBuf) && BufferGetBlockNumber(heapBuf) != heapBlk)
> > --- 257,262 ----
> > *************** visibilitymap_set(Relation rel, BlockNum
> > *** 278,284 ****
> >   		map[mapByte] |= (1 << mapBit);
> >   		MarkBufferDirty(vmBuf);
> >   
> > ! 		if (RelationNeedsWAL(rel))
> >   		{
> >   			if (XLogRecPtrIsInvalid(recptr))
> >   			{
> > --- 277,283 ----
> >   		map[mapByte] |= (1 << mapBit);
> >   		MarkBufferDirty(vmBuf);
> >   
> > ! 		if (RelationNeedsWAL(rel) && BufferIsValid(heapBuf))
> >   		{
> >   			if (XLogRecPtrIsInvalid(recptr))
> >   			{
> 
> At the very least this needs to revise visibilitymap_set's comment about
> the requirement of passing a buffer unless !InRecovery.

Oh, good point, comment block updated.

> Also, how is this going to work with replication if you're explicitly
> not WAL logging this?

Uh, I assumed that using the existing functions would handle this.  If
not, I don't know the answer.

> > *************** vac_cmp_itemptr(const void *left, const
> > *** 1730,1743 ****
> >    * transactions. Also return the visibility_cutoff_xid which is the highest
> >    * xmin amongst the visible tuples.
> >    */
> > ! static bool
> > ! heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId *visibility_cutoff_xid)
> >   {
> > - 	Page		page = BufferGetPage(buf);
> >   	OffsetNumber offnum,
> >   				maxoff;
> >   	bool		all_visible = true;
> >   
> >   	*visibility_cutoff_xid = InvalidTransactionId;
> >   
> >   	/*
> > --- 1728,1744 ----
> >    * transactions. Also return the visibility_cutoff_xid which is the highest
> >    * xmin amongst the visible tuples.
> >    */
> > ! bool
> > ! heap_page_is_all_visible(Relation rel, Buffer buf, Page page,
> > ! 						 TransactionId *visibility_cutoff_xid)
> >   {
> >   	OffsetNumber offnum,
> >   				maxoff;
> >   	bool		all_visible = true;
> >   
> > + 	if (BufferIsValid(buf))
> > + 		page = BufferGetPage(buf);
> > + 
> >   	*visibility_cutoff_xid = InvalidTransactionId;
> >   
> >   	/*
> > *************** heap_page_is_all_visible(Relation rel, B
> > *** 1758,1764 ****
> >   		if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
> >   			continue;
> >   
> > ! 		ItemPointerSet(&(tuple.t_self), BufferGetBlockNumber(buf), offnum);
> >   
> >   		/*
> >   		 * Dead line pointers can have index pointers pointing to them. So
> > --- 1759,1767 ----
> >   		if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
> >   			continue;
> >   
> > ! 		/* XXX use 0 or real offset? */
> > ! 		ItemPointerSet(&(tuple.t_self), BufferIsValid(buf) ?
> > ! 					   BufferGetBlockNumber(buf) : 0, offnum);
> 
> How about passing in the page and block number from the outside and not
> dealing with a buffer in here at all?

I would love to do that but HeapTupleSatisfiesVacuum() requires a
buffer, though you can (with my patch) optionally not supply one,
meaning if I passed in just the block number I would still need to
generate a buffer pointer.

> >   		/*
> >   		 * Dead line pointers can have index pointers pointing to them. So
> > diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
> > new file mode 100644
> > index f626755..b37ecc4
> > *** a/src/backend/utils/time/tqual.c
> > --- b/src/backend/utils/time/tqual.c
> > *************** static inline void
> > *** 108,113 ****
> > --- 108,117 ----
> >   SetHintBits(HeapTupleHeader tuple, Buffer buffer,
> >   			uint16 infomask, TransactionId xid)
> >   {
> > + 	/* we might not have a buffer if we are doing raw_heap_insert() */
> > + 	if (!BufferIsValid(buffer))
> > + 		return;
> > + 
> >   	if (TransactionIdIsValid(xid))
> >   	{
> >   		/* NB: xid must be known committed here! */
> 
> This looks seriously wrong to me.
> 
> I think the whole idea of doing this in private memory is bad. The
> visibility routines aren't written for that, and the above is just one
> instance of that, and I am far from convinced it's the only one. So you
> either need to work out how to rely on the visibility checking done in
> cluster.c, or you need to modify rewriteheap.c to write via
> shared_buffers.

I don't think we can change rewriteheap.c to use shared buffers --- that
code was written to do things in private memory for performance reasons
and I don't think setting hit bits justifies changing that.

Can you be more specific about the cluster.c idea?  I see
copy_heap_data() in cluster.c calling HeapTupleSatisfiesVacuum() with a
'buf' just like the code I am working in.

Based on Robert's feedback a few months ago I suspected I would need
help completing this patch --- now I am sure.

Updated patch attached.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

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

Attachments:

  [text/x-diff] vacuum.diff (8.3K, ../../20140215175014.GB3651@momjian.us/2-vacuum.diff)
  download | inline diff:
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
new file mode 100644
index c34ab98..7a9fe1f
*** a/src/backend/access/heap/rewriteheap.c
--- b/src/backend/access/heap/rewriteheap.c
***************
*** 107,112 ****
--- 107,114 ----
  #include "access/rewriteheap.h"
  #include "access/transam.h"
  #include "access/tuptoaster.h"
+ #include "access/visibilitymap.h"
+ #include "commands/vacuum.h"
  #include "storage/bufmgr.h"
  #include "storage/smgr.h"
  #include "utils/memutils.h"
*************** typedef OldToNewMappingData *OldToNewMap
*** 172,177 ****
--- 174,180 ----
  
  /* prototypes for internal functions */
  static void raw_heap_insert(RewriteState state, HeapTuple tup);
+ static void update_page_vm(Relation relation, Page page, BlockNumber blkno);
  
  
  /*
*************** end_heap_rewrite(RewriteState state)
*** 281,286 ****
--- 284,290 ----
  						true);
  		RelationOpenSmgr(state->rs_new_rel);
  
+ 		update_page_vm(state->rs_new_rel, state->rs_buffer, state->rs_blockno);
  		PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
  
  		smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, state->rs_blockno,
*************** raw_heap_insert(RewriteState state, Heap
*** 633,638 ****
--- 637,643 ----
  			 */
  			RelationOpenSmgr(state->rs_new_rel);
  
+ 			update_page_vm(state->rs_new_rel, page, state->rs_blockno);
  			PageSetChecksumInplace(page, state->rs_blockno);
  
  			smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM,
*************** raw_heap_insert(RewriteState state, Heap
*** 678,680 ****
--- 683,704 ----
  	if (heaptup != tup)
  		heap_freetuple(heaptup);
  }
+ 
+ static void
+ update_page_vm(Relation relation, Page page, BlockNumber blkno)
+ {
+ 	Buffer		vmbuffer = InvalidBuffer;
+ 	TransactionId visibility_cutoff_xid;
+ 
+ 	visibilitymap_pin(relation, blkno, &vmbuffer);
+ 	Assert(BufferIsValid(vmbuffer));
+ 
+ 	if (heap_page_is_all_visible(relation, InvalidBuffer, page,
+ 								 &visibility_cutoff_xid))
+ 	{
+ 		PageSetAllVisible(page);
+ 		visibilitymap_set(relation, blkno, InvalidBuffer,
+ 						  InvalidXLogRecPtr, vmbuffer, visibility_cutoff_xid);
+ 	}
+ 	ReleaseBuffer(vmbuffer);
+ }
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
new file mode 100644
index 899ffac..77122e1
*** a/src/backend/access/heap/visibilitymap.c
--- b/src/backend/access/heap/visibilitymap.c
*************** visibilitymap_pin_ok(BlockNumber heapBlk
*** 234,242 ****
   * InvalidTransactionId if the page contains no tuples.
   *
   * Caller is expected to set the heap page's PD_ALL_VISIBLE bit before calling
!  * this function. Except in recovery, caller should also pass the heap
!  * buffer. When checksums are enabled and we're not in recovery, we must add
!  * the heap buffer to the WAL chain to protect it from being torn.
   *
   * You must pass a buffer containing the correct map page to this function.
   * Call visibilitymap_pin first to pin the right one. This function doesn't do
--- 234,242 ----
   * InvalidTransactionId if the page contains no tuples.
   *
   * Caller is expected to set the heap page's PD_ALL_VISIBLE bit before calling
!  * this function. Passing in the heap buffer is optional.  When checksums are
!  * enabled and we're not in recovery, we must add the heap buffer to the WAL
!  * chain to protect it from being torn.
   *
   * You must pass a buffer containing the correct map page to this function.
   * Call visibilitymap_pin first to pin the right one. This function doesn't do
*************** visibilitymap_set(Relation rel, BlockNum
*** 257,263 ****
  #endif
  
  	Assert(InRecovery || XLogRecPtrIsInvalid(recptr));
- 	Assert(InRecovery || BufferIsValid(heapBuf));
  
  	/* Check that we have the right heap page pinned, if present */
  	if (BufferIsValid(heapBuf) && BufferGetBlockNumber(heapBuf) != heapBlk)
--- 257,262 ----
*************** visibilitymap_set(Relation rel, BlockNum
*** 278,284 ****
  		map[mapByte] |= (1 << mapBit);
  		MarkBufferDirty(vmBuf);
  
! 		if (RelationNeedsWAL(rel))
  		{
  			if (XLogRecPtrIsInvalid(recptr))
  			{
--- 277,283 ----
  		map[mapByte] |= (1 << mapBit);
  		MarkBufferDirty(vmBuf);
  
! 		if (RelationNeedsWAL(rel) && BufferIsValid(heapBuf))
  		{
  			if (XLogRecPtrIsInvalid(recptr))
  			{
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
new file mode 100644
index d77892e..2c34359
*** a/src/backend/commands/vacuumlazy.c
--- b/src/backend/commands/vacuumlazy.c
*************** static void lazy_record_dead_tuple(LVRel
*** 152,159 ****
  					   ItemPointer itemptr);
  static bool lazy_tid_reaped(ItemPointer itemptr, void *state);
  static int	vac_cmp_itemptr(const void *left, const void *right);
- static bool heap_page_is_all_visible(Relation rel, Buffer buf,
- 						 TransactionId *visibility_cutoff_xid);
  
  
  /*
--- 152,157 ----
*************** lazy_vacuum_page(Relation onerel, BlockN
*** 1234,1240 ****
  	 * check if the page has become all-visible.
  	 */
  	if (!visibilitymap_test(onerel, blkno, vmbuffer) &&
! 		heap_page_is_all_visible(onerel, buffer, &visibility_cutoff_xid))
  	{
  		Assert(BufferIsValid(*vmbuffer));
  		PageSetAllVisible(page);
--- 1232,1238 ----
  	 * check if the page has become all-visible.
  	 */
  	if (!visibilitymap_test(onerel, blkno, vmbuffer) &&
! 		heap_page_is_all_visible(onerel, buffer, NULL, &visibility_cutoff_xid))
  	{
  		Assert(BufferIsValid(*vmbuffer));
  		PageSetAllVisible(page);
*************** vac_cmp_itemptr(const void *left, const
*** 1732,1745 ****
   * transactions. Also return the visibility_cutoff_xid which is the highest
   * xmin amongst the visible tuples.
   */
! static bool
! heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId *visibility_cutoff_xid)
  {
- 	Page		page = BufferGetPage(buf);
  	OffsetNumber offnum,
  				maxoff;
  	bool		all_visible = true;
  
  	*visibility_cutoff_xid = InvalidTransactionId;
  
  	/*
--- 1730,1746 ----
   * transactions. Also return the visibility_cutoff_xid which is the highest
   * xmin amongst the visible tuples.
   */
! bool
! heap_page_is_all_visible(Relation rel, Buffer buf, Page page,
! 						 TransactionId *visibility_cutoff_xid)
  {
  	OffsetNumber offnum,
  				maxoff;
  	bool		all_visible = true;
  
+ 	if (BufferIsValid(buf))
+ 		page = BufferGetPage(buf);
+ 
  	*visibility_cutoff_xid = InvalidTransactionId;
  
  	/*
*************** heap_page_is_all_visible(Relation rel, B
*** 1760,1766 ****
  		if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
  			continue;
  
! 		ItemPointerSet(&(tuple.t_self), BufferGetBlockNumber(buf), offnum);
  
  		/*
  		 * Dead line pointers can have index pointers pointing to them. So
--- 1761,1769 ----
  		if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
  			continue;
  
! 		/* XXX use 0 or real offset? */
! 		ItemPointerSet(&(tuple.t_self), BufferIsValid(buf) ?
! 					   BufferGetBlockNumber(buf) : 0, offnum);
  
  		/*
  		 * Dead line pointers can have index pointers pointing to them. So
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
new file mode 100644
index f626755..b37ecc4
*** a/src/backend/utils/time/tqual.c
--- b/src/backend/utils/time/tqual.c
*************** static inline void
*** 108,113 ****
--- 108,117 ----
  SetHintBits(HeapTupleHeader tuple, Buffer buffer,
  			uint16 infomask, TransactionId xid)
  {
+ 	/* we might not have a buffer if we are doing raw_heap_insert() */
+ 	if (!BufferIsValid(buffer))
+ 		return;
+ 
  	if (TransactionIdIsValid(xid))
  	{
  		/* NB: xid must be known committed here! */
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
new file mode 100644
index 70350e0..230c56e
*** a/src/include/commands/vacuum.h
--- b/src/include/commands/vacuum.h
***************
*** 19,24 ****
--- 19,25 ----
  #include "catalog/pg_type.h"
  #include "nodes/parsenodes.h"
  #include "storage/buf.h"
+ #include "storage/bufpage.h"
  #include "storage/lock.h"
  #include "utils/relcache.h"
  
*************** extern void vacuum_delay_point(void);
*** 172,177 ****
--- 173,180 ----
  /* in commands/vacuumlazy.c */
  extern void lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
  				BufferAccessStrategy bstrategy);
+ extern bool heap_page_is_all_visible(Relation rel, Buffer buf, Page page,
+ 						 TransactionId *visibility_cutoff_xid);
  
  /* in commands/analyze.c */
  extern void analyze_rel(Oid relid, VacuumStmt *vacstmt,

^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2014-02-15 18:08  Andres Freund <andres@2ndquadrant.com>
  parent: Bruce Momjian <bruce@momjian.us>
  0 siblings, 1 reply; 22+ messages in thread

From: Andres Freund @ 2014-02-15 18:08 UTC (permalink / raw)
  To: Bruce Momjian <bruce@momjian.us>; +Cc: Jaime Casanova <jaime@2ndquadrant.com>; Robert Haas <robertmhaas@gmail.com>; Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On 2014-02-15 12:50:14 -0500, Bruce Momjian wrote:

> On Sat, Feb 15, 2014 at 04:16:40PM +0100, Andres Freund wrote:
> > Hi,
> > 
> > > *************** end_heap_rewrite(RewriteState state)
> > > *** 281,286 ****
> > > --- 284,290 ----
> > >   						true);
> > >   		RelationOpenSmgr(state->rs_new_rel);
> > >   
> > > + 		update_page_vm(state->rs_new_rel, state->rs_buffer, state->rs_blockno);
> > >   		PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
> > >   
> > >   		smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, state->rs_blockno,
> > > *************** raw_heap_insert(RewriteState state, Heap
> > > *** 633,638 ****
> > > --- 637,643 ----
> > >   			 */
> > >   			RelationOpenSmgr(state->rs_new_rel);
> > >   
> > > + 			update_page_vm(state->rs_new_rel, page, state->rs_blockno);
> > >   			PageSetChecksumInplace(page, state->rs_blockno);
> > >   
> > >   			smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM,
> > 
> > I think those two cases should be combined.
> 
> Uh, what I did was to pair the new update_page_vm with the existing
> PageSetChecksumInplace calls, figuring if we needed a checksum before we
> wrote the page we would need a update_page_vm too.  Is that incorrect?
> It is that _last_ page write in the second instance.

What I mean is that there should be a new function handling the writeout
of a page.

> > > diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
> > > new file mode 100644
> > > index 899ffac..3e7546b
> > > *** a/src/backend/access/heap/visibilitymap.c
> > > --- b/src/backend/access/heap/visibilitymap.c
> > > *************** visibilitymap_set(Relation rel, BlockNum
> > > *** 257,263 ****
> > >   #endif
> > >   
> > >   	Assert(InRecovery || XLogRecPtrIsInvalid(recptr));
> > > - 	Assert(InRecovery || BufferIsValid(heapBuf));
> > >   
> > >   	/* Check that we have the right heap page pinned, if present */
> > >   	if (BufferIsValid(heapBuf) && BufferGetBlockNumber(heapBuf) != heapBlk)
> > > --- 257,262 ----
> > > *************** visibilitymap_set(Relation rel, BlockNum
> > > *** 278,284 ****
> > >   		map[mapByte] |= (1 << mapBit);
> > >   		MarkBufferDirty(vmBuf);
> > >   
> > > ! 		if (RelationNeedsWAL(rel))
> > >   		{
> > >   			if (XLogRecPtrIsInvalid(recptr))
> > >   			{
> > > --- 277,283 ----
> > >   		map[mapByte] |= (1 << mapBit);
> > >   		MarkBufferDirty(vmBuf);
> > >   
> > > ! 		if (RelationNeedsWAL(rel) && BufferIsValid(heapBuf))
> > >   		{
> > >   			if (XLogRecPtrIsInvalid(recptr))
> > >   			{
> > 
> > At the very least this needs to revise visibilitymap_set's comment about
> > the requirement of passing a buffer unless !InRecovery.
> 
> Oh, good point, comment block updated.
> 
> > Also, how is this going to work with replication if you're explicitly
> > not WAL logging this?
> 
> Uh, I assumed that using the existing functions would handle this.  If
> not, I don't know the answer.

Err. Read the block above again, where you added the
BufferIsValid(heapBuf) check. That's exactly the part doing the WAL
logging.

> > > *** a/src/backend/utils/time/tqual.c
> > > --- b/src/backend/utils/time/tqual.c
> > > *************** static inline void
> > > *** 108,113 ****
> > > --- 108,117 ----
> > >   SetHintBits(HeapTupleHeader tuple, Buffer buffer,
> > >   			uint16 infomask, TransactionId xid)
> > >   {
> > > + 	/* we might not have a buffer if we are doing raw_heap_insert() */
> > > + 	if (!BufferIsValid(buffer))
> > > + 		return;
> > > + 
> > >   	if (TransactionIdIsValid(xid))
> > >   	{
> > >   		/* NB: xid must be known committed here! */
> > 
> > This looks seriously wrong to me.
> > 
> > I think the whole idea of doing this in private memory is bad. The
> > visibility routines aren't written for that, and the above is just one
> > instance of that, and I am far from convinced it's the only one. So you
> > either need to work out how to rely on the visibility checking done in
> > cluster.c, or you need to modify rewriteheap.c to write via
> > shared_buffers.
> 
> I don't think we can change rewriteheap.c to use shared buffers --- that
> code was written to do things in private memory for performance reasons
> and I don't think setting hit bits justifies changing that.

I don't think that's necessarily contradictory. You'd need to use a
ringbuffer strategy for IO, like e.g. VACUUM does. But I admit it's not
a insignificant amount of work, and it might need some adjustements in
some places.

> Can you be more specific about the cluster.c idea?  I see
> copy_heap_data() in cluster.c calling HeapTupleSatisfiesVacuum() with a
> 'buf' just like the code I am working in.

Yes, it does. But in contrast to your patch it does so on the *origin*
buffer. Which is in shared memory.
The idea would be to modify rewrite_heap_tuple() to get a new parameter
informing it it about the tuple's visibility. That'd allow you to avoid
doing any additional visibility checks.

Unfortunately that would still not fix the problem that
visibilitymap_set requires a real buffer to be passed in. If you're
going that route, you'll need to make a bit more sweeping changes. You'd
need to add new blockno parameter and a BufferIsValid() check to the
right places in log_heap_visible(). Pretty ugly if you ask me...

Greetings,

Andres Freund

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2014-02-16 02:34  Bruce Momjian <bruce@momjian.us>
  parent: Andres Freund <andres@2ndquadrant.com>
  0 siblings, 1 reply; 22+ messages in thread

From: Bruce Momjian @ 2014-02-16 02:34 UTC (permalink / raw)
  To: Andres Freund <andres@2ndquadrant.com>; +Cc: Jaime Casanova <jaime@2ndquadrant.com>; Robert Haas <robertmhaas@gmail.com>; Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On Sat, Feb 15, 2014 at 07:08:40PM +0100, Andres Freund wrote:
> > Can you be more specific about the cluster.c idea?  I see
> > copy_heap_data() in cluster.c calling HeapTupleSatisfiesVacuum() with a
> > 'buf' just like the code I am working in.
> 
> Yes, it does. But in contrast to your patch it does so on the *origin*
> buffer. Which is in shared memory.
> The idea would be to modify rewrite_heap_tuple() to get a new parameter
> informing it it about the tuple's visibility. That'd allow you to avoid
> doing any additional visibility checks.
> 
> Unfortunately that would still not fix the problem that
> visibilitymap_set requires a real buffer to be passed in. If you're
> going that route, you'll need to make a bit more sweeping changes. You'd
> need to add new blockno parameter and a BufferIsValid() check to the
> right places in log_heap_visible(). Pretty ugly if you ask me...

Thank you for the thorough review.  Unless someone else can complete
this, I think it should be marked as returned with feedback.  I don't
think I am going to learn enough to complete this during the
commit-fest.

Since learning of the limitations in setting vmmap bits for index-only
scans in October, I have been unable to improve VACUUM/CLUSTER, and
unable to improve autovacuum --- a double failure.  I guess there is
always PG 9.5.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2014-02-16 10:49  Andres Freund <andres@2ndquadrant.com>
  parent: Bruce Momjian <bruce@momjian.us>
  0 siblings, 0 replies; 22+ messages in thread

From: Andres Freund @ 2014-02-16 10:49 UTC (permalink / raw)
  To: Bruce Momjian <bruce@momjian.us>; +Cc: Jaime Casanova <jaime@2ndquadrant.com>; Robert Haas <robertmhaas@gmail.com>; Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On 2014-02-15 21:34:15 -0500, Bruce Momjian wrote:
> Thank you for the thorough review.  Unless someone else can complete
> this, I think it should be marked as returned with feedback.  I don't
> think I am going to learn enough to complete this during the
> commit-fest.

Agreed. Marked it as such.


> I guess there is always PG 9.5.

I sure hope so ;)

Greetings,

Andres Freund

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2014-02-16 20:19  Jim Nasby <jim@nasby.net>
  parent: Jaime Casanova <jaime@2ndquadrant.com>
  1 sibling, 1 reply; 22+ messages in thread

From: Jim Nasby @ 2014-02-16 20:19 UTC (permalink / raw)
  To: Jaime Casanova <jaime@2ndquadrant.com>; Bruce Momjian <bruce@momjian.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On 1/24/14, 3:52 PM, Jaime Casanova wrote:
> On Tue, Dec 3, 2013 at 11:25 AM, Bruce Momjian<bruce@momjian.us>  wrote:
>> >
>> >Is everyone else OK with this approach?  Updated patch attached.
>> >
> Hi,
>
> I started to look at this patch and i found that it fails an assertion
> as soon as you run a VACUUM FULL after a lazy VACUUM even if those are
> on unrelated relations. For example in an assert-enabled build with
> the regression database run:
>
> VACUUM customer;
> [... insert here whatever commands you like or nothing at all ...]
> VACUUM FULL tenk1;

Is anyone else confused/concerned that regression testing didn't pick this up? The vacuum.sql test does not test lazy vacuum at all, and I can't seem to find any other tests that test lazy vacuum either...
-- 
Jim C. Nasby, Data Architect                       jim@nasby.net
512.569.9461 (cell)                         http://jim.nasby.net


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread

* Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL
@ 2014-02-18 09:04  Heikki Linnakangas <hlinnakangas@vmware.com>
  parent: Jim Nasby <jim@nasby.net>
  0 siblings, 0 replies; 22+ messages in thread

From: Heikki Linnakangas @ 2014-02-18 09:04 UTC (permalink / raw)
  To: Jim Nasby <jim@nasby.net>; +Cc: Jaime Casanova <jaime@2ndquadrant.com>; Bruce Momjian <bruce@momjian.us>; Robert Haas <robertmhaas@gmail.com>; Kevin Grittner <kgrittn@mail.com>; Amit Kapila <amit.kapila@huawei.com>; Josh Berkus <josh@agliodbs.com>; pgsql-hackers

On 02/16/2014 10:19 PM, Jim Nasby wrote:
> On 1/24/14, 3:52 PM, Jaime Casanova wrote:
>> On Tue, Dec 3, 2013 at 11:25 AM, Bruce Momjian<bruce@momjian.us>  wrote:
>>>>
>>>> Is everyone else OK with this approach?  Updated patch attached.
>>>>
>> Hi,
>>
>> I started to look at this patch and i found that it fails an assertion
>> as soon as you run a VACUUM FULL after a lazy VACUUM even if those are
>> on unrelated relations. For example in an assert-enabled build with
>> the regression database run:
>>
>> VACUUM customer;
>> [... insert here whatever commands you like or nothing at all ...]
>> VACUUM FULL tenk1;
>
> Is anyone else confused/concerned that regression testing didn't pick this up?

I wouldn't expect that to be explicitly tested - it's pretty unexpected 
that they work independently but not when run one after another. But 
it's a bit surprising that we don't happen to do that combination in any 
of the tests by pure chance.

> The vacuum.sql test does not test lazy vacuum at all, and I can't seem to find any other tests that test lazy vacuum either...

There are several lazy vacuums in the regression suite:

sql/alter_table.sql:vacuum analyze atacc1(a);
sql/alter_table.sql:vacuum analyze atacc1("........pg.dropped.1........");
sql/hs_standby_disallowed.sql:VACUUM hs2;
sql/indirect_toast.sql:VACUUM FREEZE toasttest;
sql/indirect_toast.sql:VACUUM FREEZE toasttest;
sql/matview.sql:VACUUM ANALYZE hogeview;
sql/numeric_big.sql:VACUUM ANALYZE num_exp_add;
sql/numeric_big.sql:VACUUM ANALYZE num_exp_sub;
sql/numeric_big.sql:VACUUM ANALYZE num_exp_div;
sql/numeric_big.sql:VACUUM ANALYZE num_exp_mul;
sql/numeric_big.sql:VACUUM ANALYZE num_exp_sqrt;
sql/numeric_big.sql:VACUUM ANALYZE num_exp_ln;
sql/numeric_big.sql:VACUUM ANALYZE num_exp_log10;
sql/numeric_big.sql:VACUUM ANALYZE num_exp_power_10_ln;
sql/numeric.sql:VACUUM ANALYZE num_exp_add;
sql/numeric.sql:VACUUM ANALYZE num_exp_sub;
sql/numeric.sql:VACUUM ANALYZE num_exp_div;
sql/numeric.sql:VACUUM ANALYZE num_exp_mul;
sql/numeric.sql:VACUUM ANALYZE num_exp_sqrt;
sql/numeric.sql:VACUUM ANALYZE num_exp_ln;
sql/numeric.sql:VACUUM ANALYZE num_exp_log10;
sql/numeric.sql:VACUUM ANALYZE num_exp_power_10_ln;
sql/sanity_check.sql:VACUUM;
sql/without_oid.sql:VACUUM ANALYZE wi;
sql/without_oid.sql:VACUUM ANALYZE wo;

Most of those commands are there to analyze, rather than vacuum, but 
lazy vacuum is definitely exercised by the regression tests. I agree 
it's quite surprising that vacuum.sql doesn't actually perform any lazy 
vacuums.

- Heikki


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



^ permalink  raw  reply  [nested|flat] 22+ messages in thread


end of thread, other threads:[~2014-02-18 09:04 UTC | newest]

Thread overview: 22+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2013-01-09 23:07 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL Bruce Momjian <bruce@momjian.us>
2013-01-10 00:38 ` Josh Berkus <josh@agliodbs.com>
2013-01-10 06:15   ` Amit Kapila <amit.kapila@huawei.com>
2013-01-10 06:31   ` Pavan Deolasee <pavan.deolasee@gmail.com>
2013-01-10 09:18     ` Amit Kapila <amit.kapila@huawei.com>
2013-01-12 19:14 Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL Kevin Grittner <kgrittn@mail.com>
2013-11-27 21:33 ` Bruce Momjian <bruce@momjian.us>
2013-11-28 22:17   ` Robert Haas <robertmhaas@gmail.com>
2013-11-28 22:38     ` Bruce Momjian <bruce@momjian.us>
2013-12-03 16:25       ` Bruce Momjian <bruce@momjian.us>
2013-12-03 19:01         ` Robert Haas <robertmhaas@gmail.com>
2013-12-03 19:34           ` Bruce Momjian <bruce@momjian.us>
2014-01-24 21:52         ` Jaime Casanova <jaime@2ndquadrant.com>
2014-01-25 02:09           ` Bruce Momjian <bruce@momjian.us>
2014-02-15 15:16             ` Andres Freund <andres@2ndquadrant.com>
2014-02-15 17:50               ` Bruce Momjian <bruce@momjian.us>
2014-02-15 18:08                 ` Andres Freund <andres@2ndquadrant.com>
2014-02-16 02:34                   ` Bruce Momjian <bruce@momjian.us>
2014-02-16 10:49                     ` Andres Freund <andres@2ndquadrant.com>
2014-02-16 20:19           ` Jim Nasby <jim@nasby.net>
2014-02-18 09:04             ` Heikki Linnakangas <hlinnakangas@vmware.com>
2014-01-16 01:29     ` Simon Riggs <simon@2ndQuadrant.com>

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox