Re: [HACKERS] COPY FREEZE has no warning

2013-02-02 Thread Noah Misch
On Fri, Feb 01, 2013 at 12:57:18PM -0500, Bruce Momjian wrote:
 On Tue, Jan 29, 2013 at 08:34:24PM -0500, Noah Misch wrote:
  On Fri, Jan 25, 2013 at 11:28:58PM -0500, Bruce Momjian wrote:
 BEGIN;
 TRUNCATE vistest;
 SAVEPOINT s1;
 COPY vistest FROM stdin CSV FREEZE;
 ERROR:  cannot perform FREEZE because of previous table activity in the 
   current transaction
 COMMIT;
   
   Clearly it was truncated in the same transaction, but the savepoint
   somehow invalidates the freeze.  There is a C comment about it:
  
  The savepoint prevents the COPY FREEZE, because COPY FREEZE needs the table 
  to
  have been created or truncated in the current *sub*transaction.  Issuing
  RELEASE s1 before the COPY makes it work again, for example.
  
   
* BEGIN;
* TRUNCATE t;
* SAVEPOINT save;
* TRUNCATE t;
* ROLLBACK TO save;
* COPY ...
  
  This is different.  The table was truncated in the current subtransaction, 
  and
  it's safe in principle to apply the optimization.  Due to an implementation
  artifact, we'll reject it anyway.
 
 OK, so, should we change the error message:
 
   cannot perform FREEZE because of transaction activity after table
   creation or truncation
 
 to
 
   cannot perform FREEZE because the table was not created or
   truncated in the current subtransaction
 
 or do we need to keep the transaction activity weasel wording because
 of the second case you listed above?  I am suspecting the later.

Let's touch on the exception in passing by using the phrase last truncated,
giving this wording for both the second and the third COPY FREEZE error sites:

cannot perform FREEZE because the table was not created or last
truncated in the current subtransaction


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


Re: [HACKERS] COPY FREEZE has no warning

2013-02-02 Thread Bruce Momjian
On Sat, Feb  2, 2013 at 09:51:13AM -0500, Noah Misch wrote:
  OK, so, should we change the error message:
  
  cannot perform FREEZE because of transaction activity after table
  creation or truncation
  
  to
  
  cannot perform FREEZE because the table was not created or
  truncated in the current subtransaction
  
  or do we need to keep the transaction activity weasel wording because
  of the second case you listed above?  I am suspecting the later.
 
 Let's touch on the exception in passing by using the phrase last truncated,
 giving this wording for both the second and the third COPY FREEZE error sites:
 
   cannot perform FREEZE because the table was not created or last
   truncated in the current subtransaction

Well, so you are saying that there really isn't any use-visible logic
for those messages to be different, i.e. that the transaction id can be
set to invalid even if we created/truncated in the same transaction, but
not the same subtransaction?  

The comparisons that trigger the two messages are:

if (cstate-rel-rd_createSubid != InvalidSubTransactionId ||
cstate-rel-rd_newRelfilenodeSubid != InvalidSubTransactionId)

and
if (cstate-rel-rd_createSubid != GetCurrentSubTransactionId() ||
cstate-rel-rd_newRelfilenodeSubid != GetCurrentSubTransactionId())

The first comparison is creation, the second, truncation.

Please confirm and I will make the change, or you can.

-- 
  Bruce Momjian  br...@momjian.ushttp://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


Re: [HACKERS] COPY FREEZE has no warning

2013-02-02 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 Well, so you are saying that there really isn't any use-visible logic
 for those messages to be different,

No, and in fact the whole block of code is badly written because it
conflates two unrelated tests.  I guess somebody was trying to save
a couple of nanoseconds by not calling GetCurrentSubTransactionId
if a previous test had failed, but really why should we care about
that number of cycles in COPY preliminaries?  The code ought to be
more like this:

/* comment about skipping FSM or WAL here */
if (cstate-rel-rd_createSubid != InvalidSubTransactionId ||
cstate-rel-rd_newRelfilenodeSubid != InvalidSubTransactionId)
{
hi_options |= HEAP_INSERT_SKIP_FSM;
if (!XLogIsNeeded())
hi_options |= HEAP_INSERT_SKIP_WAL;
}
/* comment about when we can perform FREEZE here */
if (cstate-freeze)
{
if (!ThereAreNoPriorRegisteredSnapshots() || !ThereAreNoReadyPortals())
ereport(ERROR,
(ERRCODE_INVALID_TRANSACTION_STATE,
errmsg(cannot perform FREEZE because of prior transaction 
activity)));

if (cstate-rel-rd_createSubid != GetCurrentSubTransactionId() 
cstate-rel-rd_newRelfilenodeSubid != GetCurrentSubTransactionId())
ereport(ERROR,
(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE,
errmsg(cannot perform FREEZE because the table was not 
created or truncated in the current subtransaction)));
hi_options |= HEAP_INSERT_FROZEN;
}


regards, tom lane


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


Re: [HACKERS] COPY FREEZE has no warning

2013-02-02 Thread Bruce Momjian
On Sat, Feb  2, 2013 at 12:09:05PM -0500, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  Well, so you are saying that there really isn't any use-visible logic
  for those messages to be different,
 
 No, and in fact the whole block of code is badly written because it
 conflates two unrelated tests.  I guess somebody was trying to save
 a couple of nanoseconds by not calling GetCurrentSubTransactionId
 if a previous test had failed, but really why should we care about
 that number of cycles in COPY preliminaries?  The code ought to be
 more like this:
 
 /* comment about skipping FSM or WAL here */
 if (cstate-rel-rd_createSubid != InvalidSubTransactionId ||
 cstate-rel-rd_newRelfilenodeSubid != InvalidSubTransactionId)
 {
 hi_options |= HEAP_INSERT_SKIP_FSM;
 if (!XLogIsNeeded())
 hi_options |= HEAP_INSERT_SKIP_WAL;
 }
 /* comment about when we can perform FREEZE here */
 if (cstate-freeze)
 {
 if (!ThereAreNoPriorRegisteredSnapshots() || 
 !ThereAreNoReadyPortals())
 ereport(ERROR,
 (ERRCODE_INVALID_TRANSACTION_STATE,
 errmsg(cannot perform FREEZE because of prior 
 transaction activity)));
 
 if (cstate-rel-rd_createSubid != GetCurrentSubTransactionId() 
 cstate-rel-rd_newRelfilenodeSubid != 
 GetCurrentSubTransactionId())
 ereport(ERROR,
 (ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE,
 errmsg(cannot perform FREEZE because the table was 
 not created or truncated in the current subtransaction)));
 hi_options |= HEAP_INSERT_FROZEN;
 }

