From d4791ac35cb1d7417ea3cff6cc604623463ef0ea Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 23 Mar 2021 11:24:16 -0400 Subject: [PATCH] Avoid possible crash while finishing up a heap rewrite. end_heap_rewrite was not careful to ensure that the target relation is open at the smgr level before performing its final smgrimmedsync. In ordinary cases this is no problem, because it would have been opened earlier during the rewrite. However a crash can be reproduced by re-clustering an empty table with CLOBBER_CACHE_ALWAYS enabled. Although that exact scenario does not crash in v13, I think that's a chance result of unrelated planner changes, and the problem is likely still reachable with other test cases. The true proximate cause of this failure is commit c6b92041d, which replaced a call to heap_sync (which was careful about opening smgr) with a direct call to smgrimmedsync. Hence, back-patch to v13. Amul Sul, per report from Neha Sharma; cosmetic changes and test case by me. Discussion: https://postgr.es/m/CANiYTQsU7yMFpQYnv=BrcRVqK_3U3mtAzAsJCaqtzsDHfsUbdQ@mail.gmail.com --- src/backend/access/heap/rewriteheap.c | 6 +++++- src/test/regress/expected/cluster.out | 5 +++++ src/test/regress/sql/cluster.sql | 6 ++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c index 39e33763df4..56c61234bd2 100644 --- a/src/backend/access/heap/rewriteheap.c +++ b/src/backend/access/heap/rewriteheap.c @@ -324,10 +324,10 @@ end_heap_rewrite(RewriteState state) state->rs_blockno, state->rs_buffer, true); - RelationOpenSmgr(state->rs_new_rel); PageSetChecksumInplace(state->rs_buffer, state->rs_blockno); + RelationOpenSmgr(state->rs_new_rel); smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, state->rs_blockno, (char *) state->rs_buffer, true); } @@ -340,7 +340,11 @@ end_heap_rewrite(RewriteState state) * wrote before the checkpoint. */ if (RelationNeedsWAL(state->rs_new_rel)) + { + /* for an empty table, this could be first smgr access */ + RelationOpenSmgr(state->rs_new_rel); smgrimmedsync(state->rs_new_rel->rd_smgr, MAIN_FORKNUM); + } logical_end_heap_rewrite(state); diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index bdae8fe00cd..e46a66952f0 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -439,6 +439,11 @@ select * from clstr_temp; drop table clstr_temp; RESET SESSION AUTHORIZATION; +-- check clustering an empty table +DROP TABLE clustertest; +CREATE TABLE clustertest (f1 int PRIMARY KEY); +CLUSTER clustertest USING clustertest_pkey; +CLUSTER clustertest; -- Check that partitioned tables cannot be clustered CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a); CREATE INDEX clstrpart_idx ON clstrpart (a); diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql index 188183647c9..aee9cf83e04 100644 --- a/src/test/regress/sql/cluster.sql +++ b/src/test/regress/sql/cluster.sql @@ -196,6 +196,12 @@ drop table clstr_temp; RESET SESSION AUTHORIZATION; +-- check clustering an empty table +DROP TABLE clustertest; +CREATE TABLE clustertest (f1 int PRIMARY KEY); +CLUSTER clustertest USING clustertest_pkey; +CLUSTER clustertest; + -- Check that partitioned tables cannot be clustered CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a); CREATE INDEX clstrpart_idx ON clstrpart (a);