mirror/postgresql
2
0
Fork 0
mirror of https://git.postgresql.org/git/postgresql.git synced 2024-11-21 03:13:05 +08:00
Code Issues Packages Projects Releases Wiki Activity

Further fix ALTER COLUMN TYPE's handling of indexes and index constraints.

Browse Source 
This patch reverts all the code changes of commit e76de8861, which turns
out to have been seriously misguided.  We can't wait till later to compute
the definition string for an index; we must capture that before applying
the data type change for any column it depends on, else ruleutils.c will
deliverr wrong/misleading results.  (This fine point was documented
nowhere, of course.)

I'd also managed to forget that ATExecAlterColumnType executes once per
ALTER COLUMN TYPE clause, not once per statement; which resulted in the
code being basically completely broken for any case in which multiple ALTER
COLUMN TYPE clauses are applied to a table having non-constraint indexes
that must be rebuilt.  Through very bad luck, none of the existing test
cases nor the ones added by e76de8861 caught that, but of course it was
soon found in the field.

The previous patch also had an implicit assumption that if a constraint's
index had a dependency on a table column, so would the constraint --- but
that isn't actually true, so it didn't fix such cases.

Instead of trying to delete unneeded index dependencies later, do the
is-there-a-constraint lookup immediately on seeing an index dependency,
and switch to remembering the constraint if so.  In the unusual case of
multiple column dependencies for a constraint index, this will result in
duplicate constraint lookups, but that's not that horrible compared to all
the other work that happens here.  Besides, such cases did not work at all
before, so it's hard to argue that they're performance-critical for anyone.

Per bug #15865 from Keith Fiske.  As before, back-patch to all supported
branches.

Discussion: https://postgr.es/m/15865-17940eacc8f8b081@postgresql.org
This commit is contained in:
Tom Lane 2019-06-24 16:43:05 -04:00
parent 05399b1488
commit cb8962ce8e
3 changed files with 125 additions and 93 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

184
src/backend/commands/tablecmds.c
View File

@ -415,6 +415,9 @@ static void ATPrepAlterColumnType(List **wqueue,
static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno);
static ObjectAddress ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
AlterTableCmd *cmd, LOCKMODE lockmode);
static void RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab,
DependencyType deptype);
static void RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab);
static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
List *options, LOCKMODE lockmode);
static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab,
@ -9021,9 +9024,6 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
SysScanDesc scan;
HeapTuple depTup;
ObjectAddress address;
ListCell *lc;
ListCell *prev;
ListCell *next;
attrelation = heap_open(AttributeRelationId, RowExclusiveLock);
@ -9092,11 +9092,6 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
* performed all the individual ALTER TYPE operations. We have to save
* the info before executing ALTER TYPE, though, else the deparser will
* get confused.
*
* There could be multiple entries for the same object, so we must check
* to ensure we process each one only once. Note: we assume that an index
* that implements a constraint will not show a direct dependency on the
* column.
*/
depRel = heap_open(DependRelationId, RowExclusiveLock);
@ -9137,20 +9132,8 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
if (relKind == RELKIND_INDEX)
{
/*
* Indexes that are directly dependent on the table
* might be regular indexes or constraint indexes.
* Constraint indexes typically have only indirect
* dependencies; but there are exceptions, notably
* partial exclusion constraints. Hence we must check
* whether the index depends on any constraint that's
* due to be rebuilt, which we'll do below after we've
* found all such constraints.
*/
Assert(foundObject.objectSubId == 0);
tab->changedIndexOids =
list_append_unique_oid(tab->changedIndexOids,
foundObject.objectId);
RememberIndexForRebuilding(foundObject.objectId, tab);
}
else if (relKind == RELKIND_SEQUENCE)
{
@ -9171,39 +9154,8 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
case OCLASS_CONSTRAINT:
Assert(foundObject.objectSubId == 0);
if (!list_member_oid(tab->changedConstraintOids,
foundObject.objectId))
{
char *defstring = pg_get_constraintdef_command(foundObject.objectId);
/*
* Put NORMAL dependencies at the front of the list and
* AUTO dependencies at the back. This makes sure that
* foreign-key constraints depending on this column will
* be dropped before unique or primary-key constraints of
* the column; which we must have because the FK
* constraints depend on the indexes belonging to the
* unique constraints.
*/
if (foundDep->deptype == DEPENDENCY_NORMAL)
{
tab->changedConstraintOids =
lcons_oid(foundObject.objectId,
tab->changedConstraintOids);
tab->changedConstraintDefs =
lcons(defstring,
tab->changedConstraintDefs);
}
else
{
tab->changedConstraintOids =
lappend_oid(tab->changedConstraintOids,
foundObject.objectId);
tab->changedConstraintDefs =
lappend(tab->changedConstraintDefs,
defstring);
}
}
RememberConstraintForRebuilding(foundObject.objectId, tab,
foundDep->deptype);
break;
case OCLASS_REWRITE:
@ -9322,41 +9274,6 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
systable_endscan(scan);
/*
* Check the collected index OIDs to see which ones belong to the
* constraint(s) of the table, and drop those from the list of indexes
* that we need to process; rebuilding the constraints will handle them.
*/
prev = NULL;
for (lc = list_head(tab->changedIndexOids); lc; lc = next)
{
Oid indexoid = lfirst_oid(lc);
Oid conoid;
next = lnext(lc);
conoid = get_index_constraint(indexoid);
if (OidIsValid(conoid) &&
list_member_oid(tab->changedConstraintOids, conoid))
tab->changedIndexOids = list_delete_cell(tab->changedIndexOids,
lc, prev);
else
prev = lc;
}
/*
* Now collect the definitions of the indexes that must be rebuilt. (We
* could merge this into the previous loop, but it'd be more complicated
* for little gain.)
*/
foreach(lc, tab->changedIndexOids)
{
Oid indexoid = lfirst_oid(lc);
tab->changedIndexDefs = lappend(tab->changedIndexDefs,
pg_get_indexdef_string(indexoid));
}
/*
* Now scan for dependencies of this column on other things. The only
* thing we should find is the dependency on the column datatype, which we
@ -9460,6 +9377,95 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
return address;
}
/*
* Subroutine for ATExecAlterColumnType: remember that a constraint needs
* to be rebuilt (which we might already know).
*/
static void
RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab,
DependencyType deptype)
{
/*
* This de-duplication check is critical for two independent reasons: we
* mustn't try to recreate the same constraint twice, and if a constraint
* depends on more than one column whose type is to be altered, we must
* capture its definition string before applying any of the column type
* changes. ruleutils.c will get confused if we ask again later.
*/
if (!list_member_oid(tab->changedConstraintOids, conoid))
{
/* OK, capture the constraint's existing definition string */
char *defstring = pg_get_constraintdef_command(conoid);
/*
* Put NORMAL dependencies at the front of the list and AUTO
* dependencies at the back. This makes sure that foreign-key
* constraints depending on this column will be dropped before unique
* or primary-key constraints of the column; which we must have
* because the FK constraints depend on the indexes belonging to the
* unique constraints.
*/
if (deptype == DEPENDENCY_NORMAL)
{
tab->changedConstraintOids = lcons_oid(conoid,
tab->changedConstraintOids);
tab->changedConstraintDefs = lcons(defstring,
tab->changedConstraintDefs);
}
else
{
tab->changedConstraintOids = lappend_oid(tab->changedConstraintOids,
conoid);
tab->changedConstraintDefs = lappend(tab->changedConstraintDefs,
defstring);
}
}
}
/*
* Subroutine for ATExecAlterColumnType: remember that an index needs
* to be rebuilt (which we might already know).
*/
static void
RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab)
{
/*
* This de-duplication check is critical for two independent reasons: we
* mustn't try to recreate the same index twice, and if an index depends
* on more than one column whose type is to be altered, we must capture
* its definition string before applying any of the column type changes.
* ruleutils.c will get confused if we ask again later.
*/
if (!list_member_oid(tab->changedIndexOids, indoid))
{
/*
* Before adding it as an index-to-rebuild, we'd better see if it
* belongs to a constraint, and if so rebuild the constraint instead.
* Typically this check fails, because constraint indexes normally
* have only dependencies on their constraint. But it's possible for
* such an index to also have direct dependencies on table columns,
* for example with a partial exclusion constraint.
*/
Oid conoid = get_index_constraint(indoid);
if (OidIsValid(conoid))
{
/* index dependencies on columns should generally be AUTO */
RememberConstraintForRebuilding(conoid, tab, DEPENDENCY_AUTO);
}
else
{
/* OK, capture the index's existing definition string */
char *defstring = pg_get_indexdef_string(indoid);
tab->changedIndexOids = lappend_oid(tab->changedIndexOids,
indoid);
tab->changedIndexDefs = lappend(tab->changedIndexDefs,
defstring);
}
}
}
/*
* Returns the address of the modified column
*/