Yes, I found the blocking odd too --- the test for
InvalidSubTransactionId is used by hi_options, and for freeze checking. 
I assumed != InvalidSubTransactionId and !=
GetCurrentSubTransactionId() had different meanings for freeze
checking.

I compounded the problem because originally there was no FREEZE failure
so no action was taken if != InvalidSubTransactionId.

Applied patch attached.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
new file mode 100644
index 49cc8dd..523c1e0
*** a/src/backend/commands/copy.c
--- b/src/backend/commands/copy.c
*** CopyFrom(CopyState cstate)
*** 1996,2031 
  		hi_options |= HEAP_INSERT_SKIP_FSM;
  		if (!XLogIsNeeded())
  			hi_options |= HEAP_INSERT_SKIP_WAL;
  
! 		/*
! 		 * Optimize if new relfilenode was created in this subxact or
! 		 * one of its committed children and we won't see those rows later
! 		 * as part of an earlier scan or command. This ensures that if this
! 		 * subtransaction aborts then the frozen rows won't be visible
! 		 * after xact cleanup. Note that the stronger test of exactly
! 		 * which subtransaction created it is crucial for correctness
! 		 * of this optimisation.
! 		 */
! 		if (cstate-freeze)
! 		{
! 			if (!ThereAreNoPriorRegisteredSnapshots() || !ThereAreNoReadyPortals())
! ereport(ERROR,
! 		(ERRCODE_INVALID_TRANSACTION_STATE,
! 		errmsg(cannot perform FREEZE because of prior transaction activity)));
  
! 			if (cstate-rel-rd_createSubid == GetCurrentSubTransactionId() ||
! cstate-rel-rd_newRelfilenodeSubid == GetCurrentSubTransactionId())
! hi_options |= HEAP_INSERT_FROZEN;
! 			else
! ereport(ERROR,
! 		(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE,
! 		errmsg(cannot perform FREEZE because of transaction activity after table creation or truncation)));
! 		}
  	}
- 	else if (cstate-freeze)
- 		ereport(ERROR,
- (ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE,
-  errmsg(cannot perform FREEZE because the table was not created or truncated in the current transaction)));
  
  	/*
  	 * We need a ResultRelInfo so we can use the regular executor's
--- 1996,2027 
  		hi_options |= HEAP_INSERT_SKIP_FSM;
  		if (!XLogIsNeeded())
  			hi_options |= HEAP_INSERT_SKIP_WAL;
+ 	}
  
! 	/*
! 	 * Optimize if new relfilenode was created in this subxact or
! 	 * one of its committed children and we won't see those rows later
! 	 * as part of an earlier scan or command. This ensures that if this
! 	 * subtransaction aborts then the frozen rows won't be visible
! 	 * after xact cleanup. Note that the stronger test of exactly
! 	 * which subtransaction created it is crucial for correctness
! 	 * of this optimisation.
! 	 */
! 	if (cstate-freeze)
! 	{
! 		if (!ThereAreNoPriorRegisteredSnapshots() || !ThereAreNoReadyPortals())
! 			ereport(ERROR,
! 	(ERRCODE_INVALID_TRANSACTION_STATE,
! 	errmsg(cannot perform FREEZE because of prior transaction activity)));
  
! 		if (cstate-rel-rd_createSubid != GetCurrentSubTransactionId() 
! 			cstate-rel-rd_newRelfilenodeSubid != GetCurrentSubTransactionId())
! 			ereport(ERROR,
! 	

Re: [HACKERS] COPY FREEZE has no warning

2013-02-02 Thread Noah Misch
On Sat, Feb 02, 2013 at 10:12:54AM -0500, Bruce Momjian wrote:
 On Sat, Feb  2, 2013 at 09:51:13AM -0500, Noah Misch wrote:
  Let's touch on the exception in passing by using the phrase last 
  truncated,
  giving this wording for both the second and the third COPY FREEZE error 
  sites:
  
  cannot perform FREEZE because the table was not created or last
  truncated in the current subtransaction
 
 Well, so you are saying that there really isn't any use-visible logic
 for those messages to be different, i.e. that the transaction id can be
 set to invalid even if we created/truncated in the same transaction, but
 not the same subtransaction?  

Right.  The latest committed code makes sense to me.


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


Re: [HACKERS] COPY FREEZE has no warning

2013-02-01 Thread Bruce Momjian
On Tue, Jan 29, 2013 at 08:34:24PM -0500, Noah Misch wrote:
 On Fri, Jan 25, 2013 at 11:28:58PM -0500, Bruce Momjian wrote:
  On Fri, Jan 25, 2013 at 11:08:56PM -0500, Tom Lane wrote:
   Bruce Momjian br...@momjian.us writes:
!   ereport(ERROR,
!   
(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE,
!   errmsg(cannot perform 
FREEZE because of previous table activity in the current 
transaction)));
   
   [ itch... ]  What is table activity?  I always thought of tables as
   being rather passive objects.  And anyway, isn't this backwards?  What
   we're complaining of is *lack* of activity.  I don't see why this isn't
   using the same message as the other code path, namely
  
  Well, here is an example of this message:
  
  BEGIN;
  TRUNCATE vistest;
  SAVEPOINT s1;
  COPY vistest FROM stdin CSV FREEZE;
  ERROR:  cannot perform FREEZE because of previous table activity in the 
  current transaction
  COMMIT;
  
  Clearly it was truncated in the same transaction, but the savepoint
  somehow invalidates the freeze.  There is a C comment about it:
 
 The savepoint prevents the COPY FREEZE, because COPY FREEZE needs the table to
 have been created or truncated in the current *sub*transaction.  Issuing
 RELEASE s1 before the COPY makes it work again, for example.
 
  
   * BEGIN;
   * TRUNCATE t;
   * SAVEPOINT save;
   * TRUNCATE t;
   * ROLLBACK TO save;
   * COPY ...
 
 This is different.  The table was truncated in the current subtransaction, and
 it's safe in principle to apply the optimization.  Due to an implementation
 artifact, we'll reject it anyway.

OK, so, should we change the error message:

cannot perform FREEZE because of transaction activity after table
creation or truncation

to

cannot perform FREEZE because the table was not created or
truncated in the current subtransaction

or do we need to keep the transaction activity weasel wording because
of the second case you listed above?  I am suspecting the later.

-- 
  Bruce Momjian  br...@momjian.ushttp://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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-29 Thread Noah Misch
On Fri, Jan 25, 2013 at 11:28:58PM -0500, Bruce Momjian wrote:
 On Fri, Jan 25, 2013 at 11:08:56PM -0500, Tom Lane wrote:
  Bruce Momjian br...@momjian.us writes:
   ! ereport(ERROR,
   ! 
   (ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE,
   ! errmsg(cannot perform 
   FREEZE because of previous table activity in the current transaction)));
  
  [ itch... ]  What is table activity?  I always thought of tables as
  being rather passive objects.  And anyway, isn't this backwards?  What
  we're complaining of is *lack* of activity.  I don't see why this isn't
  using the same message as the other code path, namely
 
 Well, here is an example of this message:
 
   BEGIN;
   TRUNCATE vistest;
   SAVEPOINT s1;
   COPY vistest FROM stdin CSV FREEZE;
   ERROR:  cannot perform FREEZE because of previous table activity in the 
 current transaction
   COMMIT;
 
 Clearly it was truncated in the same transaction, but the savepoint
 somehow invalidates the freeze.  There is a C comment about it:

The savepoint prevents the COPY FREEZE, because COPY FREEZE needs the table to
have been created or truncated in the current *sub*transaction.  Issuing
RELEASE s1 before the COPY makes it work again, for example.

 
  * BEGIN;
  * TRUNCATE t;
  * SAVEPOINT save;
  * TRUNCATE t;
  * ROLLBACK TO save;
  * COPY ...

This is different.  The table was truncated in the current subtransaction, and
it's safe in principle to apply the optimization.  Due to an implementation
artifact, we'll reject it anyway.


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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-26 Thread Bruce Momjian
On Fri, Jan 25, 2013 at 11:28:58PM -0500, Bruce Momjian wrote:
 On Fri, Jan 25, 2013 at 11:08:56PM -0500, Tom Lane wrote:
  Bruce Momjian br...@momjian.us writes:
   ! ereport(ERROR,
   ! 
   (ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE,
   ! errmsg(cannot perform 
   FREEZE because of previous table activity in the current transaction)));
  
  [ itch... ]  What is table activity?  I always thought of tables as
  being rather passive objects.  And anyway, isn't this backwards?  What
  we're complaining of is *lack* of activity.  I don't see why this isn't
  using the same message as the other code path, namely
 
 Well, here is an example of this message:
 
   BEGIN;
   TRUNCATE vistest;
   SAVEPOINT s1;
   COPY vistest FROM stdin CSV FREEZE;
   ERROR:  cannot perform FREEZE because of previous table activity in the 
 current transaction
   COMMIT;
 
 Clearly it was truncated in the same transaction, but the savepoint
 somehow invalidates the freeze.  There is a C comment about it:
 
  * BEGIN;
  * TRUNCATE t;
  * SAVEPOINT save;
  * TRUNCATE t;
  * ROLLBACK TO save;
  * COPY ...
 
 I changed it to:
 
 ERROR:  cannot perform FREEZE because of transaction activity after 
 table creation or truncation

Patch applied.

-- 
  Bruce Momjian  br...@momjian.ushttp://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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Bruce Momjian
On Fri, Jan 25, 2013 at 02:48:37AM +0100, Andres Freund wrote:
 On 2013-01-23 14:02:46 -0500, Bruce Momjian wrote:
  As a reminder, COPY FREEZE still does not issue any warning/notice if
  the freezing does not happen:
 
 FWIW, and I won't annoy anyone further after this email, now that its
 deterministic, I still think that this should be an ERROR not a WARNING.

As the FREEZE is just an optimization, I thought NOTICE, vs WARNING or
ERROR was fine.  If others want this changed, please reply.

-- 
  Bruce Momjian  br...@momjian.ushttp://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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Stephen Frost
* Bruce Momjian (br...@momjian.us) wrote:
 On Fri, Jan 25, 2013 at 02:48:37AM +0100, Andres Freund wrote:
  On 2013-01-23 14:02:46 -0500, Bruce Momjian wrote:
   As a reminder, COPY FREEZE still does not issue any warning/notice if
   the freezing does not happen:
  
  FWIW, and I won't annoy anyone further after this email, now that its
  deterministic, I still think that this should be an ERROR not a WARNING.
 
 As the FREEZE is just an optimization, I thought NOTICE, vs WARNING or
 ERROR was fine.  If others want this changed, please reply.

tbh, I tend to agree w/ Andres on this one.  COPY FREEZE means do
this, not if you can get away with it, then do it.  That said, I can
really see a use-case for both which would imply that we'd have a way to
specify, ala DROP TABLE and IF EXISTS.  Not sure exactly what that'd
look like though and having one or the other is better than nothing
(presuming everyone is fine with the visibility impacts of this, which I
still contend will cause our users to give us grief over in the
future..).

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Bruce Momjian
On Fri, Jan 25, 2013 at 10:30:40AM -0500, Stephen Frost wrote:
 * Bruce Momjian (br...@momjian.us) wrote:
  On Fri, Jan 25, 2013 at 02:48:37AM +0100, Andres Freund wrote:
   On 2013-01-23 14:02:46 -0500, Bruce Momjian wrote:
As a reminder, COPY FREEZE still does not issue any warning/notice if
the freezing does not happen:
   
   FWIW, and I won't annoy anyone further after this email, now that its
   deterministic, I still think that this should be an ERROR not a WARNING.
  
  As the FREEZE is just an optimization, I thought NOTICE, vs WARNING or
  ERROR was fine.  If others want this changed, please reply.
 
 tbh, I tend to agree w/ Andres on this one.  COPY FREEZE means do
 this, not if you can get away with it, then do it.  That said, I can
 really see a use-case for both which would imply that we'd have a way to
 specify, ala DROP TABLE and IF EXISTS.  Not sure exactly what that'd
 look like though and having one or the other is better than nothing
 (presuming everyone is fine with the visibility impacts of this, which I
 still contend will cause our users to give us grief over in the
 future..).

Interesting.  I can see the visibility as making this more than an
optimization, because it has external visibility.  However, the
visibility problem is when it is silent (no NOTICE).  Do we need
a message that says we did honor FREEZE?

We could get fancy and make FREEZE more than a boolean, e.g. OFF,
PREFER, FORCE.

-- 
  Bruce Momjian  br...@momjian.ushttp://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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 On Fri, Jan 25, 2013 at 02:48:37AM +0100, Andres Freund wrote:
 FWIW, and I won't annoy anyone further after this email, now that its
 deterministic, I still think that this should be an ERROR not a WARNING.

 As the FREEZE is just an optimization, I thought NOTICE, vs WARNING or
 ERROR was fine.  If others want this changed, please reply.

The previous argument about it was if you bothered to specify FREEZE,
you probably really want/need that behavior.  So I can definitely see
Andres' point.  Perhaps WARNING would be a suitable compromise?

regards, tom lane


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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Robert Haas
On Fri, Jan 25, 2013 at 11:59 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Bruce Momjian br...@momjian.us writes:
 On Fri, Jan 25, 2013 at 02:48:37AM +0100, Andres Freund wrote:
 FWIW, and I won't annoy anyone further after this email, now that its
 deterministic, I still think that this should be an ERROR not a WARNING.

 As the FREEZE is just an optimization, I thought NOTICE, vs WARNING or
 ERROR was fine.  If others want this changed, please reply.

 The previous argument about it was if you bothered to specify FREEZE,
 you probably really want/need that behavior.  So I can definitely see
 Andres' point.  Perhaps WARNING would be a suitable compromise?

I'll vote for ERROR.  I don't see why this sound be a best-effort thing.

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


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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 On Fri, Jan 25, 2013 at 11:59 AM, Tom Lane t...@sss.pgh.pa.us wrote:
  Bruce Momjian br...@momjian.us writes:
  On Fri, Jan 25, 2013 at 02:48:37AM +0100, Andres Freund wrote:
  FWIW, and I won't annoy anyone further after this email, now that its
  deterministic, I still think that this should be an ERROR not a WARNING.
 
  As the FREEZE is just an optimization, I thought NOTICE, vs WARNING or
  ERROR was fine.  If others want this changed, please reply.
 
  The previous argument about it was if you bothered to specify FREEZE,
  you probably really want/need that behavior.  So I can definitely see
  Andres' point.  Perhaps WARNING would be a suitable compromise?
 
 I'll vote for ERROR.  I don't see why this sound be a best-effort thing.

Yeah, I tend to agree.  In part, I think having it error when the
conditions aren't met would actually reduce the chances of having this
'feature' end up as the default in some ORM somewhere...

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Jeff Janes
On Fri, Jan 25, 2013 at 9:42 AM, Robert Haas robertmh...@gmail.com wrote:
 On Fri, Jan 25, 2013 at 11:59 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Bruce Momjian br...@momjian.us writes:
 On Fri, Jan 25, 2013 at 02:48:37AM +0100, Andres Freund wrote:
 FWIW, and I won't annoy anyone further after this email, now that its
 deterministic, I still think that this should be an ERROR not a WARNING.

 As the FREEZE is just an optimization, I thought NOTICE, vs WARNING or
 ERROR was fine.  If others want this changed, please reply.

 The previous argument about it was if you bothered to specify FREEZE,
 you probably really want/need that behavior.  So I can definitely see
 Andres' point.  Perhaps WARNING would be a suitable compromise?

 I'll vote for ERROR.  I don't see why this sound be a best-effort thing.


+1.  If I had no objection to my database getting stuffed to the gills
with unfrozen tuples, I wouldn't have invoked the feature in the first
place.

As far as can tell, this ERROR/WARNING must occur immediately, because
once the first tuple is inserted frozen it is too late to change ones
mind.  So the problem can be immediately fixed and retried.

Except, is there perhaps some way for the user to decide to promote
WARNINGs to ERRORs on for a given command/transaction?

Cheers,

Jeff


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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Bruce Momjian
On Fri, Jan 25, 2013 at 11:55:12AM -0800, Jeff Janes wrote:
 On Fri, Jan 25, 2013 at 9:42 AM, Robert Haas robertmh...@gmail.com wrote:
  On Fri, Jan 25, 2013 at 11:59 AM, Tom Lane t...@sss.pgh.pa.us wrote:
  Bruce Momjian br...@momjian.us writes:
  On Fri, Jan 25, 2013 at 02:48:37AM +0100, Andres Freund wrote:
  FWIW, and I won't annoy anyone further after this email, now that its
  deterministic, I still think that this should be an ERROR not a WARNING.
 
  As the FREEZE is just an optimization, I thought NOTICE, vs WARNING or
  ERROR was fine.  If others want this changed, please reply.
 
  The previous argument about it was if you bothered to specify FREEZE,
  you probably really want/need that behavior.  So I can definitely see
  Andres' point.  Perhaps WARNING would be a suitable compromise?
 
  I'll vote for ERROR.  I don't see why this sound be a best-effort thing.
 
 
 +1.  If I had no objection to my database getting stuffed to the gills
 with unfrozen tuples, I wouldn't have invoked the feature in the first
 place.
 
 As far as can tell, this ERROR/WARNING must occur immediately, because
 once the first tuple is inserted frozen it is too late to change ones
 mind.  So the problem can be immediately fixed and retried.
 
 Except, is there perhaps some way for the user to decide to promote
 WARNINGs to ERRORs on for a given command/transaction?

OK, updated patch attached that throws an error with a more specific
message.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
new file mode 100644
index 8778e8b..ec7c311
*** a/src/backend/commands/copy.c
--- b/src/backend/commands/copy.c
*** CopyFrom(CopyState cstate)
*** 2009,2023 
  		 *
  		 * As noted above rd_newRelfilenodeSubid is not set in all cases
  		 * where we can apply the optimization, so in those rare cases
! 		 * where we cannot honour the request we do so silently.
  		 */
! 		if (cstate-freeze 
! 			ThereAreNoPriorRegisteredSnapshots() 
! 			ThereAreNoReadyPortals() 
! 			(cstate-rel-rd_newRelfilenodeSubid == GetCurrentSubTransactionId() ||
! 			 cstate-rel-rd_createSubid == GetCurrentSubTransactionId()))
! 			hi_options |= HEAP_INSERT_FROZEN;
  	}
  
  	/*
  	 * We need a ResultRelInfo so we can use the regular executor's
--- 2009,2029 
  		 *
  		 * As noted above rd_newRelfilenodeSubid is not set in all cases
  		 * where we can apply the optimization, so in those rare cases
! 		 * where we cannot honor the request.
  		 */
! 		if (cstate-freeze)
! 		{
! 			if (ThereAreNoPriorRegisteredSnapshots() 
! ThereAreNoReadyPortals() 
! (cstate-rel-rd_newRelfilenodeSubid == GetCurrentSubTransactionId() ||
!  cstate-rel-rd_createSubid == GetCurrentSubTransactionId()))
! hi_options |= HEAP_INSERT_FROZEN;
! 			else
! ereport(ERROR, (errmsg(cannot perform FREEZE operation due to invalid table or transaction state)));
! 		}
  	}
+ 	else if (cstate-freeze)
+ 		ereport(ERROR, (errmsg(cannot perform FREEZE operation due to invalid table or transaction state)));
  
  	/*
  	 * We need a ResultRelInfo so we can use the regular executor's
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
new file mode 100644
index 78c601f..e19cc5b
*** a/src/test/regress/expected/copy2.out
--- b/src/test/regress/expected/copy2.out
*** SELECT * FROM vistest;
*** 334,355 
  COMMIT;
  TRUNCATE vistest;
  COPY vistest FROM stdin CSV FREEZE;
  BEGIN;
  INSERT INTO vistest VALUES ('z');
  SAVEPOINT s1;
  TRUNCATE vistest;
  ROLLBACK TO SAVEPOINT s1;
  COPY vistest FROM stdin CSV FREEZE;
! SELECT * FROM vistest;
!  a  
! 
!  p
!  g
!  z
!  d3
!  e
! (5 rows)
! 
  COMMIT;
  CREATE FUNCTION truncate_in_subxact() RETURNS VOID AS
  $$
--- 334,347 
  COMMIT;
  TRUNCATE vistest;
  COPY vistest FROM stdin CSV FREEZE;
+ ERROR:  cannot perform FREEZE operation due to invalid table or transaction state
  BEGIN;
  INSERT INTO vistest VALUES ('z');
  SAVEPOINT s1;
  TRUNCATE vistest;
  ROLLBACK TO SAVEPOINT s1;
  COPY vistest FROM stdin CSV FREEZE;
! ERROR:  cannot perform FREEZE operation due to invalid table or transaction state
  COMMIT;
  CREATE FUNCTION truncate_in_subxact() RETURNS VOID AS
  $$
diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql
new file mode 100644
index 55568e6..87b847c
*** a/src/test/regress/sql/copy2.sql
--- b/src/test/regress/sql/copy2.sql
*** COPY vistest FROM stdin CSV FREEZE;
*** 242,248 
  d3
  e
  \.
- SELECT * FROM vistest;
  COMMIT;
  CREATE FUNCTION truncate_in_subxact() RETURNS VOID AS
  $$
--- 242,247 

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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 OK, updated patch attached that throws an error with a more specific
 message.


*
* As noted above rd_newRelfilenodeSubid is not set in all cases
* where we can apply the optimization, so in those rare cases
 !  * where we cannot honor the request.
*/

This sentence not complete.  I kind of think the entire para visible
above could be removed, anyway.

 ! ereport(ERROR, (errmsg(cannot perform FREEZE 
 operation due to invalid table or transaction state)));

I don't find this terribly specific.  It would at least be useful to
have two messages distinguishing whether the cause was invalid table
state (rd_createSubid and rd_newRelfilenodeSubid not set) or invalid
transaction state (the snapshot and portal tests).  The former might
usefully be phrased as because the table was not created or truncated
in the current transaction and the latter as because other actions are
in progress within the current transaction.

I'd also suggest cannot perform COPY FREEZE because whatever rather
than using the unnecessarily vague operation.

Also, this is missing an errcode, which means it will report itself as
an internal error, which it ain't.  It's also randomly unlike the
standard layout for ereport calls.
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE would do for the table case,
not sure about the other.

regards, tom lane


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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Michael Paquier
On Sat, Jan 26, 2013 at 2:42 AM, Robert Haas robertmh...@gmail.com wrote:

 On Fri, Jan 25, 2013 at 11:59 AM, Tom Lane t...@sss.pgh.pa.us wrote:
  Bruce Momjian br...@momjian.us writes:
  On Fri, Jan 25, 2013 at 02:48:37AM +0100, Andres Freund wrote:
  FWIW, and I won't annoy anyone further after this email, now that its
  deterministic, I still think that this should be an ERROR not a
 WARNING.
 
  As the FREEZE is just an optimization, I thought NOTICE, vs WARNING or
  ERROR was fine.  If others want this changed, please reply.
 
  The previous argument about it was if you bothered to specify FREEZE,
  you probably really want/need that behavior.  So I can definitely see
  Andres' point.  Perhaps WARNING would be a suitable compromise?

 I'll vote for ERROR.  I don't see why this sound be a best-effort thing.

+ 1. I was surprised to see COPY FREEZE failing silently when testing the
feature. An ERROR would be suited.
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Bruce Momjian
On Fri, Jan 25, 2013 at 05:30:58PM -0500, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  OK, updated patch attached that throws an error with a more specific
  message.
 
 
   *
   * As noted above rd_newRelfilenodeSubid is not set in all cases
   * where we can apply the optimization, so in those rare cases
  !* where we cannot honor the request.
   */
 
 This sentence not complete.  I kind of think the entire para visible
 above could be removed, anyway.
 
  !   ereport(ERROR, (errmsg(cannot perform FREEZE 
  operation due to invalid table or transaction state)));
 
 I don't find this terribly specific.  It would at least be useful to
 have two messages distinguishing whether the cause was invalid table
 state (rd_createSubid and rd_newRelfilenodeSubid not set) or invalid
 transaction state (the snapshot and portal tests).  The former might
 usefully be phrased as because the table was not created or truncated
 in the current transaction and the latter as because other actions are
 in progress within the current transaction.
 
 I'd also suggest cannot perform COPY FREEZE because whatever rather
 than using the unnecessarily vague operation.
 
 Also, this is missing an errcode, which means it will report itself as
 an internal error, which it ain't.  It's also randomly unlike the
 standard layout for ereport calls.
 ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE would do for the table case,
 not sure about the other.

OK, that was tricky, but completed with the attached patch. 
Surprisingly, truncation wasn't mention in our docs, though it was used
in the regression tests.  I have fixed that.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
new file mode 100644
index 6a0fabc..2137c67
*** a/doc/src/sgml/ref/copy.sgml
--- b/doc/src/sgml/ref/copy.sgml
*** COPY { replaceable class=parameterta
*** 190,207 
would be after running the commandVACUUM FREEZE/ command.
This is intended as a performance option for initial data loading.
Rows will be frozen only if the table being loaded has been created
!   in the current subtransaction, there are no cursors open and there
!   are no older snapshots held by this transaction. If those conditions
!   are not met the command will continue without error though will not
!   freeze rows. It is also possible in rare cases that the request
!   cannot be honoured for internal reasons, hence literalFREEZE/literal
!   is more of a guideline than a hard rule.
   /para
   para
Note that all other sessions will immediately be able to see the data
once it has been successfully loaded. This violates the normal rules
!   of MVCC visibility and by specifying this option the user acknowledges
!   explicitly that this is understood.
   /para
  /listitem
 /varlistentry
--- 190,203 
would be after running the commandVACUUM FREEZE/ command.
This is intended as a performance option for initial data loading.
Rows will be frozen only if the table being loaded has been created
!   or truncated in the current subtransaction, there are no cursors
!   open and there are no older snapshots held by this transaction.
   /para
   para
Note that all other sessions will immediately be able to see the data
once it has been successfully loaded. This violates the normal rules
!   of MVCC visibility and users specifying should be aware of the
!   potential problems this might cause.
   /para
  /listitem
 /varlistentry
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
new file mode 100644
index 8778e8b..be249f3
*** a/src/backend/commands/copy.c
--- b/src/backend/commands/copy.c
*** CopyFrom(CopyState cstate)
*** 1978,1985 
  	 * ROLLBACK TO save;
  	 * COPY ...
  	 *
- 	 * However this is OK since at worst we will fail to make the optimization.
- 	 *
  	 * Also, if the target file is new-in-transaction, we assume that checking
  	 * FSM for free space is a waste of time, even if we must use WAL because
  	 * of archiving.  This could possibly be wrong, but it's unlikely.
--- 1978,1983 
*** CopyFrom(CopyState cstate)
*** 1991,1996 
--- 1989,1995 
  	 * no additional work to enforce that.
  	 *--
  	 */
+ 	/* createSubid is creation check, newRelfilenodeSubid is truncation check */
  	if (cstate-rel-rd_createSubid != InvalidSubTransactionId ||
  		cstate-rel-rd_newRelfilenodeSubid != InvalidSubTransactionId)
  	{
*** CopyFrom(CopyState cstate)
*** 2006,2023 
  		 * after xact cleanup. Note that the stronger test of exactly
  		 * which subtransaction 

Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 ! ereport(ERROR,
 ! 
 (ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE,
 ! errmsg(cannot perform FREEZE 
 because of previous table activity in the current transaction)));

[ itch... ]  What is table activity?  I always thought of tables as
being rather passive objects.  And anyway, isn't this backwards?  What
we're complaining of is *lack* of activity.  I don't see why this isn't
using the same message as the other code path, namely

 + ereport(ERROR,
 + (ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE,
 +  errmsg(cannot perform FREEZE because the 
 table was not created or truncated in the current transaction)));

regards, tom lane


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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-25 Thread Bruce Momjian
On Fri, Jan 25, 2013 at 11:08:56PM -0500, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  !   ereport(ERROR,
  !   
  (ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE,
  !   errmsg(cannot perform FREEZE 
  because of previous table activity in the current transaction)));
 
 [ itch... ]  What is table activity?  I always thought of tables as
 being rather passive objects.  And anyway, isn't this backwards?  What
 we're complaining of is *lack* of activity.  I don't see why this isn't
 using the same message as the other code path, namely

Well, here is an example of this message:

BEGIN;
TRUNCATE vistest;
SAVEPOINT s1;
COPY vistest FROM stdin CSV FREEZE;
ERROR:  cannot perform FREEZE because of previous table activity in the 
current transaction
COMMIT;

Clearly it was truncated in the same transaction, but the savepoint
somehow invalidates the freeze.  There is a C comment about it:

 * BEGIN;
 * TRUNCATE t;
 * SAVEPOINT save;
 * TRUNCATE t;
 * ROLLBACK TO save;
 * COPY ...

I changed it to:

ERROR:  cannot perform FREEZE because of transaction activity after 
table creation or truncation

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
new file mode 100644
index 6a0fabc..2137c67
*** a/doc/src/sgml/ref/copy.sgml
--- b/doc/src/sgml/ref/copy.sgml
*** COPY { replaceable class=parameterta
*** 190,207 
would be after running the commandVACUUM FREEZE/ command.
This is intended as a performance option for initial data loading.
Rows will be frozen only if the table being loaded has been created
!   in the current subtransaction, there are no cursors open and there
!   are no older snapshots held by this transaction. If those conditions
!   are not met the command will continue without error though will not
!   freeze rows. It is also possible in rare cases that the request
!   cannot be honoured for internal reasons, hence literalFREEZE/literal
!   is more of a guideline than a hard rule.
   /para
   para
Note that all other sessions will immediately be able to see the data
once it has been successfully loaded. This violates the normal rules
!   of MVCC visibility and by specifying this option the user acknowledges
!   explicitly that this is understood.
   /para
  /listitem
 /varlistentry
--- 190,203 
would be after running the commandVACUUM FREEZE/ command.
This is intended as a performance option for initial data loading.
Rows will be frozen only if the table being loaded has been created
!   or truncated in the current subtransaction, there are no cursors
!   open and there are no older snapshots held by this transaction.
   /para
   para
Note that all other sessions will immediately be able to see the data
once it has been successfully loaded. This violates the normal rules
!   of MVCC visibility and users specifying should be aware of the
!   potential problems this might cause.
   /para
  /listitem
 /varlistentry
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
new file mode 100644
index 8778e8b..49cc8dd
*** a/src/backend/commands/copy.c
--- b/src/backend/commands/copy.c
*** CopyFrom(CopyState cstate)
*** 1978,1985 
  	 * ROLLBACK TO save;
  	 * COPY ...
  	 *
- 	 * However this is OK since at worst we will fail to make the optimization.
- 	 *
  	 * Also, if the target file is new-in-transaction, we assume that checking
  	 * FSM for free space is a waste of time, even if we must use WAL because
  	 * of archiving.  This could possibly be wrong, but it's unlikely.
--- 1978,1983 
*** CopyFrom(CopyState cstate)
*** 1991,1996 
--- 1989,1995 
  	 * no additional work to enforce that.
  	 *--
  	 */
+ 	/* createSubid is creation check, newRelfilenodeSubid is truncation check */
  	if (cstate-rel-rd_createSubid != InvalidSubTransactionId ||
  		cstate-rel-rd_newRelfilenodeSubid != InvalidSubTransactionId)
  	{
*** CopyFrom(CopyState cstate)
*** 2006,2023 
  		 * after xact cleanup. Note that the stronger test of exactly
  		 * which subtransaction created it is crucial for correctness
  		 * of this optimisation.
- 		 *
- 		 * As noted above rd_newRelfilenodeSubid is not set in all cases
- 		 * where we can apply the optimization, so in those rare cases
- 		 * where we cannot honour the request we do so silently.
  		 */
! 		if (cstate-freeze 
! 			ThereAreNoPriorRegisteredSnapshots() 
! 			ThereAreNoReadyPortals() 
! 			(cstate-rel-rd_newRelfilenodeSubid == 

Re: [HACKERS] COPY FREEZE has no warning

2013-01-24 Thread Bruce Momjian
On Wed, Jan 23, 2013 at 02:02:46PM -0500, Bruce Momjian wrote:
 As a reminder, COPY FREEZE still does not issue any warning/notice if
 the freezing does not happen:
 
   Requests copying the data with rows already frozen, just as they
   would be after running the commandVACUUM FREEZE/ command.
   This is intended as a performance option for initial data loading.
   Rows will be frozen only if the table being loaded has been created
   in the current subtransaction, there are no cursors open and there
   are no older snapshots held by this transaction. If those conditions
   are not met the command will continue without error though will not
   freeze rows. It is also possible in rare cases that the request
   cannot be honoured for internal reasons, hence literalFREEZE/literal
   is more of a guideline than a hard rule.
 
   Note that all other sessions will immediately be able to see the data
   once it has been successfully loaded. This violates the normal rules
   of MVCC visibility and by specifying this option the user acknowledges
   explicitly that this is understood.
 
 Didn't we want to issue the user some kind of feedback?

As no one wanted to write this patch, I have developed the attached
version.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
new file mode 100644
index 8778e8b..c8c9821
*** a/src/backend/commands/copy.c
--- b/src/backend/commands/copy.c
*** CopyFrom(CopyState cstate)
*** 2011,2023 
  		 * where we can apply the optimization, so in those rare cases
  		 * where we cannot honour the request we do so silently.
  		 */
! 		if (cstate-freeze 
! 			ThereAreNoPriorRegisteredSnapshots() 
! 			ThereAreNoReadyPortals() 
! 			(cstate-rel-rd_newRelfilenodeSubid == GetCurrentSubTransactionId() ||
! 			 cstate-rel-rd_createSubid == GetCurrentSubTransactionId()))
! 			hi_options |= HEAP_INSERT_FROZEN;
  	}
  
  	/*
  	 * We need a ResultRelInfo so we can use the regular executor's
--- 2011,2029 
  		 * where we can apply the optimization, so in those rare cases
  		 * where we cannot honour the request we do so silently.
  		 */
! 		if (cstate-freeze)
! 		{
! 			if (ThereAreNoPriorRegisteredSnapshots() 
! ThereAreNoReadyPortals() 
! (cstate-rel-rd_newRelfilenodeSubid == GetCurrentSubTransactionId() ||
!  cstate-rel-rd_createSubid == GetCurrentSubTransactionId()))
! hi_options |= HEAP_INSERT_FROZEN;
! 			else
! ereport(NOTICE, (errmsg(unable to honor \freeze\ option)));
! 		}
  	}
+ 	else if (cstate-freeze)
+ 		ereport(NOTICE, (errmsg(unable to honor \freeze\ option)));
  
  	/*
  	 * We need a ResultRelInfo so we can use the regular executor's
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
new file mode 100644
index 78c601f..126b3c7
*** a/src/test/regress/expected/copy2.out
--- b/src/test/regress/expected/copy2.out
*** SELECT * FROM vistest;
*** 334,345 
--- 334,347 
  COMMIT;
  TRUNCATE vistest;
  COPY vistest FROM stdin CSV FREEZE;
+ NOTICE:  unable to honor freeze option
  BEGIN;
  INSERT INTO vistest VALUES ('z');
  SAVEPOINT s1;
  TRUNCATE vistest;
  ROLLBACK TO SAVEPOINT s1;
  COPY vistest FROM stdin CSV FREEZE;
+ NOTICE:  unable to honor freeze option
  SELECT * FROM vistest;
   a  
  

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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-24 Thread Peter Eisentraut
On 1/24/13 5:09 PM, Bruce Momjian wrote:
 On Wed, Jan 23, 2013 at 02:02:46PM -0500, Bruce Momjian wrote:
 As a reminder, COPY FREEZE still does not issue any warning/notice if
 the freezing does not happen:

 As no one wanted to write this patch, I have developed the attached
 version.

I think it would be useful to add why it was unable to honor the option.
 Otherwise this might just end up spamming the logs without any chance
for improvement.



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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-24 Thread Bruce Momjian
On Thu, Jan 24, 2013 at 05:30:22PM -0500, Peter Eisentraut wrote:
 On 1/24/13 5:09 PM, Bruce Momjian wrote:
  On Wed, Jan 23, 2013 at 02:02:46PM -0500, Bruce Momjian wrote:
  As a reminder, COPY FREEZE still does not issue any warning/notice if
  the freezing does not happen:
 
  As no one wanted to write this patch, I have developed the attached
  version.
 
 I think it would be useful to add why it was unable to honor the option.
  Otherwise this might just end up spamming the logs without any chance
 for improvement.

Well, I would need to repeat what is already in the COPY docs.  Do you
have any suggested text?

-- 
  Bruce Momjian  br...@momjian.ushttp://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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-24 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 Didn't we want to issue the user some kind of feedback?

 As no one wanted to write this patch, I have developed the attached
 version.

Please note the comment directly above where you patched.

The proposed message doesn't seem to me to be following the message
style guide, either.

regards, tom lane


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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-24 Thread Bruce Momjian
On Thu, Jan 24, 2013 at 06:55:17PM -0500, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  Didn't we want to issue the user some kind of feedback?
 
  As no one wanted to write this patch, I have developed the attached
  version.
 
 Please note the comment directly above where you patched.
 
 The proposed message doesn't seem to me to be following the message
 style guide, either.

OK, updated patch attached.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
new file mode 100644
index 8778e8b..1086324
*** a/src/backend/commands/copy.c
--- b/src/backend/commands/copy.c
*** CopyFrom(CopyState cstate)
*** 2009,2023 
  		 *
  		 * As noted above rd_newRelfilenodeSubid is not set in all cases
  		 * where we can apply the optimization, so in those rare cases
! 		 * where we cannot honour the request we do so silently.
  		 */
! 		if (cstate-freeze 
! 			ThereAreNoPriorRegisteredSnapshots() 
! 			ThereAreNoReadyPortals() 
! 			(cstate-rel-rd_newRelfilenodeSubid == GetCurrentSubTransactionId() ||
! 			 cstate-rel-rd_createSubid == GetCurrentSubTransactionId()))
! 			hi_options |= HEAP_INSERT_FROZEN;
  	}
  
  	/*
  	 * We need a ResultRelInfo so we can use the regular executor's
--- 2009,2029 
  		 *
  		 * As noted above rd_newRelfilenodeSubid is not set in all cases
  		 * where we can apply the optimization, so in those rare cases
! 		 * where we cannot honor the request.
  		 */
! 		if (cstate-freeze)
! 		{
! 			if (ThereAreNoPriorRegisteredSnapshots() 
! ThereAreNoReadyPortals() 
! (cstate-rel-rd_newRelfilenodeSubid == GetCurrentSubTransactionId() ||
!  cstate-rel-rd_createSubid == GetCurrentSubTransactionId()))
! hi_options |= HEAP_INSERT_FROZEN;
! 			else
! ereport(NOTICE, (errmsg(cannot honor FREEZE option)));
! 		}
  	}
+ 	else if (cstate-freeze)
+ 		ereport(NOTICE, (errmsg(cannot honor FREEZE option)));
  
  	/*
  	 * We need a ResultRelInfo so we can use the regular executor's
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
new file mode 100644
index 78c601f..9a13479
*** a/src/test/regress/expected/copy2.out
--- b/src/test/regress/expected/copy2.out
*** SELECT * FROM vistest;
*** 334,345 
--- 334,347 
  COMMIT;
  TRUNCATE vistest;
  COPY vistest FROM stdin CSV FREEZE;
+ NOTICE:  cannot honor FREEZE option
  BEGIN;
  INSERT INTO vistest VALUES ('z');
  SAVEPOINT s1;
  TRUNCATE vistest;
  ROLLBACK TO SAVEPOINT s1;
  COPY vistest FROM stdin CSV FREEZE;
+ NOTICE:  cannot honor FREEZE option
  SELECT * FROM vistest;
   a  
  

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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-24 Thread Andres Freund
On 2013-01-23 14:02:46 -0500, Bruce Momjian wrote:
 As a reminder, COPY FREEZE still does not issue any warning/notice if
 the freezing does not happen:

FWIW, and I won't annoy anyone further after this email, now that its
deterministic, I still think that this should be an ERROR not a WARNING.

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


Re: [HACKERS] COPY FREEZE has no warning

2013-01-23 Thread Robert Haas
On Wed, Jan 23, 2013 at 2:02 PM, Bruce Momjian br...@momjian.us wrote:
 As a reminder, COPY FREEZE still does not issue any warning/notice if
 the freezing does not happen:

   Requests copying the data with rows already frozen, just as they
   would be after running the commandVACUUM FREEZE/ command.
   This is intended as a performance option for initial data loading.
   Rows will be frozen only if the table being loaded has been created
   in the current subtransaction, there are no cursors open and there
   are no older snapshots held by this transaction. If those conditions
   are not met the command will continue without error though will not
   freeze rows. It is also possible in rare cases that the request
   cannot be honoured for internal reasons, hence literalFREEZE/literal
   is more of a guideline than a hard rule.

   Note that all other sessions will immediately be able to see the data
   once it has been successfully loaded. This violates the normal rules
   of MVCC visibility and by specifying this option the user acknowledges
   explicitly that this is understood.

 Didn't we want to issue the user some kind of feedback?

I believe that is what was agreed.

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