Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit a63a7a5

Browse files
committed
Avoid crashing when we have problems unlinking files post-commit.
smgrdounlink takes care to not throw an ERROR if it fails to unlink something, but that caution was rendered useless by commit 3396000, which put an smgrexists call in front of it; smgrexists *does* throw error if anything looks funny, such as getting a permissions error from trying to open the file. If that happens post-commit, you get a PANIC, and what's worse the same logic appears in the WAL replay code, so the database even fails to restart. Restore the intended behavior by removing the smgrexists call --- it isn't accomplishing anything that we can't do better by adjusting mdunlink's ideas of whether it ought to warn about ENOENT or not. Per report from Joseph Shraibman of unrecoverable crash after trying to drop a table whose FSM fork had somehow gotten chmod'd to 000 permissions. Backpatch to 8.4, where the bogus coding was introduced.
1 parent bb4cfeb commit a63a7a5

File tree

4 files changed

+20
-26
lines changed

4 files changed

+20
-26
lines changed

src/backend/access/transam/twophase.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1343,8 +1343,7 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
13431343

13441344
for (fork = 0; fork <= MAX_FORKNUM; fork++)
13451345
{
1346-
if (smgrexists(srel, fork))
1347-
smgrdounlink(srel, fork, false);
1346+
smgrdounlink(srel, fork, false);
13481347
}
13491348
smgrclose(srel);
13501349
}

src/backend/access/transam/xact.c

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4532,11 +4532,8 @@ xact_redo_commit(xl_xact_commit *xlrec, TransactionId xid, XLogRecPtr lsn)
45324532

45334533
for (fork = 0; fork <= MAX_FORKNUM; fork++)
45344534
{
4535-
if (smgrexists(srel, fork))
4536-
{
4537-
XLogDropRelation(xlrec->xnodes[i], fork);
4538-
smgrdounlink(srel, fork, true);
4539-
}
4535+
XLogDropRelation(xlrec->xnodes[i], fork);
4536+
smgrdounlink(srel, fork, true);
45404537
}
45414538
smgrclose(srel);
45424539
}
@@ -4637,11 +4634,8 @@ xact_redo_abort(xl_xact_abort *xlrec, TransactionId xid)
46374634

46384635
for (fork = 0; fork <= MAX_FORKNUM; fork++)
46394636
{
4640-
if (smgrexists(srel, fork))
4641-
{
4642-
XLogDropRelation(xlrec->xnodes[i], fork);
4643-
smgrdounlink(srel, fork, true);
4644-
}
4637+
XLogDropRelation(xlrec->xnodes[i], fork);
4638+
smgrdounlink(srel, fork, true);
46454639
}
46464640
smgrclose(srel);
46474641
}

src/backend/catalog/storage.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -360,8 +360,7 @@ smgrDoPendingDeletes(bool isCommit)
360360
srel = smgropen(pending->relnode, pending->backend);
361361
for (i = 0; i <= MAX_FORKNUM; i++)
362362
{
363-
if (smgrexists(srel, i))
364-
smgrdounlink(srel, i, false);
363+
smgrdounlink(srel, i, false);
365364
}
366365
smgrclose(srel);
367366
}

src/backend/storage/smgr/md.c

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,13 @@ mdcreate(SMgrRelation reln, ForkNumber forkNum, bool isRedo)
322322
* number until it's safe, because relfilenode assignment skips over any
323323
* existing file.
324324
*
325-
* If isRedo is true, it's okay for the relation to be already gone.
325+
* All the above applies only to the relation's main fork; other forks can
326+
* just be removed immediately, since they are not needed to prevent the
327+
* relfilenode number from being recycled. Also, we do not carefully
328+
* track whether other forks have been created or not, but just attempt to
329+
* unlink them unconditionally; so we should never complain about ENOENT.
330+
*
331+
* If isRedo is true, it's unsurprising for the relation to be already gone.
326332
* Also, we should remove the file immediately instead of queuing a request
327333
* for later, since during redo there's no possibility of creating a
328334
* conflicting relation.
@@ -350,13 +356,10 @@ mdunlink(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
350356
if (isRedo || forkNum != MAIN_FORKNUM)
351357
{
352358
ret = unlink(path);
353-
if (ret < 0)
354-
{
355-
if (!isRedo || errno != ENOENT)
356-
ereport(WARNING,
357-
(errcode_for_file_access(),
358-
errmsg("could not remove file \"%s\": %m", path)));
359-
}
359+
if (ret < 0 && errno != ENOENT)
360+
ereport(WARNING,
361+
(errcode_for_file_access(),
362+
errmsg("could not remove file \"%s\": %m", path)));
360363
}
361364
else
362365
{
@@ -379,6 +382,9 @@ mdunlink(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
379382
ereport(WARNING,
380383
(errcode_for_file_access(),
381384
errmsg("could not truncate file \"%s\": %m", path)));
385+
386+
/* Register request to unlink first segment later */
387+
register_unlink(rnode);
382388
}
383389

384390
/*
@@ -410,10 +416,6 @@ mdunlink(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
410416
}
411417

412418
pfree(path);
413-
414-
/* Register request to unlink first segment later */
415-
if (!isRedo && forkNum == MAIN_FORKNUM)
416-
register_unlink(rnode);
417419
}
418420

419421
/*

0 commit comments

Comments
 (0)