Overdue code review for ALTER SEQUENCE patch. Don't generate illegal Node

tree for CYCLE option; don't assume zeros are invalid values for sequence
fields other than increment_by; don't reset cache_value when not told to;
simplify code for testing whether to apply defaults.

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 6bc59ca9fb176b0edbfd6565b5ee3d1a619f0ac5..c9076e85b462fa30731203cbcba41192b0c11116 100644 (file)
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/commands/sequence.c,v 1.103 2003/09/25 06:57:58 petere Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/commands/sequence.c,v 1.104 2003/11/24 16:54:07 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -70,7 +70,7 @@ static SeqTable seqtab = NULL;        /* Head of list of SeqTable items */
 static void init_sequence(RangeVar *relation,
                          SeqTable *p_elm, Relation *p_rel);
 static Form_pg_sequence read_info(SeqTable elm, Relation rel, Buffer *buf);
-static void init_params(List *options, Form_pg_sequence new);
+static void init_params(List *options, Form_pg_sequence new, bool isInit);
 static void do_setval(RangeVar *sequence, int64 next, bool iscalled);
 
 /*
@@ -94,16 +94,8 @@ DefineSequence(CreateSeqStmt *seq)
        int                     i;
        NameData        name;
 
-       /* Values are NULL (or false) by default */
-       new.last_value = 0;
-       new.increment_by = 0;
-       new.max_value = 0;
-       new.min_value = 0;
-       new.cache_value = 0;
-       new.is_cycled = false;
-
-       /* Check and set values */
-       init_params(seq->options, &new);
+       /* Check and set all option values */
+       init_params(seq->options, &new, true);
 
        /*
         * Create relation (and fill *null & *value)
@@ -299,7 +291,7 @@ DefineSequence(CreateSeqStmt *seq)
 /*
  * AlterSequence
  *
- * Modify the defition of a sequence relation
+ * Modify the definition of a sequence relation
  */
 void
 AlterSequence(AlterSeqStmt *stmt)
@@ -314,7 +306,7 @@ AlterSequence(AlterSeqStmt *stmt)
        /* open and AccessShareLock sequence */
        init_sequence(stmt->sequence, &elm, &seqrel);
 
-       /* allow DROP to sequence owner only */
+       /* allow ALTER to sequence owner only */
        if (!pg_class_ownercheck(elm->relid, GetUserId()))
                aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
                                           stmt->sequence->relname);
@@ -323,6 +315,7 @@ AlterSequence(AlterSeqStmt *stmt)
        seq = read_info(elm, seqrel, &buf);
        page = BufferGetPage(buf);
 
+       /* copy old values of options */
        new.increment_by = seq->increment_by;
        new.max_value = seq->max_value;
        new.min_value = seq->min_value;
@@ -330,9 +323,10 @@ AlterSequence(AlterSeqStmt *stmt)
        new.is_cycled = seq->is_cycled;
        new.last_value = seq->last_value;
 
-       /* Check and set values */
-       init_params(stmt->options, &new);
+       /* Check and set new values */
+       init_params(stmt->options, &new, false);
 
+       /* Now okay to update the on-disk tuple */
        seq->increment_by = new.increment_by;
        seq->max_value = new.max_value;
        seq->min_value = new.min_value;
@@ -871,16 +865,22 @@ read_info(SeqTable elm, Relation rel, Buffer *buf)
        return seq;
 }
 
-
+/*
+ * init_params: process the options list of CREATE or ALTER SEQUENCE,
+ * and store the values into appropriate fields of *new.
+ *
+ * If isInit is true, fill any unspecified options with default values;
+ * otherwise, do not change existing options that aren't explicitly overridden.
+ */
 static void
-init_params(List *options, Form_pg_sequence new)
+init_params(List *options, Form_pg_sequence new, bool isInit)
 {
        DefElem    *last_value = NULL;
        DefElem    *increment_by = NULL;
        DefElem    *max_value = NULL;
        DefElem    *min_value = NULL;
        DefElem    *cache_value = NULL;
-       bool            is_cycled_set = false;
+       DefElem    *is_cycled = NULL;
        List       *option;
 
        foreach(option, options)
@@ -934,12 +934,11 @@ init_params(List *options, Form_pg_sequence new)
                }
                else if (strcmp(defel->defname, "cycle") == 0)
                {
-                       if (is_cycled_set)
+                       if (is_cycled)
                                ereport(ERROR,
                                                (errcode(ERRCODE_SYNTAX_ERROR),
                                                 errmsg("conflicting or redundant options")));
-                       is_cycled_set = true;
-                       new->is_cycled = (defel->arg != NULL);
+                       is_cycled = defel;
                }
                else
                        elog(ERROR, "option \"%s\" not recognized",
@@ -947,9 +946,7 @@ init_params(List *options, Form_pg_sequence new)
        }
 
        /* INCREMENT BY */
-       if (new->increment_by == 0 && increment_by == (DefElem *) NULL)
-               new->increment_by = 1;
-       else if (increment_by != (DefElem *) NULL)
+       if (increment_by != (DefElem *) NULL)
        {
                new->increment_by = defGetInt64(increment_by);
                if (new->increment_by == 0)
@@ -957,31 +954,45 @@ init_params(List *options, Form_pg_sequence new)
                                        (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                                         errmsg("INCREMENT must not be zero")));
        }
+       else if (isInit)
+               new->increment_by = 1;
+
+       /* CYCLE */
+       if (is_cycled != (DefElem *) NULL)
+       {
+               new->is_cycled = intVal(is_cycled->arg);
+               Assert(new->is_cycled == false || new->is_cycled == true);
+       }
+       else if (isInit)
+               new->is_cycled = false;
 
-       /* MAXVALUE */
-       if ((new->max_value == 0 && max_value == (DefElem *) NULL)
-               || (max_value != (DefElem *) NULL && !max_value->arg))
+       /* MAXVALUE (null arg means NO MAXVALUE) */
+       if (max_value != (DefElem *) NULL && max_value->arg)
+       {
+               new->max_value = defGetInt64(max_value);
+       }
+       else if (isInit || max_value != (DefElem *) NULL)
        {
                if (new->increment_by > 0)
                        new->max_value = SEQ_MAXVALUE;          /* ascending seq */
                else
-                       new->max_value = -1;    /* descending seq */
+                       new->max_value = -1;                            /* descending seq */
        }
-       else if (max_value != (DefElem *) NULL)
-               new->max_value = defGetInt64(max_value);
 
-       /* MINVALUE */
-       if ((new->min_value == 0 && min_value == (DefElem *) NULL)
-               || (min_value != (DefElem *) NULL && !min_value->arg))
+       /* MINVALUE (null arg means NO MINVALUE) */
+       if (min_value != (DefElem *) NULL && min_value->arg)
+       {
+               new->min_value = defGetInt64(min_value);
+       }
+       else if (isInit || min_value != (DefElem *) NULL)
        {
                if (new->increment_by > 0)
-                       new->min_value = 1; /* ascending seq */
+                       new->min_value = 1;                                     /* ascending seq */
                else
                        new->min_value = SEQ_MINVALUE;          /* descending seq */
        }
-       else if (min_value != (DefElem *) NULL)
-               new->min_value = defGetInt64(min_value);
 
+       /* crosscheck min/max */
        if (new->min_value >= new->max_value)
        {
                char            bufm[100],
@@ -996,16 +1007,17 @@ init_params(List *options, Form_pg_sequence new)
        }
 
        /* START WITH */
-       if (new->last_value == 0 && last_value == (DefElem *) NULL)
+       if (last_value != (DefElem *) NULL)
+               new->last_value = defGetInt64(last_value);
+       else if (isInit)
        {
                if (new->increment_by > 0)
                        new->last_value = new->min_value;       /* ascending seq */
                else
                        new->last_value = new->max_value;       /* descending seq */
        }
-       else if (last_value != (DefElem *) NULL)
-               new->last_value = defGetInt64(last_value);
 
+       /* crosscheck */
        if (new->last_value < new->min_value)
        {
                char            bufs[100],
@@ -1032,17 +1044,22 @@ init_params(List *options, Form_pg_sequence new)
        }
 
        /* CACHE */
-       if (cache_value == (DefElem *) NULL)
-               new->cache_value = 1;
-       else if ((new->cache_value = defGetInt64(cache_value)) <= 0)
+       if (cache_value != (DefElem *) NULL)
        {
-               char            buf[100];
+               new->cache_value = defGetInt64(cache_value);
+               if (new->cache_value <= 0)
+               {
+                       char            buf[100];
 
-               snprintf(buf, sizeof(buf), INT64_FORMAT, new->cache_value);
-               ereport(ERROR,
-                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                                errmsg("CACHE (%s) must be greater than zero", buf)));
+                       snprintf(buf, sizeof(buf), INT64_FORMAT, new->cache_value);
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                        errmsg("CACHE (%s) must be greater than zero",
+                                                       buf)));
+               }
        }
+       else if (isInit)
+               new->cache_value = 1;
 }
 
 

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 337eef4480c8d52d4bde79bb2babf93b7ef11a74..3452b50fb03c33fad6e4453c1ec36970454e9072 100644 (file)
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -11,7 +11,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/parser/gram.y,v 2.438 2003/11/21 22:32:49 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/parser/gram.y,v 2.439 2003/11/24 16:54:07 tgl Exp $
  *
  * HISTORY
  *       AUTHOR                        DATE                    MAJOR EVENT
@@ -1926,11 +1926,11 @@ OptSeqElem: CACHE NumericOnly
                                }
                        | CYCLE
                                {
-                                       $$ = makeDefElem("cycle", (Node *)true);
+                                       $$ = makeDefElem("cycle", (Node *)makeInteger(TRUE));
                                }
                        | NO CYCLE
                                {
-                                       $$ = makeDefElem("cycle", (Node *)false);
+                                       $$ = makeDefElem("cycle", (Node *)makeInteger(FALSE));
                                }
                        | INCREMENT opt_by NumericOnly
                                {