Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitf4a3fdf

Browse files
committed
Avoid order-of-execution problems with ALTER TABLE ADD PRIMARY KEY.
Up to now, DefineIndex() was responsible for adding attnotnull constraintsto the columns of a primary key, in any case where it hadn't beenconvenient for transformIndexConstraint() to mark those columns asis_not_null. It (or rather its minion index_check_primary_key) did thisby executing an ALTER TABLE SET NOT NULL command for the target table.The trouble with this solution is that if we're creating the index dueto ALTER TABLE ADD PRIMARY KEY, and the outer ALTER TABLE has additionalsub-commands, the inner ALTER TABLE's operations executed at the wrongtime with respect to the outer ALTER TABLE's operations. In particular,the inner ALTER would perform a validation scan at a point where thetable's storage might be inconsistent with its catalog entries. (This ison the hairy edge of being a security problem, but AFAICS it isn't onebecause the inner scan would only be interested in the tuples' nullbitmaps.) This can result in unexpected failures, such as the one seenin bug #15580 from Allison Kaptur.To fix, let's remove the attempt to do SET NOT NULL from DefineIndex(),reducing index_check_primary_key's role to verifying that the columns arealready not null. (It shouldn't ever see such a case, but it seems wiseto keep the check for safety.) Instead, make transformIndexConstraint()generate ALTER TABLE SET NOT NULL subcommands to be executed ahead ofthe ADD PRIMARY KEY operation in every case where it can't force thecolumn to be created already-not-null. This requires only minor surgeryin parse_utilcmd.c, and it makes for a much more satisfying spec fortransformIndexConstraint(): it's no longer having to take it on faiththat someone else will handle addition of NOT NULL constraints.To make that work, we have to move the execution of AT_SetNotNull intoan ALTER pass that executes ahead of AT_PASS_ADD_INDEX. I moved it toAT_PASS_COL_ATTRS, and put that after AT_PASS_ADD_COL to avoid failurewhen the column is being added in the same command. This incidentallyfixes a bug in the only previous usage of AT_PASS_COL_ATTRS, forAT_SetIdentity: it didn't work either for a newly-added column.Playing around with this exposed a separate bug in ALTER TABLE ONLY ...ADD PRIMARY KEY for partitioned tables. The intent of the ONLY modifierin that context is to prevent doing anything that would require holdinglock for a long time --- but the implied SET NOT NULL would recurse tothe child partitions, and do an expensive validation scan for any childwhere the column(s) were not already NOT NULL. To fix that, invent anew ALTER subcommand AT_CheckNotNull that just insists that a childcolumn be already NOT NULL, and apply that, not AT_SetNotNull, whenrecursing to children in this scenario. This results in a slightly laxerdefinition of ALTER TABLE ONLY ... SET NOT NULL for partitioned tables,too: that command will now work as long as all children are already NOTNULL, whereas before it just threw up its hands if there were anypartitions.In passing, clean up the API of generateClonedIndexStmt(): remove auseless argument, ensure that the output argument is not left undefined,update the header comment.A small side effect of this change is that no-such-column errors in ALTERTABLE ADD PRIMARY KEY now produce a different message that includes thetable name, because they are now detected by the SET NOT NULL step whichhas historically worded its error that way. That seems fine to me, soI didn't make any effort to avoid the wording change.The basic bug #15580 is of very long standing, and these other bugsaren't new in v12 either. However, this is a pretty significant changein the way ALTER TABLE ADD PRIMARY KEY works. On balance it seems bestnot to back-patch, at least not till we get some more confidence thatthis patch has no new bugs.Patch by me, but thanks to Jie Zhang for a preliminary version.Discussion:https://postgr.es/m/15580-d1a6de5a3d65da51@postgresql.orgDiscussion:https://postgr.es/m/1396E95157071C4EBBA51892C5368521017F2E6E63@G08CNEXMBPEKD02.g08.fujitsu.local
1 parentc06e355 commitf4a3fdf

File tree

14 files changed

+320
-109
lines changed

14 files changed

+320
-109
lines changed

‎src/backend/catalog/index.c

