Re: [PATCHES] Fast COPY after TRUNCATE bug and fix

2007-03-03 Thread Bruce Momjian 

Patch applied.  Thanks.

---


Simon Riggs wrote:
 It's been pointed out to me that I introduced a bug as part of the
 recent optimisation of COPY-after-truncate. 
 
 The attached patch fixes this for me on CVS HEAD. It does this by making
 an explicit request for relcache hint cleanup at EOXact and takes a more
 cautious approach during RelationCacheInvalidate().
 
 Please can this be reviewed as soon as possible? Thanks.
 
 TRUNCATE was setting a flag to show that it had created a new
 relfilenode, but the flag was not cleared in all cases. This lead to a
 COPY that followed a truncation, yet was in a *separate* transaction
 from it and in a transaction on its own, to apparently lose data. The
 data loss was caused because the COPY inadvertently avoided writing WAL,
 which then led to skipping the recording of transaction commit, leaving
 the inserted rows showing as aborted.
 
 The failing test case was:
 
 TRUNCATE foo;
 COPY foo FROM ;
 SELECT count(*) FROM foo;
 
 The returned count should be non-zero if the COPY succeeds, yet on CVS
 HEAD this currently returns 0.
 
 CLUSTER is not affected by this change, AFAICS, because its change of
 relfilenode doesn't wait until EOXact, so COPY doesn't optimise after a
 CLUSTER-in-same-trans.
 
 Thanks to various EDB colleagues for bringing this to my attention.
 
 -- 
   Simon Riggs 
   EnterpriseDB   http://www.enterprisedb.com
 

[ Attachment, skipping... ]

 
 ---(end of broadcast)---
 TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly

-- 
  Bruce Momjian  [EMAIL PROTECTED]  http://momjian.us
  EnterpriseDB   http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster

Re: [PATCHES] Fast COPY after TRUNCATE bug and fix

2007-03-01 Thread Simon Riggs 
On Wed, 2007-02-28 at 21:09 -0500, Bruce Momjian wrote:
 Your patch has been added to the PostgreSQL unapplied patches list at:

 Simon Riggs wrote:
  It's been pointed out to me that I introduced a bug as part of the
  recent optimisation of COPY-after-truncate. 
  
  The attached patch fixes this for me on CVS HEAD. It does this by making
  an explicit request for relcache hint cleanup at EOXact and takes a more
  cautious approach during RelationCacheInvalidate().

You understand that this fixes a bug in CVS HEAD?

It isn't a new feature.

-- 
  Simon Riggs 
  EnterpriseDB   http://www.enterprisedb.com



---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster

Re: [PATCHES] Fast COPY after TRUNCATE bug and fix

2007-03-01 Thread Bruce Momjian 
Simon Riggs wrote:
 On Wed, 2007-02-28 at 21:09 -0500, Bruce Momjian wrote:
  Your patch has been added to the PostgreSQL unapplied patches list at:
 
  Simon Riggs wrote:
   It's been pointed out to me that I introduced a bug as part of the
   recent optimisation of COPY-after-truncate. 
   
   The attached patch fixes this for me on CVS HEAD. It does this by making
   an explicit request for relcache hint cleanup at EOXact and takes a more
   cautious approach during RelationCacheInvalidate().
 
 You understand that this fixes a bug in CVS HEAD?
 
 It isn't a new feature.

This is for CVS HEAD only, right?  Fixed still go into the queue, but
for a shorter amount of time, hopefully.

-- 
  Bruce Momjian  [EMAIL PROTECTED]  http://momjian.us
  EnterpriseDB   http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

---(end of broadcast)---
TIP 6: explain analyze is your friend

Re: [PATCHES] Fast COPY after TRUNCATE bug and fix

2007-03-01 Thread Andrew Dunstan 


what is the point of this?:

+ void
+ RelationCacheResetAtEOXact(void)
+ {
+   need_eoxact_work = true;
+ }



and why is it declared extern in  relcache.h when it is only used in 
relcache.c?


ISTM that there isn't much reason to un-inline the statement, and the 
patch could be a lot smaller without it.


cheers

andrew

Bruce Momjian wrote:

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---


