Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitf97de05

Browse files
committed
Fix sloppy handling of corner-case errors in fd.c.
Several places in fd.c had badly-thought-through handling of error returnsfrom lseek() and close(). The fact that those would seldom fail on validFDs is probably the reason we've not noticed this up to now; but if theydid fail, we'd get quite confused.LruDelete and LruInsert actually just Assert'd that lseek never fails,which is pretty awful on its face.In LruDelete, we indeed can't throw an error, because that's likely to getcalled during error abort and so throwing an error would probably just leadto an infinite loop. But by the same token, throwing an error from theclose() right after that was ill-advised, not to mention that it would'veleft the LRU state corrupted since we'd already unlinked the VFD from thelist. I also noticed that really, most of the time, we should know thecurrent seek position and it shouldn't be necessary to do an lseek here atall. As patched, if we don't have a seek position and an lseek attemptdoesn't give us one, we'll close the file but then subsequent re-openattempts will fail (except in the somewhat-unlikely case that aFileSeek(SEEK_SET) call comes between and allows us to re-establish a knowntarget seek position). This isn't great but it won't result in any statecorruption.Meanwhile, having an Assert instead of an honest test in LruInsert isreally dangerous: if that lseek failed, a subsequent read or write wouldread or write from the start of the file, not where the caller expected,leading to data corruption.In both LruDelete and FileClose, if close() fails, just LOG that and markthe VFD closed anyway. Possibly leaking an FD is preferable to gettinginto an infinite loop or corrupting the VFD list. Besides, as far as I cantell from the POSIX spec, it's unspecified whether or not the file has beenclosed, so treating it as still open could be the wrong thing anyhow.I also fixed a number of other places that were being sloppy aboutbehaving correctly when the seekPos is unknown.Also, I changed FileSeek to return -1 with EINVAL for the cases where itdetects a bad offset, rather than throwing a hard elog(ERROR). It seemedpretty inconsistent that some bad-offset cases would get a failure returnwhile others got elog(ERROR). It was missing an offset validity check forthe SEEK_CUR case on a closed file, too.Back-patch to all supported branches, since all this code is fundamentallyidentical in all of them.Discussion:https://postgr.es/m/2982.1487617365@sss.pgh.pa.us
1 parent3082098 commitf97de05

File tree

1 file changed

+136
-59
lines changed
  • src/backend/storage/file

1 file changed

+136
-59
lines changed

‎src/backend/storage/file/fd.c

Lines changed: 136 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,14 @@ intmax_safe_fds = 32;/* default if not changed */
160160

161161
#defineFileIsNotOpen(file) (VfdCache[file].fd == VFD_CLOSED)
162162

163+
/*
164+
* Note: a VFD's seekPos is normally always valid, but if for some reason
165+
* an lseek() fails, it might become set to FileUnknownPos. We can struggle
166+
* along without knowing the seek position in many cases, but in some places
167+
* we have to fail if we don't have it.
168+
*/
163169
#defineFileUnknownPos ((off_t) -1)
170+
#defineFilePosIsUnknown(pos) ((pos) < 0)
164171

165172
/* these are the assigned bits in fdstate below: */
166173
#defineFD_TEMPORARY(1 << 0)/* T = delete when closed */
@@ -174,7 +181,7 @@ typedef struct vfd
174181
FilenextFree;/* link to next free VFD, if in freelist */
175182
FilelruMoreRecently;/* doubly linked recency-of-use list */
176183
FilelruLessRecently;
177-
off_tseekPos;/* current logical file position */
184+
off_tseekPos;/* current logical file position, or -1 */
178185
off_tfileSize;/* current size of file (0 if not temporary) */
179186
char*fileName;/* name of file, or NULL for unused VFD */
180187
/* NB: fileName is malloc'd, and must be free'd when closing the VFD */
@@ -967,19 +974,33 @@ LruDelete(File file)
967974

968975
vfdP=&VfdCache[file];
969976

970-
/* delete the vfd record from the LRU ring */
971-
Delete(file);
972-
973-
/* save the seek position */
974-
vfdP->seekPos=lseek(vfdP->fd, (off_t)0,SEEK_CUR);
975-
Assert(vfdP->seekPos!= (off_t)-1);
977+
/*
978+
* Normally we should know the seek position, but if for some reason we
979+
* have lost track of it, try again to get it. If we still can't get it,
980+
* we have a problem: we will be unable to restore the file seek position
981+
* when and if the file is re-opened. But we can't really throw an error
982+
* and refuse to close the file, or activities such as transaction cleanup
983+
* will be broken.
984+
*/
985+
if (FilePosIsUnknown(vfdP->seekPos))
986+
{
987+
vfdP->seekPos=lseek(vfdP->fd, (off_t)0,SEEK_CUR);
988+
if (FilePosIsUnknown(vfdP->seekPos))
989+
elog(LOG,"could not seek file \"%s\" before closing: %m",
990+
vfdP->fileName);
991+
}
976992

977-
/* close the file */
993+
/*
994+
* Close the file. We aren't expecting this to fail; if it does, better
995+
* to leak the FD than to mess up our internal state.
996+
*/
978997
if (close(vfdP->fd))
979-
elog(ERROR,"could not close file \"%s\": %m",vfdP->fileName);
980-
981-
--nfile;
998+
elog(LOG,"could not close file \"%s\": %m",vfdP->fileName);
982999
vfdP->fd=VFD_CLOSED;
1000+
--nfile;
1001+
1002+
/* delete the vfd record from the LRU ring */
1003+
Delete(file);
9831004
}
9841005

9851006
staticvoid
@@ -1030,22 +1051,39 @@ LruInsert(File file)
10301051
vfdP->fileMode);
10311052
if (vfdP->fd<0)
10321053
{
1033-
DO_DB(elog(LOG,"RE_OPEN FAILED: %d",errno));
1054+
DO_DB(elog(LOG,"re-open failed: %m"));
10341055
return-1;
10351056
}
10361057
else
10371058
{
1038-
DO_DB(elog(LOG,"RE_OPEN SUCCESS"));
10391059
++nfile;
10401060
}
10411061

1042-
/* seek to the right position */
1062+
/*
1063+
* Seek to the right position. We need no special case for seekPos
1064+
* equal to FileUnknownPos, as lseek() will certainly reject that
1065+
* (thus completing the logic noted in LruDelete() that we will fail
1066+
* to re-open a file if we couldn't get its seek position before
1067+
* closing).
1068+
*/
10431069
if (vfdP->seekPos!= (off_t)0)
10441070
{
1045-
off_treturnValuePG_USED_FOR_ASSERTS_ONLY;
1046-
1047-
returnValue=lseek(vfdP->fd,vfdP->seekPos,SEEK_SET);
1048-
Assert(returnValue!= (off_t)-1);
1071+
if (lseek(vfdP->fd,vfdP->seekPos,SEEK_SET)<0)
1072+
{
1073+
/*
1074+
* If we fail to restore the seek position, treat it like an
1075+
* open() failure.
1076+
*/
1077+
intsave_errno=errno;
1078+
1079+
elog(LOG,"could not seek file \"%s\" after re-opening: %m",
1080+
vfdP->fileName);
1081+
(void)close(vfdP->fd);
1082+
vfdP->fd=VFD_CLOSED;
1083+
--nfile;
1084+
errno=save_errno;
1085+
return-1;
1086+
}
10491087
}
10501088
}
10511089

@@ -1428,15 +1466,15 @@ FileClose(File file)
14281466

14291467
if (!FileIsNotOpen(file))
14301468
{
1431-
/* remove the file from the lru ring */
1432-
Delete(file);
1433-
14341469
/* close the file */
14351470
if (close(vfdP->fd))
1436-
elog(ERROR,"could not close file \"%s\": %m",vfdP->fileName);
1471+
elog(LOG,"could not close file \"%s\": %m",vfdP->fileName);
14371472

14381473
--nfile;
14391474
vfdP->fd=VFD_CLOSED;
1475+
1476+
/* remove the file from the lru ring */
1477+
Delete(file);
14401478
}
14411479

14421480
/*
@@ -1566,6 +1604,7 @@ int
15661604
FileRead(Filefile,char*buffer,intamount)
15671605
{
15681606
intreturnCode;
1607+
Vfd*vfdP;
15691608

15701609
Assert(FileIsValid(file));
15711610

@@ -1578,11 +1617,17 @@ FileRead(File file, char *buffer, int amount)
15781617
if (returnCode<0)
15791618
returnreturnCode;
15801619

1620+
vfdP=&VfdCache[file];
1621+
15811622
retry:
1582-
returnCode=read(VfdCache[file].fd,buffer,amount);
1623+
returnCode=read(vfdP->fd,buffer,amount);
15831624

15841625
if (returnCode >=0)
1585-
VfdCache[file].seekPos+=returnCode;
1626+
{
1627+
/* if seekPos is unknown, leave it that way */
1628+
if (!FilePosIsUnknown(vfdP->seekPos))
1629+
vfdP->seekPos+=returnCode;
1630+
}
15861631
else
15871632
{
15881633
/*
@@ -1611,7 +1656,7 @@ FileRead(File file, char *buffer, int amount)
16111656
gotoretry;
16121657

16131658
/* Trouble, so assume we don't know the file position anymore */
1614-
VfdCache[file].seekPos=FileUnknownPos;
1659+
vfdP->seekPos=FileUnknownPos;
16151660
}
16161661

16171662
returnreturnCode;
@@ -1621,6 +1666,7 @@ int
16211666
FileWrite(Filefile,char*buffer,intamount)
16221667
{
16231668
intreturnCode;
1669+
Vfd*vfdP;
16241670

16251671
Assert(FileIsValid(file));
16261672

@@ -1633,6 +1679,8 @@ FileWrite(File file, char *buffer, int amount)
16331679
if (returnCode<0)
16341680
returnreturnCode;
16351681

1682+
vfdP=&VfdCache[file];
1683+
16361684
/*
16371685
* If enforcing temp_file_limit and it's a temp file, check to see if the
16381686
* write would overrun temp_file_limit, and throw error if so. Note: it's
@@ -1641,15 +1689,28 @@ FileWrite(File file, char *buffer, int amount)
16411689
* message if we do that. All current callers would just throw error
16421690
* immediately anyway, so this is safe at present.
16431691
*/
1644-
if (temp_file_limit >=0&& (VfdCache[file].fdstate&FD_TEMPORARY))
1692+
if (temp_file_limit >=0&& (vfdP->fdstate&FD_TEMPORARY))
16451693
{
1646-
off_tnewPos=VfdCache[file].seekPos+amount;
1694+
off_tnewPos;
16471695

1648-
if (newPos>VfdCache[file].fileSize)
1696+
/*
1697+
* Normally we should know the seek position, but if for some reason
1698+
* we have lost track of it, try again to get it. Here, it's fine to
1699+
* throw an error if we still can't get it.
1700+
*/
1701+
if (FilePosIsUnknown(vfdP->seekPos))
1702+
{
1703+
vfdP->seekPos=lseek(vfdP->fd, (off_t)0,SEEK_CUR);
1704+
if (FilePosIsUnknown(vfdP->seekPos))
1705+
elog(ERROR,"could not seek file \"%s\": %m",vfdP->fileName);
1706+
}
1707+
1708+
newPos=vfdP->seekPos+amount;
1709+
if (newPos>vfdP->fileSize)
16491710
{
16501711
uint64newTotal=temporary_files_size;
16511712

1652-
newTotal+=newPos-VfdCache[file].fileSize;
1713+
newTotal+=newPos-vfdP->fileSize;
16531714
if (newTotal> (uint64)temp_file_limit* (uint64)1024)
16541715
ereport(ERROR,
16551716
(errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),
@@ -1660,25 +1721,33 @@ FileWrite(File file, char *buffer, int amount)
16601721

16611722
retry:
16621723
errno=0;
1663-
returnCode=write(VfdCache[file].fd,buffer,amount);
1724+
returnCode=write(vfdP->fd,buffer,amount);
16641725

16651726
/* if write didn't set errno, assume problem is no disk space */
16661727
if (returnCode!=amount&&errno==0)
16671728
errno=ENOSPC;
16681729

16691730
if (returnCode >=0)
16701731
{
1671-
VfdCache[file].seekPos+=returnCode;
1732+
/* if seekPos is unknown, leave it that way */
1733+
if (!FilePosIsUnknown(vfdP->seekPos))
1734+
vfdP->seekPos+=returnCode;
16721735

1673-
/* maintain fileSize and temporary_files_size if it's a temp file */
1674-
if (VfdCache[file].fdstate&FD_TEMPORARY)
1736+
/*
1737+
* Maintain fileSize and temporary_files_size if it's a temp file.
1738+
*
1739+
* If seekPos is -1 (unknown), this will do nothing; but we could only
1740+
* get here in that state if we're not enforcing temporary_files_size,
1741+
* so we don't care.
1742+
*/
1743+
if (vfdP->fdstate&FD_TEMPORARY)
16751744
{
1676-
off_tnewPos=VfdCache[file].seekPos;
1745+
off_tnewPos=vfdP->seekPos;
16771746

1678-
if (newPos>VfdCache[file].fileSize)
1747+
if (newPos>vfdP->fileSize)
16791748
{
1680-
temporary_files_size+=newPos-VfdCache[file].fileSize;
1681-
VfdCache[file].fileSize=newPos;
1749+
temporary_files_size+=newPos-vfdP->fileSize;
1750+
vfdP->fileSize=newPos;
16821751
}
16831752
}
16841753
}
@@ -1706,7 +1775,7 @@ FileWrite(File file, char *buffer, int amount)
17061775
gotoretry;
17071776

17081777
/* Trouble, so assume we don't know the file position anymore */
1709-
VfdCache[file].seekPos=FileUnknownPos;
1778+
vfdP->seekPos=FileUnknownPos;
17101779
}
17111780

17121781
returnreturnCode;
@@ -1732,7 +1801,7 @@ FileSync(File file)
17321801
off_t
17331802
FileSeek(Filefile,off_toffset,intwhence)
17341803
{
1735-
intreturnCode;
1804+
Vfd*vfdP;
17361805

17371806
Assert(FileIsValid(file));
17381807

@@ -1741,25 +1810,33 @@ FileSeek(File file, off_t offset, int whence)
17411810
(int64)VfdCache[file].seekPos,
17421811
(int64)offset,whence));
17431812

1813+
vfdP=&VfdCache[file];
1814+
17441815
if (FileIsNotOpen(file))
17451816
{
17461817
switch (whence)
17471818
{
17481819
caseSEEK_SET:
17491820
if (offset<0)
1750-
elog(ERROR,"invalid seek offset: "INT64_FORMAT,
1751-
(int64)offset);
1752-
VfdCache[file].seekPos=offset;
1821+
{
1822+
errno=EINVAL;
1823+
return (off_t)-1;
1824+
}
1825+
vfdP->seekPos=offset;
17531826
break;
17541827
caseSEEK_CUR:
1755-
VfdCache[file].seekPos+=offset;
1828+
if (FilePosIsUnknown(vfdP->seekPos)||
1829+
vfdP->seekPos+offset<0)
1830+
{
1831+
errno=EINVAL;
1832+
return (off_t)-1;
1833+
}
1834+
vfdP->seekPos+=offset;
17561835
break;
17571836
caseSEEK_END:
1758-
returnCode=FileAccess(file);
1759-
if (returnCode<0)
1760-
returnreturnCode;
1761-
VfdCache[file].seekPos=lseek(VfdCache[file].fd,
1762-
offset,whence);
1837+
if (FileAccess(file)<0)
1838+
return (off_t)-1;
1839+
vfdP->seekPos=lseek(vfdP->fd,offset,whence);
17631840
break;
17641841
default:
17651842
elog(ERROR,"invalid whence: %d",whence);
@@ -1772,27 +1849,27 @@ FileSeek(File file, off_t offset, int whence)
17721849
{
17731850
caseSEEK_SET:
17741851
if (offset<0)
1775-
elog(ERROR,"invalid seek offset: "INT64_FORMAT,
1776-
(int64)offset);
1777-
if (VfdCache[file].seekPos!=offset)
1778-
VfdCache[file].seekPos=lseek(VfdCache[file].fd,
1779-
offset,whence);
1852+
{
1853+
errno=EINVAL;
1854+
return (off_t)-1;
1855+
}
1856+
if (vfdP->seekPos!=offset)
1857+
vfdP->seekPos=lseek(vfdP->fd,offset,whence);
17801858
break;
17811859
caseSEEK_CUR:
1782-
if (offset!=0||VfdCache[file].seekPos==FileUnknownPos)
1783-
VfdCache[file].seekPos=lseek(VfdCache[file].fd,
1784-
offset,whence);
1860+
if (offset!=0||FilePosIsUnknown(vfdP->seekPos))
1861+
vfdP->seekPos=lseek(vfdP->fd,offset,whence);
17851862
break;
17861863
caseSEEK_END:
1787-
VfdCache[file].seekPos=lseek(VfdCache[file].fd,
1788-
offset,whence);
1864+
vfdP->seekPos=lseek(vfdP->fd,offset,whence);
17891865
break;
17901866
default:
17911867
elog(ERROR,"invalid whence: %d",whence);
17921868
break;
17931869
}
17941870
}
1795-
returnVfdCache[file].seekPos;
1871+
1872+
returnvfdP->seekPos;
17961873
}
17971874

17981875
/*

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp