8000 Avoid corrupting tables when ANALYZE inside a transaction is rolled b… · leelingco/postgres@6ec1c3e · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit 6ec1c3e

Browse files
tglsfdctglsfdc
committed
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
1 parent 8f7bd8e commit 6ec1c3e

File tree

3 files changed

+82
-37
lines changed

3 files changed

+82
-37
lines changed

‎src/backend/commands/vacuum.c

Lines changed: 54 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -538,23 +538,31 @@ vac_estimate_reltuples(Relation relation, bool is_analyze,
538538
*
539539
* We violate transaction semantics here by overwriting the rel's
540540
* existing pg_class tuple with the new values. This is reasonably
541-
* safe since the new values are correct whether or not this transaction
542-
* commits. The reason for this is that if we updated these tuples in
543-
* the usual way, vacuuming pg_class itself wouldn't work very well ---
544-
* by the time we got done with a vacuum cycle, most of the tuples in
545-
* pg_class would've been obsoleted. Of course, this only works for
546-
* fixed-size never-null columns, but these are.
547-
*
548-
* Note another assumption: that two VACUUMs/ANALYZEs on a table can't
549-
* run in parallel, nor can VACUUM/ANALYZE run in parallel with a
550-
* schema alteration such as adding an index, rule, or trigger. Otherwise
551-
* our updates of relhasindex etc might overwrite uncommitted updates.
541+
* safe as long as we're sure that the new values are correct whether or
542+
* not this transaction commits. The reason for doing this is that if
543+
* we updated these tuples in the usual way, vacuuming pg_class itself
544+
* wouldn't work very well --- by the time we got done with a vacuum
545+
* cycle, most of the tuples in pg_class would've been obsoleted. Of
546+
* course, this only works for fixed-size not-null columns, but these are.
552547
*
553548
* Another reason for doing it this way is that when we are in a lazy
554-
* VACUUM and have PROC_IN_VACUUM set, we mustn't do any updates ---
555-
* somebody vacuuming pg_class might think they could delete a tuple
549+
* VACUUM and have PROC_IN_VACUUM set, we mustn't do any regular updates.
550+
* Somebody vacuuming pg_class might think they could delete a tuple
556551
* marked with xmin = our xid.
557552
*
553+
* In addition to fundamentally nontransactional statistics such as
554+
* relpages and relallvisible, we try to maintain certain lazily-updated
555+
* DDL flags such as relhasindex, by clearing them if no longer correct.
556+
* It's safe to do this in VACUUM, which can't run in parallel with
557+
* CREATE INDEX/RULE/TRIGGER and can't be part of a transaction block.
558+
* However, it's *not* safe to do it in an ANALYZE that's within a
559+
* transaction block, because for example the current transaction might
560+
* have dropped the last index; then we'd think relhasindex should be
561+
* cleared, but if the transaction later rolls back this would be wrong.
562+
* So we refrain from updating the DDL flags if we're inside a
563+
* transaction block. This is OK since postponing the flag maintenance
564+
* is always allowable.
565+
*
558566
* This routine is shared by VACUUM and ANALYZE.
559567
*/
560568
void
@@ -577,7 +585,7 @@ vac_update_relstats(Relation relation,
577585
relid);
578586
pgcform = (Form_pg_class) GETSTRUCT(ctup);
579587

580-
/* Apply required updates, if any, to copied tuple */
588+
/* Apply statistical updates, if any, to copied tuple */
581589

582590
dirty = false;
583591
if (pgcform->relpages != (int32) num_pages)
@@ -590,32 +598,41 @@ vac_update_relstats(Relation relation,
590598
pgcform->reltuples = (float4) num_tuples;
591599
dirty = true;
592600
}
593-
if (pgcform->relhasindex != hasindex)
594-
{
595-
pgcform->relhasindex = hasindex;
596-
dirty = true;
597-
}
598601

599-
/*
600-
* If we have discovered that there are no indexes, then there's no
601-
* primary key either. This could be done more thoroughly...
602-
*/
603-
if (pgcform->relhaspkey && !hasindex)
604-
{
605-
pgcform->relhaspkey = false;
606-
dirty = true;
607-
}
602+
/* Apply DDL updates, but not inside a transaction block (see above) */
608603

609-
/* We also clear relhasrules and relhastriggers if needed */
610-
if (pgcform->relhasrules && relation->rd_rules == NULL)
604+
if (!IsTransactionBlock())
611605
{
612-
pgcform->relhasrules = false;
613-
dirty = true;
614-
}
615-
if (pgcform->relhastriggers && relation->trigdesc == NULL)
616-
{
617-
pgcform->relhastriggers = false;
618-
dirty = true;
606+
/*
607+
* If we didn't find any indexes, reset relhasindex.
608+
*/
609+
if (pgcform->relhasindex && !hasindex)
610+
{
611+
pgcform->relhasindex = false;
612+
dirty = true;
613+
}
614+
615+
/*
616+
* If we have discovered that there are no indexes, then there's no
617+
* primary key either. This could be done more thoroughly...
618+
*/
619+
if (pgcform->relhaspkey && !hasindex)
620+
{
621+
pgcform->relhaspkey = false;
622+
dirty = true;
623+
}
624+
625+
/* We also clear relhasrules and relhastriggers if needed */
626+
if (pgcform->relhasrules && relation->rd_rules == NULL)
627+
{
628+
pgcform->relhasrules = false;
629+
dirty = true;
630+
}
631+
if (pgcform->relhastriggers && relation->trigdesc == NULL)
632+
{
633+
pgcform->relhastriggers = false;
634+
dirty = true;
635+
}
619636
}
620637

621638
/*

‎src/test/regress/expected/alter_table.out

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1559,6 +1559,24 @@ Check constraints:
15591559
"test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
15601560
Inherits: test_inh_check
15611561

1562+
-- check for rollback of ANALYZE corrupting table property flags (bug #11638)
1563+
CREATE TABLE check_fk_presence_1 (id int PRIMARY KEY, t text);
1564+
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "check_fk_presence_1_pkey" for table "check_fk_presence_1"
1565+
CREATE TABLE check_fk_presence_2 (id int REFERENCES check_fk_presence_1, t text);
1566+
BEGIN;
1567+
ALTER TABLE check_fk_presence_2 DROP CONSTRAINT check_fk_presence_2_id_fkey;
1568+
ANALYZE check_fk_presence_2;
1569+
ROLLBACK;
1570+
\d check_fk_presence_2
1571+
Table "public.check_fk_presence_2"
1572+
Column | Type | Modifiers
1573+
--------+---------+-----------
1574+
id | integer |
1575+
t | text |
1576+
Foreign-key constraints:
1577+
"check_fk_presence_2_id_fkey" FOREIGN KEY (id) REFERENCES check_fk_presence_1(id)
1578+
1579+
DROP TABLE check_fk_presence_1, check_fk_presence_2;
15621580
--
15631581
-- lock levels
15641582
--

‎src/test/regress/sql/alter_table.sql

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1151,6 +1151,16 @@ ALTER TABLE test_inh_check ALTER COLUMN a TYPE numeric;
11511151
\d test_inh_check
11521152
\d test_inh_check_child
11531153

1154+
-- check for rollback of ANALYZE corrupting table property flags (bug #11638)
1155+
CREATE TABLE check_fk_presence_1 (id int PRIMARY KEY, t text);
1156+
CREATE TABLE check_fk_presence_2 (id int REFERENCES check_fk_presence_1, t text);
1157+
BEGIN;
1158+
ALTER TABLE check_fk_presence_2 DROP CONSTRAINT check_fk_presence_2_id_fkey;
1159+
ANALYZE check_fk_presence_2;
1160+
ROLLBACK;
1161+
\d check_fk_presence_2
1162+
DROP TABLE check_fk_presence_1, check_fk_presence_2;
1163+
11541164
--
11551165
-- lock levels
11561166
--

0 commit comments

Comments
 (0)
0