22
src/test/regress/expected/alter_table.out
View File

@ -1934,12 +1934,19 @@ select * from anothertab;
(3 rows)
drop table anothertab;
-- Test alter table column type with constraint indexes (cf. bug #15835)
create table anothertab(f1 int primary key, f2 int unique, f3 int, f4 int);
-- Test index handling in alter table column type (cf. bugs #15835, #15865)
create table anothertab(f1 int primary key, f2 int unique,
f3 int, f4 int, f5 int);
alter table anothertab
add exclude using btree (f3 with =);
alter table anothertab
add exclude using btree (f4 with =) where (f4 is not null);
alter table anothertab
add exclude using btree (f4 with =) where (f5 > 0);
alter table anothertab
add unique(f1,f4);
create index on anothertab(f2,f3);
create unique index on anothertab(f4);
\d anothertab
Table "public.anothertab"
Column | Type | Collation | Nullable | Default
@ -1948,17 +1955,23 @@ alter table anothertab
f2 | integer | | |
f3 | integer | | |
f4 | integer | | |
f5 | integer | | |
Indexes:
"anothertab_pkey" PRIMARY KEY, btree (f1)
"anothertab_f1_f4_key" UNIQUE CONSTRAINT, btree (f1, f4)
"anothertab_f2_key" UNIQUE CONSTRAINT, btree (f2)
"anothertab_f4_idx" UNIQUE, btree (f4)
"anothertab_f2_f3_idx" btree (f2, f3)
"anothertab_f3_excl" EXCLUDE USING btree (f3 WITH =)
"anothertab_f4_excl" EXCLUDE USING btree (f4 WITH =) WHERE (f4 IS NOT NULL)
"anothertab_f4_excl1" EXCLUDE USING btree (f4 WITH =) WHERE (f5 > 0)
alter table anothertab alter column f1 type bigint;
alter table anothertab
alter column f2 type bigint,
alter column f3 type bigint,
alter column f4 type bigint;
alter table anothertab alter column f5 type bigint;
\d anothertab
Table "public.anothertab"
Column | Type | Collation | Nullable | Default
@ -1967,11 +1980,16 @@ alter table anothertab
f2 | bigint | | |
f3 | bigint | | |
f4 | bigint | | |
f5 | bigint | | |
Indexes:
"anothertab_pkey" PRIMARY KEY, btree (f1)
"anothertab_f1_f4_key" UNIQUE CONSTRAINT, btree (f1, f4)
"anothertab_f2_key" UNIQUE CONSTRAINT, btree (f2)
"anothertab_f4_idx" UNIQUE, btree (f4)
"anothertab_f2_f3_idx" btree (f2, f3)
"anothertab_f3_excl" EXCLUDE USING btree (f3 WITH =)
"anothertab_f4_excl" EXCLUDE USING btree (f4 WITH =) WHERE (f4 IS NOT NULL)
"anothertab_f4_excl1" EXCLUDE USING btree (f4 WITH =) WHERE (f5 > 0)
drop table anothertab;
create table another (f1 int, f2 text);

12
src/test/regress/sql/alter_table.sql
View File

@ -1307,12 +1307,19 @@ select * from anothertab;
drop table anothertab;
-- Test alter table column type with constraint indexes (cf. bug #15835)
create table anothertab(f1 int primary key, f2 int unique, f3 int, f4 int);
-- Test index handling in alter table column type (cf. bugs #15835, #15865)
create table anothertab(f1 int primary key, f2 int unique,
f3 int, f4 int, f5 int);
alter table anothertab
add exclude using btree (f3 with =);
alter table anothertab
add exclude using btree (f4 with =) where (f4 is not null);
alter table anothertab
add exclude using btree (f4 with =) where (f5 > 0);
alter table anothertab
add unique(f1,f4);
create index on anothertab(f2,f3);
create unique index on anothertab(f4);
\d anothertab
alter table anothertab alter column f1 type bigint;
@ -1320,6 +1327,7 @@ alter table anothertab
alter column f2 type bigint,
alter column f3 type bigint,
alter column f4 type bigint;
alter table anothertab alter column f5 type bigint;
\d anothertab
drop table anothertab;