Rearranging ALTER TABLE to avoid multi-operations bugs
| От | Tom Lane |
|---|---|
| Тема | Rearranging ALTER TABLE to avoid multi-operations bugs |
| Дата | |
| Msg-id | 10365.1558909428@sss.pgh.pa.us обсуждение исходный текст |
| Ответы |
Re: Rearranging ALTER TABLE to avoid multi-operations bugs
(Robert Haas <robertmhaas@gmail.com>)
|
| Список | pgsql-hackers |
We've had numerous bug reports complaining about the fact that ALTER TABLE
generates subsidiary commands that get executed unconditionally, even
if they should be discarded due to an IF NOT EXISTS or other condition;
see e.g. #14827, #15180, #15670, #15710. In [1] I speculated about
fixing this by having ALTER TABLE maintain an array of flags that record
the results of initial tests for column existence, and then letting it
conditionalize execution of subcommands on those flags. I started to
fool around with that concept today, and quickly realized that my
original thought of just adding execute-if-this-flag-is-true markers to
AlterTableCmd subcommands was insufficient. Most of the problems are with
independent commands that execute before or after the main AlterTable,
and would not have any easy connection to state maintained by AlterTable.
The way to fix this, I think, is to provide an AlterTableCmd subcommand
type that just wraps an arbitrary utility statement, and then we can
conditionalize execution of such subcommands using the flag mechanism.
So instead of generating independent "before" and "after" statements,
transformAlterTableStmt would just produce a single AlterTable with
everything in its list of subcommands --- but we'd still use the generic
ProcessUtility infrastructure to execute subcommands that correspond
to existing standalone statements.
Looking into parse_utilcmd.c with an eye to making it do that, I almost
immediately ran across bugs we hadn't even known were there in ALTER TABLE
ADD/DROP GENERATED. These have got a different but arguably-related
flavor of bug: they are making decisions inside transformAlterTableStmt
that might be wrong by the time we get to execution. Thus for example
regression=# create table t1 (f1 int);
CREATE TABLE
regression=# alter table t1 add column f2 int not null,
alter column f2 add generated always as identity;
ALTER TABLE
regression=# insert into t1 values(0);
ERROR: no owned sequence found
This happens because transformAlterTableStmt thinks it can generate
the sequence creation commands for the AT_AddIdentity subcommand,
and also figures it's okay to just ignore the case where the column
doesn't exist. So we create the column but then we don't make the
sequence. There are similar bugs in AT_SetIdentity processing, and
I rather suspect that it's also unsafe for AT_AlterColumnType to be
looking at the column's attidentity state --- though I couldn't
demonstrate a bug in that path, because of the fact that
AT_AlterColumnType executes in a pass earlier than anything that
could change attidentity.
This can't be fixed just by conditionalizing execution of subcommands,
because we need to know the target column's type in order to set up the
sequence correctly. So what has to happen to fix these things is to
move the decisions, and the creation of the subcommand parsetrees,
into ALTER TABLE execution.
That requires pretty much the same support for recursively calling
ProcessUtility() from AlterTable() that we'd need for the subcommand
wrapper idea. So I went ahead and tackled it as a separate project,
and attached is the result.
I'm not quite sure if I'm satisfied with the approach shown here.
I made a struct containing the ProcessUtility parameters that need
to be passed down through the recursion, originally with the idea
that this struct might be completely opaque outside utility.c.
However, there's a good deal of redundancy in that approach ---
the relid and stmt parameters of AlterTable() are really redundant
with stuff in the struct. So now I'm wondering if it would be better
to merge all that stuff and just have the struct as AlterTable's sole
argument. I'm also not very sure whether AlterTableInternal() ought
to be modified so that it uses or at least creates a valid struct;
it doesn't *need* to do so today, but maybe someday it will.
And the whole thing has a faint air of grottiness about it too.
This makes the minimum changes to what we've got now, but I can't
help thinking it'd look different if we'd designed from scratch.
The interactions with event triggers seem particularly ad-hoc.
It's also ugly that CreateTable's recursion is handled differently
from AlterTable's.
Anybody have thoughts about a different way to approach it?
regards, tom lane
[1] https://postgr.es/m/7824.1525200461@sss.pgh.pa.us
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e34d4cc..73c56ed 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -86,6 +86,7 @@
#include "storage/lock.h"
#include "storage/predicate.h"
#include "storage/smgr.h"
+#include "tcop/utility.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
@@ -340,12 +341,15 @@ static void validateForeignKeyConstraint(char *conname,
Relation rel, Relation pkrel,
Oid pkindOid, Oid constraintOid);
static void ATController(AlterTableStmt *parsetree,
- Relation rel, List *cmds, bool recurse, LOCKMODE lockmode);
+ Relation rel, List *cmds, bool recurse, LOCKMODE lockmode,
+ ProcessUtilityForAlterTableContext *pcontext);
static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
bool recurse, bool recursing, LOCKMODE lockmode);
-static void ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode);
+static void ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
+ ProcessUtilityForAlterTableContext *pcontext);
static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
- AlterTableCmd *cmd, LOCKMODE lockmode);
+ AlterTableCmd *cmd, LOCKMODE lockmode,
+ ProcessUtilityForAlterTableContext *pcontext);
static void ATRewriteTables(AlterTableStmt *parsetree,
List **wqueue, LOCKMODE lockmode);
static void ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode);
@@ -383,9 +387,11 @@ static bool ConstraintImpliedByRelConstraint(Relation scanrel,
static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
Node *newDefault, LOCKMODE lockmode);
static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
- Node *def, LOCKMODE lockmode);
+ Node *def, LOCKMODE lockmode,
+ ProcessUtilityForAlterTableContext *pcontext);
static ObjectAddress ATExecSetIdentity(Relation rel, const char *colName,
- Node *def, LOCKMODE lockmode);
+ Node *def, LOCKMODE lockmode,
+ ProcessUtilityForAlterTableContext *pcontext);
static ObjectAddress ATExecDropIdentity(Relation rel, const char *colName, bool missing_ok, LOCKMODE lockmode);
static void ATPrepSetStatistics(Relation rel, const char *colName, int16 colNum,
Node *newValue, LOCKMODE lockmode);
@@ -3467,7 +3473,8 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
* rather than reassess it at lower levels.
*/
void
-AlterTable(Oid relid, LOCKMODE lockmode, AlterTableStmt *stmt)
+AlterTable(Oid relid, LOCKMODE lockmode, AlterTableStmt *stmt,
+ ProcessUtilityForAlterTableContext *pcontext)
{
Relation rel;
@@ -3476,7 +3483,8 @@ AlterTable(Oid relid, LOCKMODE lockmode, AlterTableStmt *stmt)
CheckTableNotInUse(rel, "ALTER TABLE");
- ATController(stmt, rel, stmt->cmds, stmt->relation->inh, lockmode);
+ ATController(stmt, rel, stmt->cmds, stmt->relation->inh, lockmode,
+ pcontext);
}
/*
@@ -3489,6 +3497,9 @@ AlterTable(Oid relid, LOCKMODE lockmode, AlterTableStmt *stmt)
* is unsafe to use this entry point for alterations that could break
* existing query plans. On the assumption it's not used for such, we
* don't have to reject pending AFTER triggers, either.
+ *
+ * This also doesn't support subcommands that need to recursively call
+ * ProcessUtility, so no pcontext argument is needed.
*/
void
AlterTableInternal(Oid relid, List *cmds, bool recurse)
@@ -3500,7 +3511,7 @@ AlterTableInternal(Oid relid, List *cmds, bool recurse)
EventTriggerAlterTableRelid(relid);
- ATController(NULL, rel, cmds, recurse, lockmode);
+ ATController(NULL, rel, cmds, recurse, lockmode, NULL);
}
/*
@@ -3798,7 +3809,8 @@ AlterTableGetLockLevel(List *cmds)
*/
static void
ATController(AlterTableStmt *parsetree,
- Relation rel, List *cmds, bool recurse, LOCKMODE lockmode)
+ Relation rel, List *cmds, bool recurse, LOCKMODE lockmode,
+ ProcessUtilityForAlterTableContext *pcontext)
{
List *wqueue = NIL;
ListCell *lcmd;
@@ -3815,7 +3827,7 @@ ATController(AlterTableStmt *parsetree,
relation_close(rel, NoLock);
/* Phase 2: update system catalogs */
- ATRewriteCatalogs(&wqueue, lockmode);
+ ATRewriteCatalogs(&wqueue, lockmode, pcontext);
/* Phase 3: scan/rewrite tables as needed */
ATRewriteTables(parsetree, &wqueue, lockmode);
@@ -3884,7 +3896,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_AddIdentity:
ATSimplePermissions(rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
- pass = AT_PASS_ADD_CONSTR;
+ pass = AT_PASS_COL_ATTRS;
break;
case AT_DropIdentity:
ATSimplePermissions(rel, ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE);
@@ -4122,7 +4134,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
* conflicts).
*/
static void
-ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
+ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
+ ProcessUtilityForAlterTableContext *pcontext)
{
int pass;
ListCell *ltab;
@@ -4153,9 +4166,12 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
rel = relation_open(tab->relid, NoLock);
foreach(lcmd, subcmds)
+ {
ATExecCmd(wqueue, tab, rel,
castNode(AlterTableCmd, lfirst(lcmd)),
- lockmode);
+ lockmode,
+ pcontext);
+ }
/*
* After the ALTER TYPE pass, do cleanup work (this is not done in
@@ -4192,7 +4208,8 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
*/
static void
ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
- AlterTableCmd *cmd, LOCKMODE lockmode)
+ AlterTableCmd *cmd, LOCKMODE lockmode,
+ ProcessUtilityForAlterTableContext *pcontext)
{
ObjectAddress address = InvalidObjectAddress;
@@ -4213,10 +4230,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
address = ATExecColumnDefault(rel, cmd->name, cmd->def, lockmode);
break;
case AT_AddIdentity:
- address = ATExecAddIdentity(rel, cmd->name, cmd->def, lockmode);
+ address = ATExecAddIdentity(rel, cmd->name, cmd->def, lockmode,
+ pcontext);
break;
case AT_SetIdentity:
- address = ATExecSetIdentity(rel, cmd->name, cmd->def, lockmode);
+ address = ATExecSetIdentity(rel, cmd->name, cmd->def, lockmode,
+ pcontext);
break;
case AT_DropIdentity:
address = ATExecDropIdentity(rel, cmd->name, cmd->missing_ok, lockmode);
@@ -6398,14 +6417,17 @@ ATExecColumnDefault(Relation rel, const char *colName,
*/
static ObjectAddress
ATExecAddIdentity(Relation rel, const char *colName,
- Node *def, LOCKMODE lockmode)
+ Node *def, LOCKMODE lockmode,
+ ProcessUtilityForAlterTableContext *pcontext)
{
+ Constraint *condef = castNode(Constraint, def);
Relation attrelation;
HeapTuple tuple;
Form_pg_attribute attTup;
AttrNumber attnum;
+ List *seqcmds;
+ ListCell *lc;
ObjectAddress address;
- ColumnDef *cdef = castNode(ColumnDef, def);
attrelation = table_open(AttributeRelationId, RowExclusiveLock);
@@ -6448,9 +6470,22 @@ ATExecAddIdentity(Relation rel, const char *colName,
errmsg("column \"%s\" of relation \"%s\" already has a default value",
colName, RelationGetRelationName(rel))));
- attTup->attidentity = cdef->identity;
+ /* Update column's attidentity state */
+ attTup->attidentity = condef->generated_when;
CatalogTupleUpdate(attrelation, &tuple->t_self, tuple);
+ /* Create the required supporting sequence */
+ seqcmds = transformIdentityColumnSerialOptions(rel,
+ NameStr(attTup->attname),
+ attTup->atttypid,
+ condef->options);
+ foreach(lc, seqcmds)
+ {
+ Node *stmt = lfirst(lc);
+
+ ProcessUtilityForAlterTable(stmt, pcontext);
+ }
+
InvokeObjectPostAlterHook(RelationRelationId,
RelationGetRelid(rel),
attTup->attnum);
@@ -6469,17 +6504,21 @@ ATExecAddIdentity(Relation rel, const char *colName,
* Return the address of the affected column.
*/
static ObjectAddress
-ATExecSetIdentity(Relation rel, const char *colName, Node *def, LOCKMODE lockmode)
+ATExecSetIdentity(Relation rel, const char *colName, Node *def, LOCKMODE lockmode,
+ ProcessUtilityForAlterTableContext *pcontext)
{
+ List *options = castNode(List, def);
ListCell *option;
DefElem *generatedEl = NULL;
+ List *seqoptions = NIL;
HeapTuple tuple;
Form_pg_attribute attTup;
AttrNumber attnum;
Relation attrelation;
ObjectAddress address;
- foreach(option, castNode(List, def))
+ /* Examine options */
+ foreach(option, options)
{
DefElem *defel = lfirst_node(DefElem, option);
@@ -6492,14 +6531,15 @@ ATExecSetIdentity(Relation rel, const char *colName, Node *def, LOCKMODE lockmod
generatedEl = defel;
}
else
- elog(ERROR, "option \"%s\" not recognized",
- defel->defname);
+ {
+ /* Assume it is an option for ALTER SEQUENCE */
+ seqoptions = lappend(seqoptions, defel);
+ }
}
/*
- * Even if there is nothing to change here, we run all the checks. There
- * will be a subsequent ALTER SEQUENCE that relies on everything being
- * there.
+ * Even if there is nothing to change, verify that target column is valid
+ * for the command.
*/
attrelation = table_open(AttributeRelationId, RowExclusiveLock);
@@ -6525,6 +6565,7 @@ ATExecSetIdentity(Relation rel, const char *colName, Node *def, LOCKMODE lockmod
errmsg("column \"%s\" of relation \"%s\" is not an identity column",
colName, RelationGetRelationName(rel))));
+ /* Apply attidentity change if given */
if (generatedEl)
{
attTup->attidentity = defGetInt32(generatedEl);
@@ -6532,13 +6573,35 @@ ATExecSetIdentity(Relation rel, const char *colName, Node *def, LOCKMODE lockmod
InvokeObjectPostAlterHook(RelationRelationId,
RelationGetRelid(rel),
- attTup->attnum);
+ attnum);
ObjectAddressSubSet(address, RelationRelationId,
RelationGetRelid(rel), attnum);
}
else
address = InvalidObjectAddress;
+ /* Apply sequence options if given */
+ if (seqoptions)
+ {
+ List *seqlist = getOwnedSequences(RelationGetRelid(rel), attnum);
+ ListCell *seqcell;
+
+ foreach(seqcell, seqlist)
+ {
+ Oid seq_relid = lfirst_oid(seqcell);
+ AlterSeqStmt *seqstmt;
+
+ seqstmt = makeNode(AlterSeqStmt);
+ seqstmt->sequence = makeRangeVar(get_namespace_name(get_rel_namespace(seq_relid)),
+ get_rel_name(seq_relid), -1);
+ seqstmt->options = seqoptions;
+ seqstmt->for_identity = true;
+ seqstmt->missing_ok = false;
+
+ ProcessUtilityForAlterTable((Node *) seqstmt, pcontext);
+ }
+ }
+
heap_freetuple(tuple);
table_close(attrelation, RowExclusiveLock);
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 7450d74..a0526d4 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -113,6 +113,10 @@ typedef struct
} CreateSchemaStmtContext;
+static void generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
+ Oid seqtypid, List *seqoptions,
+ bool for_identity,
+ char **snamespace_p, char **sname_p);
static void transformColumnDefinition(CreateStmtContext *cxt,
ColumnDef *column);
static void transformTableConstraint(CreateStmtContext *cxt,
@@ -337,6 +341,38 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
}
/*
+ * transformIdentityColumnSerialOptions
+ * Generate CREATE SEQUENCE and ALTER SEQUENCE ... OWNED BY statements
+ * to create the sequence for an identity column.
+ *
+ * This is used during ALTER TABLE ADD IDENTITY. We don't need to separate
+ * the execution of the two commands, because the column already exists and
+ * doesn't need its default expression set. So just pass them back as a
+ * single List.
+ */
+List *
+transformIdentityColumnSerialOptions(Relation rel,
+ char *colName, Oid colTypeOid,
+ List *seqoptions)
+{
+ CreateStmtContext cxt;
+ ColumnDef *column = makeNode(ColumnDef);
+
+ /* Set up just enough of cxt for generateSerialExtraStmts() */
+ memset(&cxt, 0, sizeof(cxt));
+ cxt.stmtType = "ALTER TABLE";
+ cxt.rel = rel;
+
+ /* Need a mostly-dummy ColumnDef, too */
+ column->colname = colName;
+
+ generateSerialExtraStmts(&cxt, column, colTypeOid, seqoptions, true,
+ NULL, NULL);
+
+ return list_concat(cxt.blist, cxt.alist);
+}
+
+/*
* generateSerialExtraStmts
* Generate CREATE SEQUENCE and ALTER SEQUENCE ... OWNED BY statements
* to create the sequence for a serial or identity column.
@@ -350,7 +386,7 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
Oid seqtypid, List *seqoptions, bool for_identity,
char **snamespace_p, char **sname_p)
{
- ListCell *option;
+ char *relname;
DefElem *nameEl = NULL;
Oid snamespaceid;
char *snamespace;
@@ -358,6 +394,14 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
CreateSeqStmt *seqstmt;
AlterSeqStmt *altseqstmt;
List *attnamelist;
+ ListCell *option;
+
+ /*
+ * Get name of relation. Note: this function mustn't access cxt->relation
+ * when cxt->rel is set, because transformIdentityColumnSerialOptions()
+ * only provides the latter.
+ */
+ relname = cxt->rel ? RelationGetRelationName(cxt->rel) : cxt->relation->relname;
/*
* Determine namespace and name to use for the sequence.
@@ -415,7 +459,7 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
RangeVarAdjustRelationPersistence(cxt->relation, snamespaceid);
}
snamespace = get_namespace_name(snamespaceid);
- sname = ChooseRelationName(cxt->relation->relname,
+ sname = ChooseRelationName(relname,
column->colname,
"seq",
snamespaceid,
@@ -425,7 +469,7 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
ereport(DEBUG1,
(errmsg("%s will create implicit sequence \"%s\" for serial column \"%s.%s\"",
cxt->stmtType, sname,
- cxt->relation->relname, column->colname)));
+ relname, column->colname)));
/*
* Build a CREATE SEQUENCE command to create the sequence object, and add
@@ -478,7 +522,7 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
altseqstmt = makeNode(AlterSeqStmt);
altseqstmt->sequence = makeRangeVar(snamespace, sname, -1);
attnamelist = list_make3(makeString(snamespace),
- makeString(cxt->relation->relname),
+ makeString(relname),
makeString(column->colname));
altseqstmt->options = list_make1(makeDefElem("owned_by",
(Node *) attnamelist, -1));
@@ -3077,8 +3121,14 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
/*
* The only subtypes that currently require parse transformation handling
- * are ADD COLUMN, ADD CONSTRAINT and SET DATA TYPE. These largely re-use
- * code from CREATE TABLE.
+ * are ADD COLUMN, ADD CONSTRAINT, SET DATA TYPE, and ATTACH/DETACH
+ * PARTITION. These largely re-use code from CREATE TABLE.
+ *
+ * NOW HEAR THIS: you can NOT put code here that examines the current
+ * properties of the target table or anything associated with it. Such
+ * code will do the wrong thing if any preceding ALTER TABLE subcommand
+ * changes the property in question. Wait till runtime to look at the
+ * table.
*/
foreach(lcmd, stmt->cmds)
{
@@ -3155,6 +3205,14 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
/*
* For identity column, create ALTER SEQUENCE command to
* change the data type of the sequence.
+ *
+ * XXX This is a direct violation of the advice given
+ * above to not look at the table's properties yet. It
+ * accidentally works (at least for most cases) because of
+ * the ordering of operations in ALTER TABLE --- note in
+ * particular that we must add the new command to blist
+ * not alist. But we ought to move this to be done at
+ * execution.
*/
attnum = get_attnum(relid, cmd->name);
@@ -3181,90 +3239,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
break;
}
- case AT_AddIdentity:
- {
- Constraint *def = castNode(Constraint, cmd->def);
- ColumnDef *newdef = makeNode(ColumnDef);
- AttrNumber attnum;
-
- newdef->colname = cmd->name;
- newdef->identity = def->generated_when;
- cmd->def = (Node *) newdef;
-
- attnum = get_attnum(relid, cmd->name);
-
- /*
- * if attribute not found, something will error about it
- * later
- */
- if (attnum != InvalidAttrNumber)
- generateSerialExtraStmts(&cxt, newdef,
- get_atttype(relid, attnum),
- def->options, true,
- NULL, NULL);
-
- newcmds = lappend(newcmds, cmd);
- break;
- }
-
- case AT_SetIdentity:
- {
- /*
- * Create an ALTER SEQUENCE statement for the internal
- * sequence of the identity column.
- */
- ListCell *lc;
- List *newseqopts = NIL;
- List *newdef = NIL;
- List *seqlist;
- AttrNumber attnum;
-
- /*
- * Split options into those handled by ALTER SEQUENCE and
- * those for ALTER TABLE proper.
- */
- foreach(lc, castNode(List, cmd->def))
- {
- DefElem *def = lfirst_node(DefElem, lc);
-
- if (strcmp(def->defname, "generated") == 0)
- newdef = lappend(newdef, def);
- else
- newseqopts = lappend(newseqopts, def);
- }
-
- attnum = get_attnum(relid, cmd->name);
-
- if (attnum)
- {
- seqlist = getOwnedSequences(relid, attnum);
- if (seqlist)
- {
- AlterSeqStmt *seqstmt;
- Oid seq_relid;
-
- seqstmt = makeNode(AlterSeqStmt);
- seq_relid = linitial_oid(seqlist);
- seqstmt->sequence = makeRangeVar(get_namespace_name(get_rel_namespace(seq_relid)),
- get_rel_name(seq_relid), -1);
- seqstmt->options = newseqopts;
- seqstmt->for_identity = true;
- seqstmt->missing_ok = false;
-
- cxt.alist = lappend(cxt.alist, seqstmt);
- }
- }
-
- /*
- * If column was not found or was not an identity column,
- * we just let the ALTER TABLE command error out later.
- */
-
- cmd->def = (Node *) newdef;
- newcmds = lappend(newcmds, cmd);
- break;
- }
-
case AT_AttachPartition:
case AT_DetachPartition:
{
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 9578b5c..937a9b7 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1075,7 +1075,7 @@ ProcessUtilitySlow(ParseState *pstate,
queryString,
PROCESS_UTILITY_SUBCOMMAND,
params,
- NULL,
+ queryEnv,
None_Receiver,
NULL);
}
@@ -1097,8 +1097,6 @@ ProcessUtilitySlow(ParseState *pstate,
{
AlterTableStmt *atstmt = (AlterTableStmt *) parsetree;
Oid relid;
- List *stmts;
- ListCell *l;
LOCKMODE lockmode;
/*
@@ -1112,10 +1110,21 @@ ProcessUtilitySlow(ParseState *pstate,
if (OidIsValid(relid))
{
+ List *stmts;
+ ProcessUtilityForAlterTableContext pcontext;
+ ListCell *l;
+
/* Run parse analysis ... */
stmts = transformAlterTableStmt(relid, atstmt,
queryString);
+ /* ... set up info for possible recursion ... */
+ pcontext.pstmt = pstmt;
+ pcontext.queryString = queryString;
+ pcontext.params = params;
+ pcontext.queryEnv = queryEnv;
+ pcontext.relid = relid;
+
/* ... ensure we have an event trigger context ... */
EventTriggerAlterTableStart(parsetree);
EventTriggerAlterTableRelid(relid);
@@ -1129,36 +1138,21 @@ ProcessUtilitySlow(ParseState *pstate,
{
/* Do the table alteration proper */
AlterTable(relid, lockmode,
- (AlterTableStmt *) stmt);
+ (AlterTableStmt *) stmt,
+ &pcontext);
}
else
{
/*
- * Recurse for anything else. If we need to
- * do so, "close" the current complex-command
- * set, and start a new one at the bottom;
- * this is needed to ensure the ordering of
- * queued commands is consistent with the way
- * they are executed here.
+ * Recurse for anything else. We get here if
+ * transformAlterTableStmt() tacked extra
+ * commands onto its output, but it's also
+ * possible for AlterTable() to generate extra
+ * commands on-the-fly, in which case it will
+ * call ProcessUtilityForAlterTable directly.
*/
- PlannedStmt *wrapper;
-
- EventTriggerAlterTableEnd();
- wrapper = makeNode(PlannedStmt);
- wrapper->commandType = CMD_UTILITY;
- wrapper->canSetTag = false;
- wrapper->utilityStmt = stmt;
- wrapper->stmt_location = pstmt->stmt_location;
- wrapper->stmt_len = pstmt->stmt_len;
- ProcessUtility(wrapper,
- queryString,
- PROCESS_UTILITY_SUBCOMMAND,
- params,
- NULL,
- None_Receiver,
- NULL);
- EventTriggerAlterTableStart(parsetree);
- EventTriggerAlterTableRelid(relid);
+ ProcessUtilityForAlterTable(stmt,
+ &pcontext);
}
/* Need CCI between commands */
@@ -1719,6 +1713,42 @@ ProcessUtilitySlow(ParseState *pstate,
}
/*
+ * ProcessUtilityForAlterTable
+ * Recursively process an arbitrary utility command as a subcommand
+ * of ALTER TABLE.
+ */
+void
+ProcessUtilityForAlterTable(Node *stmt,
+ ProcessUtilityForAlterTableContext *pcontext)
+{
+ PlannedStmt *wrapper = makeNode(PlannedStmt);
+
+ /*
+ * When recursing, "close" the current complex-command set, and start a
+ * new one afterwards; this is needed to ensure the ordering of queued
+ * commands is consistent with the way they are executed here.
+ */
+ EventTriggerAlterTableEnd();
+
+ wrapper->commandType = CMD_UTILITY;
+ wrapper->canSetTag = false;
+ wrapper->utilityStmt = stmt;
+ wrapper->stmt_location = pcontext->pstmt->stmt_location;
+ wrapper->stmt_len = pcontext->pstmt->stmt_len;
+
+ ProcessUtility(wrapper,
+ pcontext->queryString,
+ PROCESS_UTILITY_SUBCOMMAND,
+ pcontext->params,
+ pcontext->queryEnv,
+ None_Receiver,
+ NULL);
+
+ EventTriggerAlterTableStart(pcontext->pstmt->utilityStmt);
+ EventTriggerAlterTableRelid(pcontext->relid);
+}
+
+/*
* Dispatch function for DropStmt
*/
static void
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index b09afa2..25282c0 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -21,6 +21,8 @@
#include "storage/lock.h"
#include "utils/relcache.h"
+struct ProcessUtilityForAlterTableContext; /* avoid including utility.h */
+
extern ObjectAddress DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
ObjectAddress *typaddress, const char *queryString);
@@ -29,7 +31,8 @@ extern void RemoveRelations(DropStmt *drop);
extern Oid AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode);
-extern void AlterTable(Oid relid, LOCKMODE lockmode, AlterTableStmt *stmt);
+extern void AlterTable(Oid relid, LOCKMODE lockmode, AlterTableStmt *stmt,
+ struct ProcessUtilityForAlterTableContext *pcontext);
extern LOCKMODE AlterTableGetLockLevel(List *cmds);
diff --git a/src/include/parser/parse_utilcmd.h b/src/include/parser/parse_utilcmd.h
index 1348064..3f30fc6 100644
--- a/src/include/parser/parse_utilcmd.h
+++ b/src/include/parser/parse_utilcmd.h
@@ -27,6 +27,9 @@ extern void transformRuleStmt(RuleStmt *stmt, const char *queryString,
extern List *transformCreateSchemaStmt(CreateSchemaStmt *stmt);
extern PartitionBoundSpec *transformPartitionBound(ParseState *pstate, Relation parent,
PartitionBoundSpec *spec);
+extern List *transformIdentityColumnSerialOptions(Relation rel,
+ char *colName, Oid colTypeOid,
+ List *seqoptions);
extern IndexStmt *generateClonedIndexStmt(RangeVar *heapRel,
Relation source_idx,
const AttrNumber *attmap, int attmap_length,
diff --git a/src/include/tcop/utility.h b/src/include/tcop/utility.h
index 5abcacf..34390e6 100644
--- a/src/include/tcop/utility.h
+++ b/src/include/tcop/utility.h
@@ -25,6 +25,16 @@ typedef enum
PROCESS_UTILITY_SUBCOMMAND /* a portion of a query */
} ProcessUtilityContext;
+typedef struct ProcessUtilityForAlterTableContext
+{
+ /* Info that has to be passed through an ALTER TABLE */
+ PlannedStmt *pstmt;
+ const char *queryString;
+ ParamListInfo params;
+ QueryEnvironment *queryEnv;
+ Oid relid;
+} ProcessUtilityForAlterTableContext;
+
/* Hook for plugins to get control in ProcessUtility() */
typedef void (*ProcessUtility_hook_type) (PlannedStmt *pstmt,
const char *queryString, ProcessUtilityContext context,
@@ -42,6 +52,9 @@ extern void standard_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
QueryEnvironment *queryEnv,
DestReceiver *dest, char *completionTag);
+extern void ProcessUtilityForAlterTable(Node *stmt,
+ ProcessUtilityForAlterTableContext *pcontext);
+
extern bool UtilityReturnsTuples(Node *parsetree);
extern TupleDesc UtilityTupleDescriptor(Node *parsetree);
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index 2286519..b72c9d0 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -39,7 +39,7 @@ SELECT pg_get_serial_sequence('itest1', 'a');
integer | 1 | 1 | 2147483647 | 1 | no | 1
Sequence for identity column: public.itest1.a
-CREATE TABLE itest4 (a int, b text);
+CREATE TABLE itest4 (a int, b text not null);
ALTER TABLE itest4 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY; -- error, requires NOT NULL
ERROR: column "a" of relation "itest4" must be declared NOT NULL before identity can be added
ALTER TABLE itest4 ALTER COLUMN a SET NOT NULL;
@@ -387,6 +387,68 @@ SELECT * FROM itest8;
RESET ROLE;
DROP TABLE itest8;
DROP USER regress_identity_user1;
+-- multiple steps in ALTER TABLE
+CREATE TABLE itest8 (f1 int);
+ALTER TABLE itest8
+ ADD COLUMN f2 int NOT NULL,
+ ALTER COLUMN f2 ADD GENERATED ALWAYS AS IDENTITY;
+ALTER TABLE itest8
+ ADD COLUMN f3 int NOT NULL,
+ ALTER COLUMN f3 ADD GENERATED ALWAYS AS IDENTITY,
+ ALTER COLUMN f3 SET GENERATED BY DEFAULT SET INCREMENT 10;
+ALTER TABLE itest8
+ ADD COLUMN f4 int;
+ALTER TABLE itest8
+ ALTER COLUMN f4 SET NOT NULL,
+ ALTER COLUMN f4 ADD GENERATED ALWAYS AS IDENTITY,
+ ALTER COLUMN f4 SET DATA TYPE bigint;
+ALTER TABLE itest8
+ ADD COLUMN f5 int GENERATED ALWAYS AS IDENTITY;
+ALTER TABLE itest8
+ ALTER COLUMN f5 DROP IDENTITY,
+ ALTER COLUMN f5 DROP NOT NULL,
+ ALTER COLUMN f5 SET DATA TYPE bigint;
+INSERT INTO itest8 VALUES(0), (1);
+TABLE itest8;
+ f1 | f2 | f3 | f4 | f5
+----+----+----+----+----
+ 0 | 1 | 1 | 1 |
+ 1 | 2 | 11 | 2 |
+(2 rows)
+
+\d+ itest8
+ Table "public.itest8"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
+--------+---------+-----------+----------+----------------------------------+---------+--------------+-------------
+ f1 | integer | | | | plain | |
+ f2 | integer | | not null | generated always as identity | plain | |
+ f3 | integer | | not null | generated by default as identity | plain | |
+ f4 | bigint | | not null | generated always as identity | plain | |
+ f5 | bigint | | | | plain | |
+
+\d itest8_f2_seq
+ Sequence "public.itest8_f2_seq"
+ Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
+---------+-------+---------+------------+-----------+---------+-------
+ integer | 1 | 1 | 2147483647 | 1 | no | 1
+Sequence for identity column: public.itest8.f2
+
+\d itest8_f3_seq
+ Sequence "public.itest8_f3_seq"
+ Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
+---------+-------+---------+------------+-----------+---------+-------
+ integer | 1 | 1 | 2147483647 | 10 | no | 1
+Sequence for identity column: public.itest8.f3
+
+\d itest8_f4_seq
+ Sequence "public.itest8_f4_seq"
+ Type | Start | Minimum | Maximum | Increment | Cycles? | Cache
+--------+-------+---------+---------------------+-----------+---------+-------
+ bigint | 1 | 1 | 9223372036854775807 | 1 | no | 1
+Sequence for identity column: public.itest8.f4
+
+\d itest8_f5_seq
+DROP TABLE itest8;
-- typed tables (currently not supported)
CREATE TYPE itest_type AS (f1 integer, f2 text, f3 bigint);
CREATE TABLE itest12 OF itest_type (f1 WITH OPTIONS GENERATED ALWAYS AS IDENTITY); -- error
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 8dcfdf3..5126a66 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -16,7 +16,7 @@ SELECT pg_get_serial_sequence('itest1', 'a');
\d itest1_a_seq
-CREATE TABLE itest4 (a int, b text);
+CREATE TABLE itest4 (a int, b text not null);
ALTER TABLE itest4 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY; -- error, requires NOT NULL
ALTER TABLE itest4 ALTER COLUMN a SET NOT NULL;
ALTER TABLE itest4 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY; -- ok
@@ -239,6 +239,44 @@ RESET ROLE;
DROP TABLE itest8;
DROP USER regress_identity_user1;
+-- multiple steps in ALTER TABLE
+CREATE TABLE itest8 (f1 int);
+
+ALTER TABLE itest8
+ ADD COLUMN f2 int NOT NULL,
+ ALTER COLUMN f2 ADD GENERATED ALWAYS AS IDENTITY;
+
+ALTER TABLE itest8
+ ADD COLUMN f3 int NOT NULL,
+ ALTER COLUMN f3 ADD GENERATED ALWAYS AS IDENTITY,
+ ALTER COLUMN f3 SET GENERATED BY DEFAULT SET INCREMENT 10;
+
+ALTER TABLE itest8
+ ADD COLUMN f4 int;
+
+ALTER TABLE itest8
+ ALTER COLUMN f4 SET NOT NULL,
+ ALTER COLUMN f4 ADD GENERATED ALWAYS AS IDENTITY,
+ ALTER COLUMN f4 SET DATA TYPE bigint;
+
+ALTER TABLE itest8
+ ADD COLUMN f5 int GENERATED ALWAYS AS IDENTITY;
+
+ALTER TABLE itest8
+ ALTER COLUMN f5 DROP IDENTITY,
+ ALTER COLUMN f5 DROP NOT NULL,
+ ALTER COLUMN f5 SET DATA TYPE bigint;
+
+INSERT INTO itest8 VALUES(0), (1);
+
+TABLE itest8;
+\d+ itest8
+\d itest8_f2_seq
+\d itest8_f3_seq
+\d itest8_f4_seq
+\d itest8_f5_seq
+DROP TABLE itest8;
+
-- typed tables (currently not supported)
В списке pgsql-hackers по дате отправления: