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
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;
 
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to