Simon Riggs wrote:
  

It's been pointed out to me that I introduced a bug as part of the
recent optimisation of COPY-after-truncate. 


The attached patch fixes this for me on CVS HEAD. It does this by making
an explicit request for relcache hint cleanup at EOXact and takes a more
cautious approach during RelationCacheInvalidate().

Please can this be reviewed as soon as possible? Thanks.

TRUNCATE was setting a flag to show that it had created a new
relfilenode, but the flag was not cleared in all cases. This lead to a
COPY that followed a truncation, yet was in a *separate* transaction
from it and in a transaction on its own, to apparently lose data. The
data loss was caused because the COPY inadvertently avoided writing WAL,
which then led to skipping the recording of transaction commit, leaving
the inserted rows showing as aborted.

The failing test case was:

TRUNCATE foo;
COPY foo FROM ;
SELECT count(*) FROM foo;

The returned count should be non-zero if the COPY succeeds, yet on CVS
HEAD this currently returns 0.

CLUSTER is not affected by this change, AFAICS, because its change of
relfilenode doesn't wait until EOXact, so COPY doesn't optimise after a
CLUSTER-in-same-trans.

Thanks to various EDB colleagues for bringing this to my attention.

--
  Simon Riggs 
  EnterpriseDB   http://www.enterprisedb.com





[ Attachment, skipping... ]

  

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly



  



---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings

Re: [PATCHES] Fast COPY after TRUNCATE bug and fix

2007-03-01 Thread Andrew Dunstan 


I'm sorry. I misread the patch. I now see it used in index.c. return to 
normal viewing ...


cheers

andrew


Andrew Dunstan wrote:


what is the point of this?:

+ void
+ RelationCacheResetAtEOXact(void)
+ {
+ need_eoxact_work = true;
+ }



and why is it declared extern in  relcache.h when it is only used in 
relcache.c?


ISTM that there isn't much reason to un-inline the statement, and the 
patch could be a lot smaller without it.


cheers

andrew

Bruce Momjian wrote:

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

--- 




Simon Riggs wrote:
 

It's been pointed out to me that I introduced a bug as part of the
recent optimisation of COPY-after-truncate.
The attached patch fixes this for me on CVS HEAD. It does this by 
making
an explicit request for relcache hint cleanup at EOXact and takes a 
more

cautious approach during RelationCacheInvalidate().

Please can this be reviewed as soon as possible? Thanks.

TRUNCATE was setting a flag to show that it had created a new
relfilenode, but the flag was not cleared in all cases. This lead to a
COPY that followed a truncation, yet was in a *separate* transaction
from it and in a transaction on its own, to apparently lose data. The
data loss was caused because the COPY inadvertently avoided writing 
WAL,

which then led to skipping the recording of transaction commit, leaving
the inserted rows showing as aborted.

The failing test case was:

TRUNCATE foo;
COPY foo FROM ;
SELECT count(*) FROM foo;

The returned count should be non-zero if the COPY succeeds, yet on CVS
HEAD this currently returns 0.

CLUSTER is not affected by this change, AFAICS, because its change of
relfilenode doesn't wait until EOXact, so COPY doesn't optimise after a
CLUSTER-in-same-trans.

Thanks to various EDB colleagues for bringing this to my attention.

--
  Simon Riggs   EnterpriseDB   http://www.enterprisedb.com




[ Attachment, skipping... ]

 
---(end of 
broadcast)---

TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that 
your

   message can get through to the mailing list cleanly



  



---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings




---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings

Re: [PATCHES] Fast COPY after TRUNCATE bug and fix

2007-03-01 Thread Simon Riggs 
On Thu, 2007-03-01 at 09:12 -0500, Andrew Dunstan wrote:
 what is the point of this?:
 
 + void
 + RelationCacheResetAtEOXact(void)
 + {
 + need_eoxact_work = true;
 + }
 
 
 
 and why is it declared extern in  relcache.h when it is only used in 
 relcache.c?

It is called from index.c and relcache.c, hence it is extern.

-- 
  Simon Riggs 
  EnterpriseDB   http://www.enterprisedb.com



---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org

Re: [PATCHES] Fast COPY after TRUNCATE bug and fix

2007-03-01 Thread Andrew Dunstan 

Simon Riggs wrote:

On Thu, 2007-03-01 at 09:12 -0500, Andrew Dunstan wrote:
  

what is the point of this?:

+ void
+ RelationCacheResetAtEOXact(void)
+ {
+   need_eoxact_work = true;
+ }



and why is it declared extern in  relcache.h when it is only used in 
relcache.c?



It is called from index.c and relcache.c, hence it is extern.

  

Yeah. I noticed just after I hit the send button ... sorry.

cheers

andrew

---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
  choose an index scan if your joining column's datatypes do not
  match

Re: [PATCHES] Fast COPY after TRUNCATE bug and fix

2007-03-01 Thread Tom Lane 
Bruce Momjian [EMAIL PROTECTED] writes:
 Simon Riggs wrote:
 You understand that this fixes a bug in CVS HEAD?

 This is for CVS HEAD only, right?  Fixed still go into the queue, but
 for a shorter amount of time, hopefully.

It still needs review ... and the presence of the patch in the queue
reminds me that I never really reviewed the original patch.  Which now
proves to have been a mistake.

regards, tom lane

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster

Re: [PATCHES] Fast COPY after TRUNCATE bug and fix

2007-02-28 Thread Bruce Momjian 

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---


Simon Riggs wrote:
 It's been pointed out to me that I introduced a bug as part of the
 recent optimisation of COPY-after-truncate. 
 
 The attached patch fixes this for me on CVS HEAD. It does this by making
 an explicit request for relcache hint cleanup at EOXact and takes a more
 cautious approach during RelationCacheInvalidate().
 
 Please can this be reviewed as soon as possible? Thanks.
 
 TRUNCATE was setting a flag to show that it had created a new
 relfilenode, but the flag was not cleared in all cases. This lead to a
 COPY that followed a truncation, yet was in a *separate* transaction
 from it and in a transaction on its own, to apparently lose data. The
 data loss was caused because the COPY inadvertently avoided writing WAL,
 which then led to skipping the recording of transaction commit, leaving
 the inserted rows showing as aborted.
 
 The failing test case was:
 
 TRUNCATE foo;
 COPY foo FROM ;
 SELECT count(*) FROM foo;
 
 The returned count should be non-zero if the COPY succeeds, yet on CVS
 HEAD this currently returns 0.
 
 CLUSTER is not affected by this change, AFAICS, because its change of
 relfilenode doesn't wait until EOXact, so COPY doesn't optimise after a
 CLUSTER-in-same-trans.
 
 Thanks to various EDB colleagues for bringing this to my attention.
 
 -- 
   Simon Riggs 
   EnterpriseDB   http://www.enterprisedb.com
 

[ Attachment, skipping... ]

 
 ---(end of broadcast)---
 TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly

-- 
  Bruce Momjian  [EMAIL PROTECTED]  http://momjian.us
  EnterpriseDB   http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly

[PATCHES] Fast COPY after TRUNCATE bug and fix

2007-02-24 Thread Simon Riggs 
It's been pointed out to me that I introduced a bug as part of the
recent optimisation of COPY-after-truncate. 

The attached patch fixes this for me on CVS HEAD. It does this by making
an explicit request for relcache hint cleanup at EOXact and takes a more
cautious approach during RelationCacheInvalidate().

Please can this be reviewed as soon as possible? Thanks.

TRUNCATE was setting a flag to show that it had created a new
relfilenode, but the flag was not cleared in all cases. This lead to a
COPY that followed a truncation, yet was in a *separate* transaction
from it and in a transaction on its own, to apparently lose data. The
data loss was caused because the COPY inadvertently avoided writing WAL,
which then led to skipping the recording of transaction commit, leaving
the inserted rows showing as aborted.

The failing test case was:

TRUNCATE foo;
COPY foo FROM ;
SELECT count(*) FROM foo;

The returned count should be non-zero if the COPY succeeds, yet on CVS
HEAD this currently returns 0.

CLUSTER is not affected by this change, AFAICS, because its change of
relfilenode doesn't wait until EOXact, so COPY doesn't optimise after a
CLUSTER-in-same-trans.

Thanks to various EDB colleagues for bringing this to my attention.

-- 
  Simon Riggs 
  EnterpriseDB   http://www.enterprisedb.com

Index: src/backend/catalog/index.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/catalog/index.c,v
retrieving revision 1.279
diff -c -r1.279 index.c
*** src/backend/catalog/index.c	14 Feb 2007 01:58:56 -	1.279
--- src/backend/catalog/index.c	24 Feb 2007 15:37:45 -
***
*** 1250,1255 
--- 1250,1256 
  
  	/* Remember we did this in current transaction, to allow later optimisations */
  	relation-rd_newRelfilenodeSubid = GetCurrentSubTransactionId();
+ 	RelationCacheResetAtEOXact();
  
  	/* Make sure the relfilenode change is visible */
  	CommandCounterIncrement();
Index: src/backend/utils/cache/relcache.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/utils/cache/relcache.c,v
retrieving revision 1.255
diff -c -r1.255 relcache.c
*** src/backend/utils/cache/relcache.c	25 Jan 2007 02:17:26 -	1.255
--- src/backend/utils/cache/relcache.c	24 Feb 2007 15:37:55 -
***
*** 1915,1923 
   *	 so we do not touch new-in-transaction relations; they cannot be targets
   *	 of cross-backend SI updates (and our own updates now go through a
   *	 separate linked list that isn't limited by the SI message buffer size).
-  *	 We don't do anything special for newRelfilenode-in-transaction relations, 
-  *	 though since we have a lock on the relation nobody else should be 
-  *	 generating cache invalidation messages for it anyhow.
   *
   *	 We do this in two phases: the first pass deletes deletable items, and
   *	 the second one rebuilds the rebuildable items.  This is essential for
--- 1915,1920 
***
*** 1960,1965 
--- 1957,1970 
  		if (relation-rd_createSubid != InvalidSubTransactionId)
  			continue;
  
+ 		/* 
+ 		 * Reset newRelfilenode hint. It is never used for correctness, only
+ 		 * for performance optimization. An incorrectly set hint can lead
+ 		 * to data loss in some circumstances, so play safe.
+ 		 */
+ 		if (relation-rd_newRelfilenodeSubid != InvalidSubTransactionId)
+ 			relation-rd_newRelfilenodeSubid = InvalidSubTransactionId;
+ 
  		relcacheInvalsReceived++;
  
  		if (RelationHasReferenceCountZero(relation))
***
*** 2012,2017 
--- 2017,2033 
  }
  
  /*
+  * RelationCacheResetAtEOXact
+  *
+  *  Register that work will be required at main-transaction commit or abort
+  */
+ void
+ RelationCacheResetAtEOXact(void)
+ {
+ 	need_eoxact_work = true;
+ }
+ 
+ /*
   * AtEOXact_RelationCache
   *
   *	Clean up the relcache at main-transaction commit or abort.
***
*** 2161,2167 
  			if (isCommit)
  relation-rd_newRelfilenodeSubid = parentSubid;
  			else
! 			 relation-rd_newRelfilenodeSubid = InvalidSubTransactionId;
  		}
  
  		/*
--- 2177,2183 
  			if (isCommit)
  relation-rd_newRelfilenodeSubid = parentSubid;
  			else
! relation-rd_newRelfilenodeSubid = InvalidSubTransactionId;
  		}
  
  		/*
***
*** 2255,2261 
  	rel-rd_newRelfilenodeSubid = InvalidSubTransactionId;
  
  	/* must flag that we have rels created in this transaction */
! 	need_eoxact_work = true;
  
  	/* is it a temporary relation? */
  	rel-rd_istemp = isTempNamespace(relnamespace);
--- 2271,2277 
  	rel-rd_newRelfilenodeSubid = InvalidSubTransactionId;
  
  	/* must flag that we have rels created in this transaction */
! 	RelationCacheResetAtEOXact();
  
  	/* is it a temporary relation? */
  	rel-rd_istemp = isTempNamespace(relnamespace);
***
*** 2911,2917 
  	relation-rd_oidindex = oidIndex;
  	relation-rd_indexvalid = 2;	/* mark list