currval and DISCARD ALL

Started by Nigel Heronover 12 years ago25 messages
#1Nigel Heron
nigel@psycode.com

Hi,
is there a way to clear the session state of sequence values fetched by
currval(regclass)? "DISCARD ALL" doesn't seem to do it.

eg. (w/ pg 9.2.4)
test=# CREATE SEQUENCE foo_seq;
CREATE SEQUENCE
test=# SELECT nextval('foo_seq');
-[ RECORD 1 ]
nextval | 1

test=# SELECT currval('foo_seq');
-[ RECORD 1 ]
currval | 1

test=# DISCARD ALL;
DISCARD ALL
test=# SELECT currval('foo_seq');
-[ RECORD 1 ]
currval | 1

I'm trying to migrate a large web app to work with pgbouncer's
transaction pool mode and it would be easier to identify issues if
currval() would return the usual "ERROR: currval of sequence "foo_seq"
is not yet defined in this session" if nextval() wasn't called in the
same pgbouncer session instead of getting old numbers from past
transactions.

thanks,
-nigel.

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

#2Adrian Klaver
adrian.klaver@gmail.com
In reply to: Nigel Heron (#1)
Re: currval and DISCARD ALL

On 04/15/2013 02:42 PM, Nigel Heron wrote:

Hi,
is there a way to clear the session state of sequence values fetched by
currval(regclass)? "DISCARD ALL" doesn't seem to do it.

eg. (w/ pg 9.2.4)
test=# CREATE SEQUENCE foo_seq;
CREATE SEQUENCE
test=# SELECT nextval('foo_seq');
-[ RECORD 1 ]
nextval | 1

test=# SELECT currval('foo_seq');
-[ RECORD 1 ]
currval | 1

test=# DISCARD ALL;
DISCARD ALL
test=# SELECT currval('foo_seq');
-[ RECORD 1 ]
currval | 1

I'm trying to migrate a large web app to work with pgbouncer's
transaction pool mode and it would be easier to identify issues if
currval() would return the usual "ERROR: currval of sequence "foo_seq"
is not yet defined in this session" if nextval() wasn't called in the
same pgbouncer session instead of getting old numbers from past
transactions.

Might want to take a look at:

http://www.depesz.com/2012/12/02/what-is-the-point-of-bouncing/

for some hints on dealing with sequences and pgBouncer.

thanks,
-nigel.

--
Adrian Klaver
adrian.klaver@gmail.com

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

#3Nigel Heron
nigel@psycode.com
In reply to: Adrian Klaver (#2)
Re: currval and DISCARD ALL

On 04/15/2013 05:57 PM, Adrian Klaver wrote:

On 04/15/2013 02:42 PM, Nigel Heron wrote:

Hi,
is there a way to clear the session state of sequence values fetched by
currval(regclass)? "DISCARD ALL" doesn't seem to do it.

<snip>

Might want to take a look at:

http://www.depesz.com/2012/12/02/what-is-the-point-of-bouncing/

for some hints on dealing with sequences and pgBouncer.

thanks, I read it (his blogs are always interesting!). I'm not disputing
that calling currval() at the wrong time is a bad idea.
I'm just wondering why DISCARD ALL clears everything but this?
from the docs:
"DISCARD ALL resets a session to its original state, discarding
temporary resources and resetting session-local configuration changes."
.. but at the beginning of a session currval(foo) would return an error,
whereas calling nexval(foo); DISCARD ALL; currval(foo); does not return
an error.. clearly something isn't getting reset to the original state.

If you create a TEMP sequence, then DISCARD ALL does clear the state,
probably because the underlying table disappears.

-nigel.

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

#4Adrian Klaver
adrian.klaver@gmail.com
In reply to: Nigel Heron (#3)
Re: currval and DISCARD ALL

On 04/16/2013 08:07 AM, Nigel Heron wrote:

On 04/15/2013 05:57 PM, Adrian Klaver wrote:

On 04/15/2013 02:42 PM, Nigel Heron wrote:

Hi,
is there a way to clear the session state of sequence values fetched by
currval(regclass)? "DISCARD ALL" doesn't seem to do it.

<snip>

Might want to take a look at:

http://www.depesz.com/2012/12/02/what-is-the-point-of-bouncing/

for some hints on dealing with sequences and pgBouncer.

thanks, I read it (his blogs are always interesting!). I'm not disputing
that calling currval() at the wrong time is a bad idea.
I'm just wondering why DISCARD ALL clears everything but this?

Well per the docs:

http://www.postgresql.org/docs/9.2/interactive/sql-discard.html

DISCARD ALL

is equivalent to:

SET SESSION AUTHORIZATION DEFAULT;
RESET ALL;
DEALLOCATE ALL;
CLOSE ALL;
UNLISTEN *;
SELECT pg_advisory_unlock_all();
DISCARD PLANS;
DISCARD TEMP;

AFAIK, none of the above affect sequences.

-nigel.

--
Adrian Klaver
adrian.klaver@gmail.com

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

#5Bruce Momjian
bruce@momjian.us
In reply to: Adrian Klaver (#4)
Re: [GENERAL] currval and DISCARD ALL

On Tue, Apr 16, 2013 at 12:13:48PM -0700, Adrian Klaver wrote:

On 04/16/2013 08:07 AM, Nigel Heron wrote:

On 04/15/2013 05:57 PM, Adrian Klaver wrote:

On 04/15/2013 02:42 PM, Nigel Heron wrote:

Hi,
is there a way to clear the session state of sequence values fetched by
currval(regclass)? "DISCARD ALL" doesn't seem to do it.

<snip>

Might want to take a look at:

http://www.depesz.com/2012/12/02/what-is-the-point-of-bouncing/

for some hints on dealing with sequences and pgBouncer.

thanks, I read it (his blogs are always interesting!). I'm not disputing
that calling currval() at the wrong time is a bad idea.
I'm just wondering why DISCARD ALL clears everything but this?

Well per the docs:

http://www.postgresql.org/docs/9.2/interactive/sql-discard.html

DISCARD ALL

is equivalent to:

SET SESSION AUTHORIZATION DEFAULT;
RESET ALL;
DEALLOCATE ALL;
CLOSE ALL;
UNLISTEN *;
SELECT pg_advisory_unlock_all();
DISCARD PLANS;
DISCARD TEMP;

AFAIK, none of the above affect sequences.

I think his point is why don't we clear currval() on DISCARD ALL? I
can't think of a good reason we don't. He is saying currval() should
throw an error after DISCARD ALL:

test=> SELECT currval('seq');
ERROR: currval of sequence "seq" is not yet defined in this session

I have moved this thead to hackers to get comments.

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

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#5)
Re: [GENERAL] currval and DISCARD ALL

Bruce Momjian <bruce@momjian.us> writes:

I think his point is why don't we clear currval() on DISCARD ALL? I
can't think of a good reason we don't.

Because we'd have to invent a new suboperation DISCARD SEQUENCES,
for one thing, in order to be consistent. I'd rather ask why it's
important that we should throw away such state. It doesn't seem to
me to be important enough to justify a new subcommand.

Or, if you'd rather a more direct answer: wanting this sounds like
evidence of bad application design. Why is your app dependent on
getting failures from currval, and isn't there a better way to do it?

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

#7Fabrízio de Royes Mello
fabriziomello@gmail.com
In reply to: Tom Lane (#6)
Re: [GENERAL] currval and DISCARD ALL

On Tue, Apr 16, 2013 at 6:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

[...]

Or, if you'd rather a more direct answer: wanting this sounds like
evidence of bad application design. Why is your app dependent on
getting failures from currval, and isn't there a better way to do it?

The sequence cache (seqtab) is used per each backend, so if we use a
connection pooler (like pgbouncer in session mode) between our app and
postgres we can get a wrong value from 'currval' because the backend isn't
completely clean.

This isn't it a good reason to implement this feature?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

Show quoted text

Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello

#8Marko Kreen
markokr@gmail.com
In reply to: Tom Lane (#6)
Re: [GENERAL] currval and DISCARD ALL

On Tue, Apr 16, 2013 at 05:09:19PM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

I think his point is why don't we clear currval() on DISCARD ALL? I
can't think of a good reason we don't.

Because we'd have to invent a new suboperation DISCARD SEQUENCES,
for one thing, in order to be consistent. I'd rather ask why it's
important that we should throw away such state. It doesn't seem to
me to be important enough to justify a new subcommand.

"consistency" is a philosophical thing. Practical reason for
subcommands is possibility to have partial reset for special
situations, pooling or otherwise. But such usage seems rather
rare in real life.

If the sequences are not worth subcommand, then let's not give them
subcommand and just wait until someone comes with actual reason
to have one.

But currval() is quite noticeable thing that DISCARD ALL should clear.

Or, if you'd rather a more direct answer: wanting this sounds like
evidence of bad application design. Why is your app dependent on
getting failures from currval, and isn't there a better way to do it?

It just does not sound like, but thats exactly the request - because
DISCARD ALL leaves user-visible state around, it's hard to fix
application that depends on broken assumptions.

In fact, it was surprise to me that currval() works across transactions.
My alternative proposal would be to get rid of such silly behaviour...

--
marko

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

#9Tom Lane
tgl@sss.pgh.pa.us
In reply to: Marko Kreen (#8)
Re: [GENERAL] currval and DISCARD ALL

Marko Kreen <markokr@gmail.com> writes:

On Tue, Apr 16, 2013 at 05:09:19PM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

I think his point is why don't we clear currval() on DISCARD ALL? I
can't think of a good reason we don't.

Because we'd have to invent a new suboperation DISCARD SEQUENCES,
for one thing, in order to be consistent. I'd rather ask why it's
important that we should throw away such state. It doesn't seem to
me to be important enough to justify a new subcommand.

"consistency" is a philosophical thing.

No, it's a critical tool in complexity management. When you're dealing
with systems as complicated as a database, every little non-orthogonal
detail adds up. DISCARD ALL has a clear definition in terms of simpler
commands, and it's going to stay that way. Either this is worth a
subcommand, or it's not worth worrying about at all.

But currval() is quite noticeable thing that DISCARD ALL should clear.

If it were as obvious and noticeable as all that, somebody would have
noticed before now. We've had DISCARD ALL with its current meaning
since 8.3, and nobody complained in the five-plus years since that
shipped.

At this point, even if a concrete case were made why DISCARD ALL should
clear currval (and I repeat that no credible case has been made; nobody
has for example pointed to a reasonably-well-designed application that
this breaks), there would be a pretty strong backwards-compatibility
argument not to change it.

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

#10Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#9)
Re: [GENERAL] currval and DISCARD ALL

On Wed, Apr 17, 2013 at 6:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

No, it's a critical tool in complexity management. When you're dealing
with systems as complicated as a database, every little non-orthogonal
detail adds up. DISCARD ALL has a clear definition in terms of simpler
commands, and it's going to stay that way. Either this is worth a
subcommand, or it's not worth worrying about at all.

We had this same argument back in November of 2008. Marko said:

/messages/by-id/24710.1227732351@sss.pgh.pa.us

And Greg Stark said:

/messages/by-id/87iqqapag2.fsf@oxford.xeocode.com

And you said:

/messages/by-id/24710.1227732351@sss.pgh.pa.us

And then you did this:

commit e309739670ac8c2fa0b236d116fcd44b0522025a
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu Nov 27 00:28:06 2008 +0000

Tweak wording of DISCARD ALL description to avoid giving the impression
that the presented list of equivalent operations is meant to be the
primary definition of what it does. Per comment from Guillaume Smet.

So it seems to me that we pretty much already made a decision that the
controlling definition of DISCARD ALL is that, as the fine manual says
"DISCARD ALL resets a session to its original state". Whatever
decision we make now ought to be consistent with that.

IOW, I don't care whether we introduce a new subcommand or not. But I
*do* think that that we ought to make our best effort to have DISCARD
ALL clear everything that smells like session-local state. Random
incompatibilities between what you see when running under a connection
pooler and what you see when connecting the DB directly are *bad*,
regardless of whether a well-designed application should be relying on
those particular things or not. The whole point of having a
transparent connection pooler is that it's supposed to be transparent
to the application.

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

#11Fabrízio de Royes Mello
fabriziomello@gmail.com
In reply to: Robert Haas (#10)
1 attachment(s)
Re: [GENERAL] currval and DISCARD ALL

On Fri, Apr 19, 2013 at 10:50 AM, Robert Haas <robertmhaas@gmail.com> wrote:

[...]

So it seems to me that we pretty much already made a decision that the
controlling definition of DISCARD ALL is that, as the fine manual says
"DISCARD ALL resets a session to its original state". Whatever
decision we make now ought to be consistent with that.

IOW, I don't care whether we introduce a new subcommand or not. But I
*do* think that that we ought to make our best effort to have DISCARD
ALL clear everything that smells like session-local state. Random
incompatibilities between what you see when running under a connection
pooler and what you see when connecting the DB directly are *bad*,
regardless of whether a well-designed application should be relying on
those particular things or not. The whole point of having a
transparent connection pooler is that it's supposed to be transparent
to the application.

+1

The attached wip patch do that and introduce a subcommand 'SEQUENCES', but
if we decide to don't add a new subcommand to DISCARD, then its easier to
modify the patch.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

Show quoted text

Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello

Attachments:

discard_sequences.patchapplication/octet-stream; name=discard_sequences.patchDownload
diff --git a/doc/src/sgml/ref/discard.sgml b/doc/src/sgml/ref/discard.sgml
index 65ebbae..abd3e28 100644
--- a/doc/src/sgml/ref/discard.sgml
+++ b/doc/src/sgml/ref/discard.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  <refsynopsisdiv>
 <synopsis>
-DISCARD { ALL | PLANS | TEMPORARY | TEMP }
+DISCARD { ALL | PLANS | SEQUENCES | TEMPORARY | TEMP }
 </synopsis>
  </refsynopsisdiv>
 
@@ -67,6 +67,15 @@ DISCARD { ALL | PLANS | TEMPORARY | TEMP }
    </varlistentry>
 
    <varlistentry>
+    <term><literal>SEQUENCES</literal></term>
+    <listitem>
+     <para>
+      Releases all internally cached sequences.
+     </para>
+    </listitem>
+   </varlistentry>
+
+   <varlistentry>
     <term><literal>ALL</literal></term>
     <listitem>
      <para>
@@ -83,6 +92,7 @@ UNLISTEN *;
 SELECT pg_advisory_unlock_all();
 DISCARD PLANS;
 DISCARD TEMP;
+DISCARD SEQUENCES;
 </programlisting></para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/commands/discard.c b/src/backend/commands/discard.c
index 76f3ab6..7cd727e 100644
--- a/src/backend/commands/discard.c
+++ b/src/backend/commands/discard.c
@@ -18,6 +18,7 @@
 #include "commands/async.h"
 #include "commands/discard.h"
 #include "commands/prepare.h"
+#include "commands/sequence.h"
 #include "utils/guc.h"
 #include "utils/portal.h"
 
@@ -39,6 +40,10 @@ DiscardCommand(DiscardStmt *stmt, bool isTopLevel)
 			ResetPlanCache();
 			break;
 
+		case DISCARD_SEQUENCES:
+			ReleaseSequenceCaches();
+			break;
+
 		case DISCARD_TEMP:
 			ResetTempTableNamespace();
 			break;
@@ -69,4 +74,5 @@ DiscardAll(bool isTopLevel)
 	LockReleaseAll(USER_LOCKMETHOD, true);
 	ResetPlanCache();
 	ResetTempTableNamespace();
+	ReleaseSequenceCaches();
 }
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index ff855d6..db2f317 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1601,3 +1601,10 @@ seq_redo(XLogRecPtr lsn, XLogRecord *record)
 
 	pfree(localpage);
 }
+
+
+void
+ReleaseSequenceCaches()
+{
+	free(seqtab);
+}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index ec69373..d056621 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -1672,7 +1672,7 @@ CheckPointStmt:
 
 /*****************************************************************************
  *
- * DISCARD { ALL | TEMP | PLANS }
+ * DISCARD { ALL | TEMP | PLANS | SEQUENCES }
  *
  *****************************************************************************/
 
@@ -1701,6 +1701,13 @@ DiscardStmt:
 					n->target = DISCARD_PLANS;
 					$$ = (Node *) n;
 				}
+			| DISCARD SEQUENCES
+				{
+					DiscardStmt *n = makeNode(DiscardStmt);
+					n->target = DISCARD_SEQUENCES;
+					$$ = (Node *) n;
+				}
+
 		;
 
 
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 7d2c812..2dc8a8c 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2374,7 +2374,7 @@ psql_completion(char *text, int start, int end)
 	else if (pg_strcasecmp(prev_wd, "DISCARD") == 0)
 	{
 		static const char *const list_DISCARD[] =
-		{"ALL", "PLANS", "TEMP", NULL};
+		{"ALL", "PLANS", "SEQUENCES", "TEMP", NULL};
 
 		COMPLETE_WITH_LIST(list_DISCARD);
 	}
diff --git a/src/include/commands/sequence.h b/src/include/commands/sequence.h
index d867754..3698332 100644
--- a/src/include/commands/sequence.h
+++ b/src/include/commands/sequence.h
@@ -77,5 +77,6 @@ extern void ResetSequence(Oid seq_relid);
 
 extern void seq_redo(XLogRecPtr lsn, XLogRecord *rptr);
 extern void seq_desc(StringInfo buf, uint8 xl_info, char *rec);
+extern void ReleaseSequenceCaches();
 
 #endif   /* SEQUENCE_H */
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 2229ef0..2c0fda4 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2494,6 +2494,7 @@ typedef enum DiscardMode
 {
 	DISCARD_ALL,
 	DISCARD_PLANS,
+	DISCARD_SEQUENCES,
 	DISCARD_TEMP
 } DiscardMode;
 
#12Robert Haas
robertmhaas@gmail.com
In reply to: Fabrízio de Royes Mello (#11)
Re: [GENERAL] currval and DISCARD ALL

On Fri, Apr 19, 2013 at 10:05 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

The attached wip patch do that and introduce a subcommand 'SEQUENCES', but
if we decide to don't add a new subcommand to DISCARD, then its easier to
modify the patch.

This patch is quite wrong. It frees seqtab without clearing the
pointer, so the next reference will stomp on memory that may have been
reallocated. And it doesn't even free seqtab correctly, since it only
frees the first node in the linked list.

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

#13Adrian Klaver
adrian.klaver@gmail.com
In reply to: Robert Haas (#10)
Re: [GENERAL] currval and DISCARD ALL

On 04/19/2013 06:50 AM, Robert Haas wrote:

On Wed, Apr 17, 2013 at 6:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

No, it's a critical tool in complexity management. When you're dealing
with systems as complicated as a database, every little non-orthogonal
detail adds up. DISCARD ALL has a clear definition in terms of simpler
commands, and it's going to stay that way. Either this is worth a
subcommand, or it's not worth worrying about at all.

And then you did this:

commit e309739670ac8c2fa0b236d116fcd44b0522025a
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu Nov 27 00:28:06 2008 +0000

Tweak wording of DISCARD ALL description to avoid giving the impression
that the presented list of equivalent operations is meant to be the
primary definition of what it does. Per comment from Guillaume Smet.

So it seems to me that we pretty much already made a decision that the
controlling definition of DISCARD ALL is that, as the fine manual says
"DISCARD ALL resets a session to its original state". Whatever
decision we make now ought to be consistent with that.

IOW, I don't care whether we introduce a new subcommand or not. But I
*do* think that that we ought to make our best effort to have DISCARD
ALL clear everything that smells like session-local state. Random
incompatibilities between what you see when running under a connection
pooler and what you see when connecting the DB directly are *bad*,
regardless of whether a well-designed application should be relying on
those particular things or not. The whole point of having a
transparent connection pooler is that it's supposed to be transparent
to the application.

I understand the confusion on what constitutes ALL in DISCARD, though I
am not sure about the incompatibility argument. The OP is using the
transaction mode from pgBouncer and from their docs:

http://wiki.postgresql.org/wiki/PgBouncer

"Transaction pooling
Server connection is assigned to client only during a transaction. When
PgBouncer notices that transaction is over, the server will be put back
into pool.
This mode breaks few session-based features of PostgreSQL. You can use
it only when application cooperates by not using features that break.
See the table below for incompatible features."

" Note that 'transaction' pooling breaks client expectations of server
by design and can be used only if application cooperates by not using
non-working features."

"
Session pooling
server_reset_query = DISCARD ALL;
This will clean everything.

Transaction pooling
server_reset_query =
Yes, empty. In transaction pooling mode the clients should not use any
session-based features, so there is no need to clean anything. The
server_reset_query would only add unnecessary round-trip between
transactions and would drop various caches that the next transaction
would unnecessarily need to fill again."

I could see the argument for a transparent pooler where it part of the
core code. Not sure if it is the projects responsibility to maintain
transparency with the feature matrices of external projects.

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

--
Adrian Klaver
adrian.klaver@gmail.com

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

#14Fabrízio de Royes Mello
fabriziomello@gmail.com
In reply to: Robert Haas (#12)
1 attachment(s)
Re: [GENERAL] currval and DISCARD ALL

On Fri, Apr 19, 2013 at 11:12 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Apr 19, 2013 at 10:05 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

The attached wip patch do that and introduce a subcommand 'SEQUENCES',

but

if we decide to don't add a new subcommand to DISCARD, then its easier to
modify the patch.

This patch is quite wrong. It frees seqtab without clearing the
pointer, so the next reference will stomp on memory that may have been
reallocated. And it doesn't even free seqtab correctly, since it only
frees the first node in the linked list.

Ohh sorry... you're all right... I completely forgot to finish the
ReleaseSequenceCaches to transverse 'seqtab' linked list and free each
node.

The attached patch have this correct code.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

Show quoted text

Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello

Attachments:

discard_sequences.patchapplication/octet-stream; name=discard_sequences.patchDownload
diff --git a/doc/src/sgml/ref/discard.sgml b/doc/src/sgml/ref/discard.sgml
index 65ebbae..abd3e28 100644
--- a/doc/src/sgml/ref/discard.sgml
+++ b/doc/src/sgml/ref/discard.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  <refsynopsisdiv>
 <synopsis>
-DISCARD { ALL | PLANS | TEMPORARY | TEMP }
+DISCARD { ALL | PLANS | SEQUENCES | TEMPORARY | TEMP }
 </synopsis>
  </refsynopsisdiv>
 
@@ -67,6 +67,15 @@ DISCARD { ALL | PLANS | TEMPORARY | TEMP }
    </varlistentry>
 
    <varlistentry>
+    <term><literal>SEQUENCES</literal></term>
+    <listitem>
+     <para>
+      Releases all internally cached sequences.
+     </para>
+    </listitem>
+   </varlistentry>
+
+   <varlistentry>
     <term><literal>ALL</literal></term>
     <listitem>
      <para>
@@ -83,6 +92,7 @@ UNLISTEN *;
 SELECT pg_advisory_unlock_all();
 DISCARD PLANS;
 DISCARD TEMP;
+DISCARD SEQUENCES;
 </programlisting></para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/commands/discard.c b/src/backend/commands/discard.c
index 76f3ab6..7cd727e 100644
--- a/src/backend/commands/discard.c
+++ b/src/backend/commands/discard.c
@@ -18,6 +18,7 @@
 #include "commands/async.h"
 #include "commands/discard.h"
 #include "commands/prepare.h"
+#include "commands/sequence.h"
 #include "utils/guc.h"
 #include "utils/portal.h"
 
@@ -39,6 +40,10 @@ DiscardCommand(DiscardStmt *stmt, bool isTopLevel)
 			ResetPlanCache();
 			break;
 
+		case DISCARD_SEQUENCES:
+			ReleaseSequenceCaches();
+			break;
+
 		case DISCARD_TEMP:
 			ResetTempTableNamespace();
 			break;
@@ -69,4 +74,5 @@ DiscardAll(bool isTopLevel)
 	LockReleaseAll(USER_LOCKMETHOD, true);
 	ResetPlanCache();
 	ResetTempTableNamespace();
+	ReleaseSequenceCaches();
 }
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index ff855d6..bce071a 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1601,3 +1601,20 @@ seq_redo(XLogRecPtr lsn, XLogRecord *record)
 
 	pfree(localpage);
 }
+
+
+void
+ReleaseSequenceCaches()
+{
+	SeqTableData *ptr = seqtab;
+	SeqTableData *tmp = NULL;
+
+	while (ptr != NULL)
+	{
+		tmp = ptr;
+		ptr = ptr->next;
+		free(tmp);
+	}
+
+	seqtab = NULL;
+}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index ec69373..d056621 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -1672,7 +1672,7 @@ CheckPointStmt:
 
 /*****************************************************************************
  *
- * DISCARD { ALL | TEMP | PLANS }
+ * DISCARD { ALL | TEMP | PLANS | SEQUENCES }
  *
  *****************************************************************************/
 
@@ -1701,6 +1701,13 @@ DiscardStmt:
 					n->target = DISCARD_PLANS;
 					$$ = (Node *) n;
 				}
+			| DISCARD SEQUENCES
+				{
+					DiscardStmt *n = makeNode(DiscardStmt);
+					n->target = DISCARD_SEQUENCES;
+					$$ = (Node *) n;
+				}
+
 		;
 
 
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 7d2c812..2dc8a8c 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2374,7 +2374,7 @@ psql_completion(char *text, int start, int end)
 	else if (pg_strcasecmp(prev_wd, "DISCARD") == 0)
 	{
 		static const char *const list_DISCARD[] =
-		{"ALL", "PLANS", "TEMP", NULL};
+		{"ALL", "PLANS", "SEQUENCES", "TEMP", NULL};
 
 		COMPLETE_WITH_LIST(list_DISCARD);
 	}
diff --git a/src/include/commands/sequence.h b/src/include/commands/sequence.h
index d867754..3698332 100644
--- a/src/include/commands/sequence.h
+++ b/src/include/commands/sequence.h
@@ -77,5 +77,6 @@ extern void ResetSequence(Oid seq_relid);
 
 extern void seq_redo(XLogRecPtr lsn, XLogRecord *rptr);
 extern void seq_desc(StringInfo buf, uint8 xl_info, char *rec);
+extern void ReleaseSequenceCaches();
 
 #endif   /* SEQUENCE_H */
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 2229ef0..2c0fda4 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2494,6 +2494,7 @@ typedef enum DiscardMode
 {
 	DISCARD_ALL,
 	DISCARD_PLANS,
+	DISCARD_SEQUENCES,
 	DISCARD_TEMP
 } DiscardMode;
 
#15Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Fabrízio de Royes Mello (#14)
Re: [GENERAL] currval and DISCARD ALL

Fabrízio de Royes Mello escribió:

Ohh sorry... you're all right... I completely forgot to finish the
ReleaseSequenceCaches to transverse 'seqtab' linked list and free each
node.

The attached patch have this correct code.

It seems a bad idea to backpatch this; whoever wants this functionality
in back branches should probably run a patched server.

--
Álvaro Herrera 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

#16Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#15)
Re: [GENERAL] currval and DISCARD ALL

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

It seems a bad idea to backpatch this; whoever wants this functionality
in back branches should probably run a patched server.

Surely this is 9.4 material at this point in any case.

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

#17Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#16)
Re: [GENERAL] currval and DISCARD ALL

On Fri, Apr 19, 2013 at 11:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

It seems a bad idea to backpatch this; whoever wants this functionality
in back branches should probably run a patched server.

Surely this is 9.4 material at this point in any case.

I don't know why this couldn't be slipped into 9.3; we have done worse
later. But I don't have a personal stake in it either, and will
certainly defer to whatever the consensus is.

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

#18suresh.balasubra
suresh@netsonicuae.com
In reply to: Tom Lane (#9)
Re: [GENERAL] currval and DISCARD ALL

Disclaimer: I am no hacker, just a PostGreSQL user, trying to provide a user
scenario where DISCARD SEQUENCES functionality is required.

We have designed a developed a small Application Development platform for
which the backend is PostGreSQL.

There is a DBLayer which is responsible in generating SQL statements for all
the INSERT, UPDATE, DELETE operations. Data can be pushed into multiple
tables using this layer. What we provide to this layer is just a DataSet
(.NET). DataTables in the DataSet will be named after their respective
tables. We also use DataColumn extended properties to push in additional
logic. All these happen with in a transaction and also in one shot, just one
hit to the DB by appending SQL statements in proper order.

There is an interesting feature that we have built into this DBlayer which
is auto linking. All tables in our system will have a serial field 'id'.
Suppose there is a master table (let us call it 'voucher') and a detail
table ('voucher_lines'), and we are using the layer to push one record to
the master table and 50 records to the detail table. 'voucher_lines' table
will have a integer column 'voucher_id'. '*_id' fields are automatically
populated with 'currval('*_id_seq'). All this works like a charm.

Now, imagine we want to push data into another table (say 'invoice') which
also has a field 'voucher_id'. This is a different activity not connected
with the above mentioned transaction. In this scenario this field will get
updated as currval('voucher_id_seq') returns a value. But we do not want
that to be updated. What we want is to resolve '*_id' fields into values
only within a transaction. After the transaction we want to get away with
the session. If we could have cleared the session someway (DISCARD ALL does
it, but not the currval sequence info) after the first transaction it would
have worked for us.

Hope I am clear enough.

Thanks for PostGres, it is a great DB, love working with it.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/Re-GENERAL-currval-and-DISCARD-ALL-tp5752340p5765110.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

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

#19Fabrízio de Royes Mello
fabrizio@timbira.com.br
In reply to: suresh.balasubra (#18)
Re: [GENERAL] currval and DISCARD ALL

On 25-07-2013 05:32, suresh.balasubra wrote:

Disclaimer: I am no hacker, just a PostGreSQL user, trying to provide a user
scenario where DISCARD SEQUENCES functionality is required.

We have designed a developed a small Application Development platform for
which the backend is PostGreSQL.

There is a DBLayer which is responsible in generating SQL statements for all
the INSERT, UPDATE, DELETE operations. Data can be pushed into multiple
tables using this layer. What we provide to this layer is just a DataSet
(.NET). DataTables in the DataSet will be named after their respective
tables. We also use DataColumn extended properties to push in additional
logic. All these happen with in a transaction and also in one shot, just one
hit to the DB by appending SQL statements in proper order.

There is an interesting feature that we have built into this DBlayer which
is auto linking. All tables in our system will have a serial field 'id'.
Suppose there is a master table (let us call it 'voucher') and a detail
table ('voucher_lines'), and we are using the layer to push one record to
the master table and 50 records to the detail table. 'voucher_lines' table
will have a integer column 'voucher_id'. '*_id' fields are automatically
populated with 'currval('*_id_seq'). All this works like a charm.

Now, imagine we want to push data into another table (say 'invoice') which
also has a field 'voucher_id'. This is a different activity not connected
with the above mentioned transaction. In this scenario this field will get
updated as currval('voucher_id_seq') returns a value. But we do not want
that to be updated. What we want is to resolve '*_id' fields into values
only within a transaction. After the transaction we want to get away with
the session. If we could have cleared the session someway (DISCARD ALL does
it, but not the currval sequence info) after the first transaction it would
have worked for us.

I already sent a patch to implement DISCARD SEQUENCES [1]https://commitfest.postgresql.org/action/patch_view?id=1171 that I expect
to be part of 9.4 version.

Regards,

[1]: https://commitfest.postgresql.org/action/patch_view?id=1171

--
Fabr�zio de Royes Mello Timbira - http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

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

#20Boszormenyi Zoltan
zb@cybertec.at
In reply to: Fabrízio de Royes Mello (#14)
Re: [GENERAL] currval and DISCARD ALL

Hi,

2013-04-19 16:58 keltez�ssel, Fabr�zio de Royes Mello �rta:

On Fri, Apr 19, 2013 at 11:12 AM, Robert Haas <robertmhaas@gmail.com
<mailto:robertmhaas@gmail.com>> wrote:

On Fri, Apr 19, 2013 at 10:05 AM, Fabr�zio de Royes Mello
<fabriziomello@gmail.com <mailto:fabriziomello@gmail.com>> wrote:

The attached wip patch do that and introduce a subcommand 'SEQUENCES', but
if we decide to don't add a new subcommand to DISCARD, then its easier to
modify the patch.

This patch is quite wrong. It frees seqtab without clearing the
pointer, so the next reference will stomp on memory that may have been
reallocated. And it doesn't even free seqtab correctly, since it only
frees the first node in the linked list.

Ohh sorry... you're all right... I completely forgot to finish the ReleaseSequenceCaches
to transverse 'seqtab' linked list and free each node.

The attached patch have this correct code.

Regards,

--
Fabr�zio de Royes Mello
Consultoria/Coaching PostgreSQL

Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello

I am reviewing your patch.

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

Almost. No rejects, no fuzz, only offset for some files.

* Does it include reasonable tests, necessary doc patches, etc?

Documentation, yes. Tests, no.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

The SQL standard doesn't have DISCARD.
Otherwise the behaviour is obvious.

* Does it include pg_dump support (if applicable)?

n/a

* Are there dangers?

It changes applications' assumptions slightly but takes the
behaviour closer to the wording of the documentation.

* Have all the bases been covered?

Yes.

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

No.

* Are there any assertion failures or crashes?

No.

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

n/a

* Does it slow down other things?

No.

* Does it follow the project coding guidelines?

Yes.

Maybe a little stylistic comment:

+void
+ReleaseSequenceCaches()
+{
+       SeqTableData *ptr = seqtab;
+       SeqTableData *tmp = NULL;
+
+       while (ptr != NULL)
+       {
+               tmp = ptr;
+               ptr = ptr->next;
+               free(tmp);
+       }
+
+       seqtab = NULL;
+}

I would rename the variables to "seq" and "next" from
"ptr" and "tmp", respectively, to make them even more
obvious. This looks a little better:

+void
+ReleaseSequenceCaches()
+{
+       SeqTableData *seq = seqtab;
+       SeqTableData *next;
+
+       while (seq)
+       {
+               next = seq->next;
+               free(seq);
+               seq = next;
+       }
+
+       seqtab = NULL;
+}

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

It should. There are no extra system calls.

* Are the comments sufficient and accurate?

The feature needs very little code which is downright obvious.
There are no comments but I don't think the code needs it.

* Does it do what it says, correctly?

Yes.

* Does it produce compiler warnings?

Only one:

src/backend/commands/sequence.c should have

#include <commands/sequence.h>

because of this:

sequence.c:1608:1: warning: no previous prototype for 'ReleaseSequenceCaches'
[-Wmissing-prototypes]
ReleaseSequenceCaches()
^

* Can you make it crash?

No.

* Is everything done in a way that fits together coherently with other features/modules?

Yes.

* Are there interdependencies that can cause problems?

I don't think so.

Best regards,
Zolt�n B�sz�rm�nyi

--
----------------------------------
Zolt�n B�sz�rm�nyi
Cybertec Sch�nig & Sch�nig GmbH
Gr�hrm�hlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

#21Boszormenyi Zoltan
zb@cybertec.at
In reply to: Boszormenyi Zoltan (#20)
Re: [GENERAL] currval and DISCARD ALL

2013-08-19 21:02 keltez�ssel, Boszormenyi Zoltan �rta:

Hi,

2013-04-19 16:58 keltez�ssel, Fabr�zio de Royes Mello �rta:

On Fri, Apr 19, 2013 at 11:12 AM, Robert Haas <robertmhaas@gmail.com
<mailto:robertmhaas@gmail.com>> wrote:

On Fri, Apr 19, 2013 at 10:05 AM, Fabr�zio de Royes Mello
<fabriziomello@gmail.com <mailto:fabriziomello@gmail.com>> wrote:

The attached wip patch do that and introduce a subcommand 'SEQUENCES', but
if we decide to don't add a new subcommand to DISCARD, then its easier to
modify the patch.

This patch is quite wrong. It frees seqtab without clearing the
pointer, so the next reference will stomp on memory that may have been
reallocated. And it doesn't even free seqtab correctly, since it only
frees the first node in the linked list.

Ohh sorry... you're all right... I completely forgot to finish the
ReleaseSequenceCaches to transverse 'seqtab' linked list and free each node.

The attached patch have this correct code.

Regards,

--
Fabr�zio de Royes Mello
Consultoria/Coaching PostgreSQL

Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello

I am reviewing your patch.

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

Almost. No rejects, no fuzz, only offset for some files.

* Does it include reasonable tests, necessary doc patches, etc?

Documentation, yes. Tests, no.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

The SQL standard doesn't have DISCARD.
Otherwise the behaviour is obvious.

* Does it include pg_dump support (if applicable)?

n/a

* Are there dangers?

It changes applications' assumptions slightly but takes the
behaviour closer to the wording of the documentation.

* Have all the bases been covered?

Yes.

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

No.

* Are there any assertion failures or crashes?

No.

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

n/a

* Does it slow down other things?

No.

* Does it follow the project coding guidelines?

Yes.

Maybe a little stylistic comment:

+void
+ReleaseSequenceCaches()
+{
+       SeqTableData *ptr = seqtab;
+       SeqTableData *tmp = NULL;
+
+       while (ptr != NULL)
+       {
+               tmp = ptr;
+               ptr = ptr->next;
+               free(tmp);
+       }
+
+       seqtab = NULL;
+}

I would rename the variables to "seq" and "next" from
"ptr" and "tmp", respectively, to make them even more
obvious. This looks a little better:

+void
+ReleaseSequenceCaches()
+{
+       SeqTableData *seq = seqtab;
+       SeqTableData *next;
+
+       while (seq)
+       {
+               next = seq->next;
+               free(seq);
+               seq = next;
+       }
+
+       seqtab = NULL;
+}

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

It should. There are no extra system calls.

* Are the comments sufficient and accurate?

The feature needs very little code which is downright obvious.
There are no comments but I don't think the code needs it.

* Does it do what it says, correctly?

Yes.

I lied.

There is one little problem. There is no command tag
reported for DISCARD SEQUENCES:

zozo=# create sequence s1;
CREATE SEQUENCE
zozo=# select nextval('s1');
nextval
---------
1
(1 row)

zozo=# select currval('s1');
currval
---------
1
(1 row)

zozo=# discard all;
DISCARD ALL
zozo=# discard sequences;
???
zozo=# select currval('s1');
ERROR: currval of sequence "s1" is not yet defined in this session

* Does it produce compiler warnings?

Only one:

src/backend/commands/sequence.c should have

#include <commands/sequence.h>

because of this:

sequence.c:1608:1: warning: no previous prototype for 'ReleaseSequenceCaches'
[-Wmissing-prototypes]
ReleaseSequenceCaches()
^

* Can you make it crash?

No.

* Is everything done in a way that fits together coherently with other features/modules?

Yes.

* Are there interdependencies that can cause problems?

I don't think so.

Best regards,
Zolt�n B�sz�rm�nyi

--
----------------------------------
Zolt�n B�sz�rm�nyi
Cybertec Sch�nig & Sch�nig GmbH
Gr�hrm�hlgasse 26
A-2700 Wiener Neustadt, Austria
Web:http://www.postgresql-support.de
http://www.postgresql.at/

--
----------------------------------
Zolt�n B�sz�rm�nyi
Cybertec Sch�nig & Sch�nig GmbH
Gr�hrm�hlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

#22Peter Eisentraut
peter_e@gmx.net
In reply to: Fabrízio de Royes Mello (#14)
Re: [GENERAL] currval and DISCARD ALL

On Fri, 2013-04-19 at 11:58 -0300, Fabrízio de Royes Mello wrote:

Ohh sorry... you're all right... I completely forgot to finish the
ReleaseSequenceCaches to transverse 'seqtab' linked list and free each
node.

The attached patch have this correct code.

Please fix this compiler warning:

sequence.c:1608:1: warning: no previous prototype for ‘ReleaseSequenceCaches’ [-Wmissing-prototypes]

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

#23Fabrízio de Royes Mello
fabrizio@timbira.com.br
In reply to: Boszormenyi Zoltan (#21)
1 attachment(s)
Re: [GENERAL] currval and DISCARD ALL

On 19-08-2013 16:10, Boszormenyi Zoltan wrote:

I am reviewing your patch.

Thanks...

* Is the patch in a patch format which has context? (eg: context diff
format)

Yes.

* Does it apply cleanly to the current git master?

Almost. No rejects, no fuzz, only offset for some files.

* Does it include reasonable tests, necessary doc patches, etc?

Documentation, yes. Tests, no.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

The SQL standard doesn't have DISCARD.
Otherwise the behaviour is obvious.

* Does it include pg_dump support (if applicable)?

n/a

* Are there dangers?

It changes applications' assumptions slightly but takes the
behaviour closer to the wording of the documentation.

* Have all the bases been covered?

Yes.

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

No.

* Are there any assertion failures or crashes?

No.

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

n/a

* Does it slow down other things?

No.

* Does it follow the project coding guidelines?

Yes.

Maybe a little stylistic comment:

+void
+ReleaseSequenceCaches()
+{
+       SeqTableData *ptr = seqtab;
+       SeqTableData *tmp = NULL;
+
+       while (ptr != NULL)
+       {
+               tmp = ptr;
+               ptr = ptr->next;
+               free(tmp);
+       }
+
+       seqtab = NULL;
+}

I would rename the variables to "seq" and "next" from
"ptr" and "tmp", respectively, to make them even more
obvious. This looks a little better:

+void
+ReleaseSequenceCaches()
+{
+       SeqTableData *seq = seqtab;
+       SeqTableData *next;
+
+       while (seq)
+       {
+               next = seq->next;
+               free(seq);
+               seq = next;
+       }
+
+       seqtab = NULL;
+}

Done!

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

It should. There are no extra system calls.

* Are the comments sufficient and accurate?

The feature needs very little code which is downright obvious.
There are no comments but I don't think the code needs it.

* Does it do what it says, correctly?

Yes.

I lied.

There is one little problem. There is no command tag
reported for DISCARD SEQUENCES:

zozo=# create sequence s1;
CREATE SEQUENCE
zozo=# select nextval('s1');
nextval
---------
1
(1 row)

zozo=# select currval('s1');
currval
---------
1
(1 row)

zozo=# discard all;
DISCARD ALL
zozo=# discard sequences;
???
zozo=# select currval('s1');
ERROR: currval of sequence "s1" is not yet defined in this session

Fixed!

* Does it produce compiler warnings?

Only one:

src/backend/commands/sequence.c should have

#include <commands/sequence.h>

because of this:

sequence.c:1608:1: warning: no previous prototype for
�ReleaseSequenceCaches� [-Wmissing-prototypes]
ReleaseSequenceCaches()
^

Fixed!

* Can you make it crash?

No.

* Is everything done in a way that fits together coherently with other
features/modules?

Yes.

* Are there interdependencies that can cause problems?

I don't think so.

The attached patch fix the items reviewed by you.

Regards,

--
Fabr�zio de Royes Mello Timbira - http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Attachments:

discard_sequences_v2.patchtext/x-patch; name=discard_sequences_v2.patchDownload
diff --git a/doc/src/sgml/ref/discard.sgml b/doc/src/sgml/ref/discard.sgml
index 65ebbae..abd3e28 100644
--- a/doc/src/sgml/ref/discard.sgml
+++ b/doc/src/sgml/ref/discard.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
 
  <refsynopsisdiv>
 <synopsis>
-DISCARD { ALL | PLANS | TEMPORARY | TEMP }
+DISCARD { ALL | PLANS | SEQUENCES | TEMPORARY | TEMP }
 </synopsis>
  </refsynopsisdiv>
 
@@ -67,6 +67,15 @@ DISCARD { ALL | PLANS | TEMPORARY | TEMP }
    </varlistentry>
 
    <varlistentry>
+    <term><literal>SEQUENCES</literal></term>
+    <listitem>
+     <para>
+      Releases all internally cached sequences.
+     </para>
+    </listitem>
+   </varlistentry>
+
+   <varlistentry>
     <term><literal>ALL</literal></term>
     <listitem>
      <para>
@@ -83,6 +92,7 @@ UNLISTEN *;
 SELECT pg_advisory_unlock_all();
 DISCARD PLANS;
 DISCARD TEMP;
+DISCARD SEQUENCES;
 </programlisting></para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/commands/discard.c b/src/backend/commands/discard.c
index 76f3ab6..f4e7e06 100644
--- a/src/backend/commands/discard.c
+++ b/src/backend/commands/discard.c
@@ -18,13 +18,14 @@
 #include "commands/async.h"
 #include "commands/discard.h"
 #include "commands/prepare.h"
+#include "commands/sequence.h"
 #include "utils/guc.h"
 #include "utils/portal.h"
 
 static void DiscardAll(bool isTopLevel);
 
 /*
- * DISCARD { ALL | TEMP | PLANS }
+ * DISCARD { ALL | SEQUENCES | TEMP | PLANS }
  */
 void
 DiscardCommand(DiscardStmt *stmt, bool isTopLevel)
@@ -39,6 +40,10 @@ DiscardCommand(DiscardStmt *stmt, bool isTopLevel)
 			ResetPlanCache();
 			break;
 
+		case DISCARD_SEQUENCES:
+			ReleaseSequenceCaches();
+			break;
+
 		case DISCARD_TEMP:
 			ResetTempTableNamespace();
 			break;
@@ -69,4 +74,5 @@ DiscardAll(bool isTopLevel)
 	LockReleaseAll(USER_LOCKMETHOD, true);
 	ResetPlanCache();
 	ResetTempTableNamespace();
+	ReleaseSequenceCaches();
 }
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index ddfaf3b..66ac9a5 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1602,3 +1602,22 @@ seq_redo(XLogRecPtr lsn, XLogRecord *record)
 
 	pfree(localpage);
 }
+
+/*
+ * Release the internal sequence caches
+ */
+void
+ReleaseSequenceCaches(void)
+{
+	SeqTableData *seq  = seqtab;
+	SeqTableData *next = NULL;
+
+	while (seq != NULL)
+	{
+		next = seq;
+		seq  = seq->next;
+		free(next);
+	}
+
+	seqtab = NULL;
+}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 22e82ba..202eb2e 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -1673,7 +1673,7 @@ CheckPointStmt:
 
 /*****************************************************************************
  *
- * DISCARD { ALL | TEMP | PLANS }
+ * DISCARD { ALL | TEMP | PLANS | SEQUENCES }
  *
  *****************************************************************************/
 
@@ -1702,6 +1702,13 @@ DiscardStmt:
 					n->target = DISCARD_PLANS;
 					$$ = (Node *) n;
 				}
+			| DISCARD SEQUENCES
+				{
+					DiscardStmt *n = makeNode(DiscardStmt);
+					n->target = DISCARD_SEQUENCES;
+					$$ = (Node *) n;
+				}
+
 		;
 
 
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index c940897..0a16a78 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -2189,6 +2189,9 @@ CreateCommandTag(Node *parsetree)
 				case DISCARD_TEMP:
 					tag = "DISCARD TEMP";
 					break;
+				case DISCARD_SEQUENCES:
+					tag = "DISCARD SEQUENCES";
+					break;
 				default:
 					tag = "???";
 			}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index b3de387..255061c 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2378,7 +2378,7 @@ psql_completion(char *text, int start, int end)
 	else if (pg_strcasecmp(prev_wd, "DISCARD") == 0)
 	{
 		static const char *const list_DISCARD[] =
-		{"ALL", "PLANS", "TEMP", NULL};
+		{"ALL", "PLANS", "SEQUENCES", "TEMP", NULL};
 
 		COMPLETE_WITH_LIST(list_DISCARD);
 	}
diff --git a/src/include/commands/sequence.h b/src/include/commands/sequence.h
index 6bd4892..913a23f 100644
--- a/src/include/commands/sequence.h
+++ b/src/include/commands/sequence.h
@@ -74,6 +74,7 @@ extern Datum pg_sequence_parameters(PG_FUNCTION_ARGS);
 extern Oid	DefineSequence(CreateSeqStmt *stmt);
 extern Oid	AlterSequence(AlterSeqStmt *stmt);
 extern void ResetSequence(Oid seq_relid);
+extern void ReleaseSequenceCaches(void);
 
 extern void seq_redo(XLogRecPtr lsn, XLogRecord *rptr);
 extern void seq_desc(StringInfo buf, uint8 xl_info, char *rec);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 51fef68..e5235cb 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2543,6 +2543,7 @@ typedef enum DiscardMode
 {
 	DISCARD_ALL,
 	DISCARD_PLANS,
+	DISCARD_SEQUENCES,
 	DISCARD_TEMP
 } DiscardMode;
 
#24Robert Haas
robertmhaas@gmail.com
In reply to: Fabrízio de Royes Mello (#23)
Re: [GENERAL] currval and DISCARD ALL

On Mon, Sep 2, 2013 at 4:35 PM, Fabrízio de Royes Mello
<fabrizio@timbira.com.br> wrote:

The attached patch fix the items reviewed by you.

Committed with assorted revisions. In particular, I renamed the
function that discards cached sequence data, revised the wording of
the documentation, added a regression test, and tweaked the list-free
code to pop items off one after the other instead of walking the list
and then NULLing it out at the end. Although no ERROR is possible
here currently, this coding style is generally preferable because it's
robust against being interrupted in the middle.

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

#25Fabrízio de Royes Mello
fabrizio@timbira.com.br
In reply to: Robert Haas (#24)
Re: [GENERAL] currval and DISCARD ALL

On Thu, Oct 3, 2013 at 5:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Committed with assorted revisions. In particular, I renamed the
function that discards cached sequence data, revised the wording of
the documentation, added a regression test, and tweaked the list-free
code to pop items off one after the other instead of walking the list
and then NULLing it out at the end. Although no ERROR is possible
here currently, this coding style is generally preferable because it's
robust against being interrupted in the middle.

Thanks!

--
Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento