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