Lines changed: 15 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -185,13 +185,14 @@ relationHasPrimaryKey(Relation rel)
185185
*
186186
* We check for a pre-existing primary key, and that all columns of the index
187187
* are simple column references (not expressions), and that all those
188-
* columns are marked NOT NULL. If they aren't (which can only happen during
189-
* ALTER TABLE ADD CONSTRAINT, since the parser forces such columns to be
190-
* created NOT NULL during CREATE TABLE), do an ALTER SET NOT NULL to mark
191-
* them so --- or fail if they are not in fact nonnull.
188+
* columns are marked NOT NULL. If not, fail.
192189
*
193-
* As of PG v10, the SET NOT NULL is applied to child tables as well, so
194-
* that the behavior is like a manual SET NOT NULL.
190+
* We used to automatically change unmarked columns to NOT NULL here by doing
191+
* our own local ALTER TABLE command. But that doesn't work well if we're
192+
* executing one subcommand of an ALTER TABLE: the operations may not get
193+
* performed in the right order overall. Now we expect that the parser
194+
* inserted any required ALTER TABLE SET NOT NULL operations before trying
195+
* to create a primary-key index.
195196
*
196197
* Caller had better have at least ShareLock on the table, else the not-null
197198
* checking isn't trustworthy.
@@ -202,12 +203,11 @@ index_check_primary_key(Relation heapRel,
202203
boolis_alter_table,
203204
IndexStmt*stmt)
204205
{
205-
List*cmds;
206206
inti;
207207

208208
/*
209-
* If ALTER TABLEand CREATE TABLE .. PARTITION OF, check that there isn't
210-
* already a PRIMARY KEY. In CREATE TABLE for an ordinaryrelations, we
209+
* If ALTER TABLEor CREATE TABLE .. PARTITION OF, check that there isn't
210+
* already a PRIMARY KEY. In CREATE TABLE for an ordinaryrelation, we
211211
* have faith that the parser rejected multiple pkey clauses; and CREATE
212212
* INDEX doesn't have a way to say PRIMARY KEY, so it's no problem either.
213213
*/
@@ -222,9 +222,9 @@ index_check_primary_key(Relation heapRel,
222222

223223
/*
224224
* Check that all of the attributes in a primary key are marked as not
225-
* null, otherwise attempt to ALTER TABLE .. SET NOT NULL
225+
* null. (We don't really expect to see that; it'd mean the parser messed
226+
* up. But it seems wise to check anyway.)
226227
*/
227-
cmds=NIL;
228228
for (i=0;i<indexInfo->ii_NumIndexKeyAttrs;i++)
229229
{
230230
AttrNumberattnum=indexInfo->ii_IndexAttrNumbers[i];
@@ -249,30 +249,13 @@ index_check_primary_key(Relation heapRel,
249249
attform= (Form_pg_attribute)GETSTRUCT(atttuple);
250250

251251
if (!attform->attnotnull)
252-
{
253-
/* Add a subcommand to make this one NOT NULL */
254-
AlterTableCmd*cmd=makeNode(AlterTableCmd);
255-
256-
cmd->subtype=AT_SetNotNull;
257-
cmd->name=pstrdup(NameStr(attform->attname));
258-
cmds=lappend(cmds,cmd);
259-
}
252+
ereport(ERROR,
253+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
254+
errmsg("primary key column \"%s\" is not marked NOT NULL",
255+
NameStr(attform->attname))));
260256

261257
ReleaseSysCache(atttuple);
262258
}
263-
264-
/*
265-
* XXX: possible future improvement: when being called from ALTER TABLE,
266-
* it would be more efficient to merge this with the outer ALTER TABLE, so
267-
* as to avoid two scans. But that seems to complicate DefineIndex's API
268-
* unduly.
269-
*/
270-
if (cmds)
271-
{
272-
EventTriggerAlterTableStart((Node*)stmt);
273-
AlterTableInternal(RelationGetRelid(heapRel),cmds, true);
274-
EventTriggerAlterTableEnd();
275-
}
276259
}
277260

278261
/*

‎src/backend/commands/tablecmds.c

Lines changed: 104 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -142,9 +142,9 @@ static List *on_commits = NIL;
142142
#define AT_PASS_ALTER_TYPE1/* ALTER COLUMN TYPE */
143143
#define AT_PASS_OLD_INDEX2/* re-add existing indexes */
144144
#define AT_PASS_OLD_CONSTR3/* re-add existing constraints */
145-
#define AT_PASS_COL_ATTRS4/* set other column attributes */
146145
/* We could support a RENAME COLUMN pass here, but not currently used */
147-
#define AT_PASS_ADD_COL5/* ADD COLUMN */
146+
#define AT_PASS_ADD_COL4/* ADD COLUMN */
147+
#define AT_PASS_COL_ATTRS5/* set other column attributes */
148148
#define AT_PASS_ADD_INDEX6/* ADD indexes */
149149
#define AT_PASS_ADD_CONSTR7/* ADD constraints, defaults */
150150
#define AT_PASS_MISC8/* other stuff */
@@ -370,9 +370,13 @@ static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
370370
static void add_column_collation_dependency(Oid relid, int32 attnum, Oid collid);
371371
static void ATPrepDropNotNull(Relation rel, bool recurse, bool recursing);
372372
static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode);
373-
static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
373+
static void ATPrepSetNotNull(List **wqueue, Relation rel,
374+
AlterTableCmd *cmd, bool recurse, bool recursing,
375+
LOCKMODE lockmode);
374376
static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
375377
const char *colName, LOCKMODE lockmode);
378+
static void ATExecCheckNotNull(AlteredTableInfo *tab, Relation rel,
379+
const char *colName, LOCKMODE lockmode);
376380
static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
377381
static bool ConstraintImpliedByRelConstraint(Relation scanrel,
378382
List *partConstraint, List *existedConstraints);
@@ -1068,7 +1072,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
10681072
RelationGetDescr(parent),
10691073
gettext_noop("could not convert row type"));
10701074
idxstmt =
1071-
generateClonedIndexStmt(NULL,RelationGetRelid(rel),idxRel,
1075+
generateClonedIndexStmt(NULL, idxRel,
10721076
attmap, RelationGetDescr(rel)->natts,
10731077
&constraintOid);
10741078
DefineIndex(RelationGetRelid(rel),
@@ -3765,6 +3769,15 @@ AlterTableGetLockLevel(List *cmds)
37653769
cmd_lockmode = AccessExclusiveLock;
37663770
break;
37673771

3772+
case AT_CheckNotNull:
3773+
3774+
/*
3775+
* This only examines the table's schema; but lock must be
3776+
* strong enough to prevent concurrent DROP NOT NULL.
3777+
*/
3778+
cmd_lockmode = AccessShareLock;
3779+
break;
3780+
37683781
default:/* oops */
37693782
elog(ERROR, "unrecognized alter table type: %d",
37703783
(int) cmd->subtype);
@@ -3889,15 +3902,19 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
38893902
ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
38903903
ATPrepDropNotNull(rel, recurse, recursing);
38913904
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
3892-
/* No command-specific prep needed */
38933905
pass = AT_PASS_DROP;
38943906
break;
38953907
case AT_SetNotNull:/* ALTER COLUMN SET NOT NULL */
38963908
ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
3897-
ATPrepSetNotNull(rel, recurse, recursing);
3909+
/* Need command-specific recursion decision */
3910+
ATPrepSetNotNull(wqueue, rel, cmd, recurse, recursing, lockmode);
3911+
pass = AT_PASS_COL_ATTRS;
3912+
break;
3913+
case AT_CheckNotNull:/* check column is already marked NOT NULL */
3914+
ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
38983915
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
38993916
/* No command-specific prep needed */
3900-
pass =AT_PASS_ADD_CONSTR;
3917+
pass =AT_PASS_COL_ATTRS;
39013918
break;
39023919
case AT_SetStatistics:/* ALTER COLUMN SET STATISTICS */
39033920
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
@@ -4214,6 +4231,9 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
42144231
case AT_SetNotNull:/* ALTER COLUMN SET NOT NULL */
42154232
address = ATExecSetNotNull(tab, rel, cmd->name, lockmode);
42164233
break;
4234+
case AT_CheckNotNull:/* check column is already marked NOT NULL */
4235+
ATExecCheckNotNull(tab, rel, cmd->name, lockmode);
4236+
break;
42174237
case AT_SetStatistics:/* ALTER COLUMN SET STATISTICS */
42184238
address = ATExecSetStatistics(rel, cmd->name, cmd->num, cmd->def, lockmode);
42194239
break;
@@ -5966,9 +5986,6 @@ add_column_collation_dependency(Oid relid, int32 attnum, Oid collid)
59665986

59675987
/*
59685988
* ALTER TABLE ALTER COLUMN DROP NOT NULL
5969-
*
5970-
* Return the address of the modified column. If the column was already
5971-
* nullable, InvalidObjectAddress is returned.
59725989
*/
59735990

59745991
static void
@@ -5990,6 +6007,11 @@ ATPrepDropNotNull(Relation rel, bool recurse, bool recursing)
59906007
errhint("Do not specify the ONLY keyword.")));
59916008
}
59926009
}
6010+
6011+
/*
6012+
* Return the address of the modified column. If the column was already
6013+
* nullable, InvalidObjectAddress is returned.
6014+
*/
59936015
static ObjectAddress
59946016
ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
59956017
{
@@ -6116,23 +6138,33 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
61166138
*/
61176139

61186140
static void
6119-
ATPrepSetNotNull(Relation rel, bool recurse, bool recursing)
6141+
ATPrepSetNotNull(List **wqueue, Relation rel,
6142+
AlterTableCmd *cmd, bool recurse, bool recursing,
6143+
LOCKMODE lockmode)
61206144
{
61216145
/*
6122-
* If the parent is a partitioned table, like check constraints, NOT NULL
6123-
* constraints must be added to the child tables. Complain if requested
6124-
* otherwise and partitions exist.
6146+
* If we're already recursing, there's nothing to do; the topmost
6147+
* invocation of ATSimpleRecursion already visited all children.
61256148
*/
6126-
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
6149+
if (recursing)
6150+
return;
6151+
6152+
/*
6153+
* If we have ALTER TABLE ONLY ... SET NOT NULL on a partitioned table,
6154+
* apply ALTER TABLE ... CHECK NOT NULL to every child. Otherwise, use
6155+
* normal recursion logic.
6156+
*/
6157+
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
6158+
!recurse)
61276159
{
6128-
PartitionDesc partdesc =RelationGetPartitionDesc(rel);
6160+
AlterTableCmd *newcmd =makeNode(AlterTableCmd);
61296161

6130-
if (partdesc && partdesc->nparts > 0 && !recurse && !recursing)
6131-
ereport(ERROR,
6132-
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
6133-
errmsg("cannot add constraint to only the partitioned table when partitions exist"),
6134-
errhint("Do not specify the ONLY keyword.")));
6162+
newcmd->subtype = AT_CheckNotNull;
6163+
newcmd->name = pstrdup(cmd->name);
6164+
ATSimpleRecursion(wqueue, rel, newcmd, true, lockmode);
61356165
}
6166+
else
6167+
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
61366168
}
61376169

61386170
/*
@@ -6207,6 +6239,46 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
62076239
return address;
62086240
}
62096241

6242+
/*
6243+
* ALTER TABLE ALTER COLUMN CHECK NOT NULL
6244+
*
6245+
* This doesn't exist in the grammar, but we generate AT_CheckNotNull
6246+
* commands against the partitions of a partitioned table if the user
6247+
* writes ALTER TABLE ONLY ... SET NOT NULL on the partitioned table,
6248+
* or tries to create a primary key on it (which internally creates
6249+
* AT_SetNotNull on the partitioned table). Such a command doesn't
6250+
* allow us to actually modify any partition, but we want to let it
6251+
* go through if the partitions are already properly marked.
6252+
*
6253+
* In future, this might need to adjust the child table's state, likely
6254+
* by incrementing an inheritance count for the attnotnull constraint.
6255+
* For now we need only check for the presence of the flag.
6256+
*/
6257+
static void
6258+
ATExecCheckNotNull(AlteredTableInfo *tab, Relation rel,
6259+
const char *colName, LOCKMODE lockmode)
6260+
{
6261+
HeapTupletuple;
6262+
6263+
tuple = SearchSysCacheAttName(RelationGetRelid(rel), colName);
6264+
6265+
if (!HeapTupleIsValid(tuple))
6266+
ereport(ERROR,
6267+
(errcode(ERRCODE_UNDEFINED_COLUMN),
6268+
errmsg("column \"%s\" of relation \"%s\" does not exist",
6269+
colName, RelationGetRelationName(rel))));
6270+
6271+
if (!((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull)
6272+
ereport(ERROR,
6273+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
6274+
errmsg("constraint must be added to child tables too"),
6275+
errdetail("Column \"%s\" of relation \"%s\" is not already NOT NULL.",
6276+
colName, RelationGetRelationName(rel)),
6277+
errhint("Do not specify the ONLY keyword.")));
6278+
6279+
ReleaseSysCache(tuple);
6280+
}
6281+
62106282
/*
62116283
* NotNullImpliedByRelConstraints
62126284
*Does rel's existing constraints imply NOT NULL for the given attribute?
@@ -11269,6 +11341,16 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
1126911341
NIL,
1127011342
con->conname);
1127111343
}
11344+
else if (cmd->subtype == AT_SetNotNull)
11345+
{
11346+
/*
11347+
* The parser will create AT_SetNotNull subcommands for
11348+
* columns of PRIMARY KEY indexes/constraints, but we need
11349+
* not do anything with them here, because the columns'
11350+
* NOT NULL marks will already have been propagated into
11351+
* the new table definition.
11352+
*/
11353+
}
1127211354
else
1127311355
elog(ERROR, "unexpected statement subtype: %d",
1127411356
(int) cmd->subtype);
@@ -15649,7 +15731,7 @@ AttachPartitionEnsureIndexes(Relation rel, Relation attachrel)
1564915731
IndexStmt *stmt;
1565015732
OidconstraintOid;
1565115733

15652-
stmt = generateClonedIndexStmt(NULL, RelationGetRelid(attachrel),
15734+
stmt = generateClonedIndexStmt(NULL,
1565315735
idxRel, attmap,
1565415736
RelationGetDescr(rel)->natts,
1565515737
&constraintOid);

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp