From e0722d9cb57d06c2e459cf338cf7387ec3edb1f5 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 29 Oct 2014 18:12:02 -0400 Subject: [PATCH] Avoid corrupting tables when ANALYZE inside a transaction is rolled back. VACUUM and ANALYZE update the target table's pg_class row in-place, that is nontransactionally. This is OK, more or less, for the statistical columns, which are mostly nontransactional anyhow. It's not so OK for the DDL hint flags (relhasindex etc), which might get changed in response to transactional changes that could still be rolled back. This isn't a problem for VACUUM, since it can't be run inside a transaction block nor in parallel with DDL on the table. However, we allow ANALYZE inside a transaction block, so if the transaction had earlier removed the last index, rule, or trigger from the table, and then we roll back the transaction after ANALYZE, the table would be left in a corrupted state with the hint flags not set though they should be. To fix, suppress the hint-flag updates if we are InTransactionBlock(). This is safe enough because it's always OK to postpone hint maintenance some more; the worst-case consequence is a few extra searches of pg_index et al. There was discussion of instead using a transactional update, but that would change the behavior in ways that are not all desirable: in most scenarios we're better off keeping ANALYZE's statistical values even if the ANALYZE itself rolls back. In any case we probably don't want to change this behavior in back branches. Per bug #11638 from Casey Shobe. This has been broken for a good long time, so back-patch to all supported branches. Tom Lane and Michael Paquier, initial diagnosis by Andres Freund --- src/backend/commands/vacuum.c | 91 ++++++++++++++--------- src/test/regress/expected/alter_table.out | 17 +++++ src/test/regress/sql/alter_table.sql | 10 +++ 3 files changed, 81 insertions(+), 37 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index e5fefa35a4..0dc92bab9b 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -648,23 +648,31 @@ vac_estimate_reltuples(Relation relation, bool is_analyze, * * We violate transaction semantics here by overwriting the rel's * existing pg_class tuple with the new values. This is reasonably - * safe since the new values are correct whether or not this transaction - * commits. The reason for this is that if we updated these tuples in - * the usual way, vacuuming pg_class itself wouldn't work very well --- - * by the time we got done with a vacuum cycle, most of the tuples in - * pg_class would've been obsoleted. Of course, this only works for - * fixed-size never-null columns, but these are. - * - * Note another assumption: that two VACUUMs/ANALYZEs on a table can't - * run in parallel, nor can VACUUM/ANALYZE run in parallel with a - * schema alteration such as adding an index, rule, or trigger. Otherwise - * our updates of relhasindex etc might overwrite uncommitted updates. + * safe as long as we're sure that the new values are correct whether or + * not this transaction commits. The reason for doing this is that if + * we updated these tuples in the usual way, vacuuming pg_class itself + * wouldn't work very well --- by the time we got done with a vacuum + * cycle, most of the tuples in pg_class would've been obsoleted. Of + * course, this only works for fixed-size not-null columns, but these are. * * Another reason for doing it this way is that when we are in a lazy - * VACUUM and have PROC_IN_VACUUM set, we mustn't do any updates --- - * somebody vacuuming pg_class might think they could delete a tuple + * VACUUM and have PROC_IN_VACUUM set, we mustn't do any regular updates. + * Somebody vacuuming pg_class might think they could delete a tuple * marked with xmin = our xid. * + * In addition to fundamentally nontransactional statistics such as + * relpages and relallvisible, we try to maintain certain lazily-updated + * DDL flags such as relhasindex, by clearing them if no longer correct. + * It's safe to do this in VACUUM, which can't run in parallel with + * CREATE INDEX/RULE/TRIGGER and can't be part of a transaction block. + * However, it's *not* safe to do it in an ANALYZE that's within a + * transaction block, because for example the current transaction might + * have dropped the last index; then we'd think relhasindex should be + * cleared, but if the transaction later rolls back this would be wrong. + * So we refrain from updating the DDL flags if we're inside a + * transaction block. This is OK since postponing the flag maintenance + * is always allowable. + * * This routine is shared by VACUUM and ANALYZE. */ void @@ -689,7 +697,7 @@ vac_update_relstats(Relation relation, relid); pgcform = (Form_pg_class) GETSTRUCT(ctup); - /* Apply required updates, if any, to copied tuple */ + /* Apply statistical updates, if any, to copied tuple */ dirty = false; if (pgcform->relpages != (int32) num_pages) @@ -707,32 +715,41 @@ vac_update_relstats(Relation relation, pgcform->relallvisible = (int32) num_all_visible_pages; dirty = true; } - if (pgcform->relhasindex != hasindex) - { - pgcform->relhasindex = hasindex; - dirty = true; - } - /* - * If we have discovered that there are no indexes, then there's no - * primary key either. This could be done more thoroughly... - */ - if (pgcform->relhaspkey && !hasindex) - { - pgcform->relhaspkey = false; - dirty = true; - } + /* Apply DDL updates, but not inside a transaction block (see above) */ - /* We also clear relhasrules and relhastriggers if needed */ - if (pgcform->relhasrules && relation->rd_rules == NULL) + if (!IsTransactionBlock()) { - pgcform->relhasrules = false; - dirty = true; - } - if (pgcform->relhastriggers && relation->trigdesc == NULL) - { - pgcform->relhastriggers = false; - dirty = true; + /* + * If we didn't find any indexes, reset relhasindex. + */ + if (pgcform->relhasindex && !hasindex) + { + pgcform->relhasindex = false; + dirty = true; + } + + /* + * If we have discovered that there are no indexes, then there's no + * primary key either. This could be done more thoroughly... + */ + if (pgcform->relhaspkey && !hasindex) + { + pgcform->relhaspkey = false; + dirty = true; + } + + /* We also clear relhasrules and relhastriggers if needed */ + if (pgcform->relhasrules && relation->rd_rules == NULL) + { + pgcform->relhasrules = false; + dirty = true; + } + if (pgcform->relhastriggers && relation->trigdesc == NULL) + { + pgcform->relhastriggers = false; + dirty = true; + } } /* diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 10f45f25a5..d233710871 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1811,6 +1811,23 @@ Check constraints: "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision) Inherits: test_inh_check +-- check for rollback of ANALYZE corrupting table property flags (bug #11638) +CREATE TABLE check_fk_presence_1 (id int PRIMARY KEY, t text); +CREATE TABLE check_fk_presence_2 (id int REFERENCES check_fk_presence_1, t text); +BEGIN; +ALTER TABLE check_fk_presence_2 DROP CONSTRAINT check_fk_presence_2_id_fkey; +ANALYZE check_fk_presence_2; +ROLLBACK; +\d check_fk_presence_2 +Table "public.check_fk_presence_2" + Column | Type | Modifiers +--------+---------+----------- + id | integer | + t | text | +Foreign-key constraints: + "check_fk_presence_2_id_fkey" FOREIGN KEY (id) REFERENCES check_fk_presence_1(id) + +DROP TABLE check_fk_presence_1, check_fk_presence_2; -- -- lock levels -- diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 12fd7c2321..d0d29ee62b 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1254,6 +1254,16 @@ ALTER TABLE test_inh_check ALTER COLUMN a TYPE numeric; \d test_inh_check \d test_inh_check_child +-- check for rollback of ANALYZE corrupting table property flags (bug #11638) +CREATE TABLE check_fk_presence_1 (id int PRIMARY KEY, t text); +CREATE TABLE check_fk_presence_2 (id int REFERENCES check_fk_presence_1, t text); +BEGIN; +ALTER TABLE check_fk_presence_2 DROP CONSTRAINT check_fk_presence_2_id_fkey; +ANALYZE check_fk_presence_2; +ROLLBACK; +\d check_fk_presence_2 +DROP TABLE check_fk_presence_1, check_fk_presence_2; + -- -- lock levels -- -- 2.40.0