Skip to content

Commit 1281a5c

Browse files
tglsfdctglsfdc
committed
Restructure ALTER TABLE execution to fix assorted bugs.
We've had numerous bug reports about how (1) IF NOT EXISTS clauses in ALTER TABLE don't behave as-expected, and (2) combining certain actions into one ALTER TABLE doesn't work, though executing the same actions as separate statements does. This patch cleans up all of the cases so far reported from the field, though there are still some oddities associated with identity columns. The core problem behind all of these bugs is that we do parse analysis of ALTER TABLE subcommands too soon, before starting execution of the statement. The root of the bugs in group (1) is that parse analysis schedules derived commands (such as a CREATE SEQUENCE for a serial column) before it's known whether the IF NOT EXISTS clause should cause a subcommand to be skipped. The root of the bugs in group (2) is that earlier subcommands may change the catalog state that later subcommands need to be parsed against. Hence, postpone parse analysis of ALTER TABLE's subcommands, and do that one subcommand at a time, during "phase 2" of ALTER TABLE which is the phase that does catalog rewrites. Thus the catalog effects of earlier subcommands are already visible when we analyze later ones. (The sole exception is that we do parse analysis for ALTER COLUMN TYPE subcommands during phase 1, so that their USING expressions can be parsed against the table's original state, which is what we need. Arguably, these bugs stem from falsely concluding that because ALTER COLUMN TYPE must do early parse analysis, every other command subtype can too.) This means that ALTER TABLE itself must deal with execution of any non-ALTER-TABLE derived statements that are generated by parse analysis. Add a suitable entry point to utility.c to accept those recursive calls, and create a struct to pass through the information needed by the recursive call, rather than making the argument lists of AlterTable() and friends even longer. Getting this to work correctly required a little bit of fiddling with the subcommand pass structure, in particular breaking up AT_PASS_ADD_CONSTR into multiple passes. But otherwise it's mostly a pretty straightforward application of the above ideas. Fixing the residual issues for identity columns requires refactoring of where the dependency link from an identity column to its sequence gets set up. So that seems like suitable material for a separate patch, especially since this one is pretty big already. Discussion: https://postgr.es/m/10365.1558909428@sss.pgh.pa.us
1 parent a166d40 commit 1281a5c

File tree

13 files changed

+827
-267
lines changed

13 files changed

+827
-267
lines changed

‎src/backend/commands/tablecmds.c

+387-107
Large diffs are not rendered by default.

‎src/backend/commands/view.c

+4
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,10 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace,
145145
* Note that we must do this before updating the query for the view,
146146
* since the rules system requires that the correct view columns be in
147147
* place when defining the new rules.
148+
*
149+
* Also note that ALTER TABLE doesn't run parse transformation on
150+
* AT_AddColumnToView commands. The ColumnDef we supply must be ready
151+
* to execute as-is.
148152
*/
149153
if (list_length(attrList) > rel->rd_att->natts)
150154
{

‎src/backend/parser/parse_utilcmd.c

+77-65
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,8 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
347347
*/
348348
static void
349349
generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
350-
Oid seqtypid, List *seqoptions, bool for_identity,
350+
Oid seqtypid, List *seqoptions,
351+
bool for_identity, bool col_exists,
351352
char **snamespace_p, char **sname_p)
352353
{
353354
ListCell *option;
@@ -472,8 +473,12 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
472473

473474
/*
474475
* Build an ALTER SEQUENCE ... OWNED BY command to mark the sequence as
475-
* owned by this column, and add it to the list of things to be done after
476-
* this CREATE/ALTER TABLE.
476+
* owned by this column, and add it to the appropriate list of things to
477+
* be done along with this CREATE/ALTER TABLE. In a CREATE or ALTER ADD
478+
* COLUMN, it must be done after the statement because we don't know the
479+
* column's attnum yet. But if we do have the attnum (in AT_AddIdentity),
480+
* we can do the marking immediately, which improves some ALTER TABLE
481+
* behaviors.
477482
*/
478483
altseqstmt = makeNode(AlterSeqStmt);
479484
altseqstmt->sequence = makeRangeVar(snamespace, sname, -1);
@@ -484,7 +489,10 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
484489
(Node *) attnamelist, -1));
485490
altseqstmt->for_identity = for_identity;
486491

487-
cxt->alist = lappend(cxt->alist, altseqstmt);
492+
if (col_exists)
493+
cxt->blist = lappend(cxt->blist, altseqstmt);
494+
else
495+
cxt->alist = lappend(cxt->alist, altseqstmt);
488496

489497
if (snamespace_p)
490498
*snamespace_p = snamespace;
@@ -568,7 +576,8 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
568576
Constraint *constraint;
569577

570578
generateSerialExtraStmts(cxt, column,
571-
column->typeName->typeOid, NIL, false,
579+
column->typeName->typeOid, NIL,
580+
false, false,
572581
&snamespace, &sname);
573582

574583
/*
@@ -684,7 +693,8 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
684693
constraint->location)));
685694

686695
generateSerialExtraStmts(cxt, column,
687-
typeOid, constraint->options, true,
696+
typeOid, constraint->options,
697+
true, false,
688698
NULL, NULL);
689699

690700
column->identity = constraint->generated_when;
@@ -1086,7 +1096,8 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
10861096
seq_relid = getIdentitySequence(RelationGetRelid(relation), attribute->attnum, false);
10871097
seq_options = sequence_options(seq_relid);
10881098
generateSerialExtraStmts(cxt, def,
1089-
InvalidOid, seq_options, true,
1099+
InvalidOid, seq_options,
1100+
true, false,
10901101
NULL, NULL);
10911102
def->identity = attribute->attidentity;
10921103
}
@@ -2572,7 +2583,7 @@ transformFKConstraints(CreateStmtContext *cxt,
25722583
Constraint *constraint = (Constraint *) lfirst(fkclist);
25732584
AlterTableCmd *altercmd = makeNode(AlterTableCmd);
25742585

2575-
altercmd->subtype = AT_ProcessedConstraint;
2586+
altercmd->subtype = AT_AddConstraint;
25762587
altercmd->name = NULL;
25772588
altercmd->def = (Node *) constraint;
25782589
alterstmt->cmds = lappend(alterstmt->cmds, altercmd);
@@ -3004,23 +3015,23 @@ transformRuleStmt(RuleStmt *stmt, const char *queryString,
30043015
* transformAlterTableStmt -
30053016
* parse analysis for ALTER TABLE
30063017
*
3007-
* Returns a List of utility commands to be done in sequence. One of these
3008-
* will be the transformed AlterTableStmt, but there may be additional actions
3009-
* to be done before and after the actual AlterTable() call.
3018+
* Returns the transformed AlterTableStmt. There may be additional actions
3019+
* to be done before and after the transformed statement, which are returned
3020+
* in *beforeStmts and *afterStmts as lists of utility command parsetrees.
30103021
*
30113022
* To avoid race conditions, it's important that this function rely only on
30123023
* the passed-in relid (and not on stmt->relation) to determine the target
30133024
* relation.
30143025
*/
3015-
List *
3026+
AlterTableStmt *
30163027
transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
3017-
const char *queryString)
3028+
const char *queryString,
3029+
List **beforeStmts, List **afterStmts)
30183030
{
30193031
Relation rel;
30203032
TupleDesc tupdesc;
30213033
ParseState *pstate;
30223034
CreateStmtContext cxt;
3023-
List *result;
30243035
List *save_alist;
30253036
ListCell *lcmd,
30263037
*l;
@@ -3052,7 +3063,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
30523063

30533064
/* Set up CreateStmtContext */
30543065
cxt.pstate = pstate;
3055-
if (stmt->relkind == OBJECT_FOREIGN_TABLE)
3066+
if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
30563067
{
30573068
cxt.stmtType = "ALTER FOREIGN TABLE";
30583069
cxt.isforeign = true;
@@ -3080,9 +3091,8 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
30803091
cxt.ofType = false;
30813092

30823093
/*
3083-
* The only subtypes that currently require parse transformation handling
3084-
* are ADD COLUMN, ADD CONSTRAINT and SET DATA TYPE. These largely re-use
3085-
* code from CREATE TABLE.
3094+
* Transform ALTER subcommands that need it (most don't). These largely
3095+
* re-use code from CREATE TABLE.
30863096
*/
30873097
foreach(lcmd, stmt->cmds)
30883098
{
@@ -3091,7 +3101,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
30913101
switch (cmd->subtype)
30923102
{
30933103
case AT_AddColumn:
3094-
case AT_AddColumnToView:
3104+
case AT_AddColumnRecurse:
30953105
{
30963106
ColumnDef *def = castNode(ColumnDef, cmd->def);
30973107

@@ -3115,6 +3125,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
31153125
}
31163126

31173127
case AT_AddConstraint:
3128+
case AT_AddConstraintRecurse:
31183129

31193130
/*
31203131
* The original AddConstraint cmd node doesn't go to newcmds
@@ -3130,19 +3141,9 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
31303141
(int) nodeTag(cmd->def));
31313142
break;
31323143

3133-
case AT_ProcessedConstraint:
3134-
3135-
/*
3136-
* Already-transformed ADD CONSTRAINT, so just make it look
3137-
* like the standard case.
3138-
*/
3139-
cmd->subtype = AT_AddConstraint;
3140-
newcmds = lappend(newcmds, cmd);
3141-
break;
3142-
31433144
case AT_AlterColumnType:
31443145
{
3145-
ColumnDef *def = (ColumnDef *) cmd->def;
3146+
ColumnDef *def = castNode(ColumnDef, cmd->def);
31463147
AttrNumber attnum;
31473148

31483149
/*
@@ -3161,13 +3162,13 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
31613162
* change the data type of the sequence.
31623163
*/
31633164
attnum = get_attnum(relid, cmd->name);
3165+
if (attnum == InvalidAttrNumber)
3166+
ereport(ERROR,
3167+
(errcode(ERRCODE_UNDEFINED_COLUMN),
3168+
errmsg("column \"%s\" of relation \"%s\" does not exist",
3169+
cmd->name, RelationGetRelationName(rel))));
31643170

3165-
/*
3166-
* if attribute not found, something will error about it
3167-
* later
3168-
*/
3169-
if (attnum != InvalidAttrNumber &&
3170-
TupleDescAttr(tupdesc, attnum - 1)->attidentity)
3171+
if (TupleDescAttr(tupdesc, attnum - 1)->attidentity)
31713172
{
31723173
Oid seq_relid = getIdentitySequence(relid, attnum, false);
31733174
Oid typeOid = typenameTypeId(pstate, def->typeName);
@@ -3196,16 +3197,16 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
31963197
cmd->def = (Node *) newdef;
31973198

31983199
attnum = get_attnum(relid, cmd->name);
3200+
if (attnum == InvalidAttrNumber)
3201+
ereport(ERROR,
3202+
(errcode(ERRCODE_UNDEFINED_COLUMN),
3203+
errmsg("column \"%s\" of relation \"%s\" does not exist",
3204+
cmd->name, RelationGetRelationName(rel))));
31993205

3200-
/*
3201-
* if attribute not found, something will error about it
3202-
* later
3203-
*/
3204-
if (attnum != InvalidAttrNumber)
3205-
generateSerialExtraStmts(&cxt, newdef,
3206-
get_atttype(relid, attnum),
3207-
def->options, true,
3208-
NULL, NULL);
3206+
generateSerialExtraStmts(&cxt, newdef,
3207+
get_atttype(relid, attnum),
3208+
def->options, true, true,
3209+
NULL, NULL);
32093210

32103211
newcmds = lappend(newcmds, cmd);
32113212
break;
@@ -3221,6 +3222,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
32213222
List *newseqopts = NIL;
32223223
List *newdef = NIL;
32233224
AttrNumber attnum;
3225+
Oid seq_relid;
32243226

32253227
/*
32263228
* Split options into those handled by ALTER SEQUENCE and
@@ -3237,29 +3239,34 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
32373239
}
32383240

32393241
attnum = get_attnum(relid, cmd->name);
3242+
if (attnum == InvalidAttrNumber)
3243+
ereport(ERROR,
3244+
(errcode(ERRCODE_UNDEFINED_COLUMN),
3245+
errmsg("column \"%s\" of relation \"%s\" does not exist",
3246+
cmd->name, RelationGetRelationName(rel))));
32403247

3241-
if (attnum)
3242-
{
3243-
Oid seq_relid = getIdentitySequence(relid, attnum, true);
3248+
seq_relid = getIdentitySequence(relid, attnum, true);
32443249

3245-
if (seq_relid)
3246-
{
3247-
AlterSeqStmt *seqstmt;
3250+
if (seq_relid)
3251+
{
3252+
AlterSeqStmt *seqstmt;
32483253

3249-
seqstmt = makeNode(AlterSeqStmt);
3250-
seqstmt->sequence = makeRangeVar(get_namespace_name(get_rel_namespace(seq_relid)),
3251-
get_rel_name(seq_relid), -1);
3252-
seqstmt->options = newseqopts;
3253-
seqstmt->for_identity = true;
3254-
seqstmt->missing_ok = false;
3254+
seqstmt = makeNode(AlterSeqStmt);
3255+
seqstmt->sequence = makeRangeVar(get_namespace_name(get_rel_namespace(seq_relid)),
3256+
get_rel_name(seq_relid), -1);
3257+
seqstmt->options = newseqopts;
3258+
seqstmt->for_identity = true;
3259+
seqstmt->missing_ok = false;
32553260

3256-
cxt.alist = lappend(cxt.alist, seqstmt);
3257-
}
3261+
cxt.blist = lappend(cxt.blist, seqstmt);
32583262
}
32593263

32603264
/*
3261-
* If column was not found or was not an identity column,
3262-
* we just let the ALTER TABLE command error out later.
3265+
* If column was not an identity column, we just let the
3266+
* ALTER TABLE command error out later. (There are cases
3267+
* this fails to cover, but we'll need to restructure
3268+
* where creation of the sequence dependency linkage
3269+
* happens before we can fix it.)
32633270
*/
32643271

32653272
cmd->def = (Node *) newdef;
@@ -3281,6 +3288,12 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
32813288
break;
32823289

32833290
default:
3291+
3292+
/*
3293+
* Currently, we shouldn't actually get here for subcommand
3294+
* types that don't require transformation; but if we do, just
3295+
* emit them unchanged.
3296+
*/
32843297
newcmds = lappend(newcmds, cmd);
32853298
break;
32863299
}
@@ -3361,11 +3374,10 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
33613374
*/
33623375
stmt->cmds = newcmds;
33633376

3364-
result = lappend(cxt.blist, stmt);
3365-
result = list_concat(result, cxt.alist);
3366-
result = list_concat(result, save_alist);
3377+
*beforeStmts = cxt.blist;
3378+
*afterStmts = list_concat(cxt.alist, save_alist);
33673379

3368-
return result;
3380+
return stmt;
33693381
}
33703382

33713383

0 commit comments

Comments
 (0)