Skip to content

Commit 2069e6f

Browse files
tglsfdctglsfdc
committed
Clean up assorted messiness around AllocateDir() usage.
This patch fixes a couple of low-probability bugs that could lead to reporting an irrelevant errno value (and hence possibly a wrong SQLSTATE) concerning directory-open or file-open failures. It also fixes places where we took shortcuts in reporting such errors, either by using elog instead of ereport or by using ereport but forgetting to specify an errcode. And it eliminates a lot of just plain redundant error-handling code. In service of all this, export fd.c's formerly-static function ReadDirExtended, so that external callers can make use of the coding pattern dir = AllocateDir(path); while ((de = ReadDirExtended(dir, path, LOG)) != NULL) if they'd like to treat directory-open failures as mere LOG conditions rather than errors. Also fix FreeDir to be a no-op if we reach it with dir == NULL, as such a coding pattern would cause. Then, remove code at many call sites that was throwing an error or log message for AllocateDir failure, as ReadDir or ReadDirExtended can handle that job just fine. Aside from being a net code savings, this gets rid of a lot of not-quite-up-to-snuff reports, as mentioned above. (In some places these changes result in replacing a custom error message such as "could not open tablespace directory" with more generic wording "could not open directory", but it was agreed that the custom wording buys little as long as we report the directory name.) In some other call sites where we can't just remove code, change the error reports to be fully project-style-compliant. Also reorder code in restoreTwoPhaseData that was acquiring a lock between AllocateDir and ReadDir; in the unlikely but surely not impossible case that LWLockAcquire changes errno, AllocateDir failures would be misreported. There is no great value in opening the directory before acquiring TwoPhaseStateLock, so just do it in the other order. Also fix CheckXLogRemoved to guarantee that it preserves errno, as quite a number of call sites are implicitly assuming. (Again, it's unlikely but I think not impossible that errno could change during a SpinLockAcquire. If so, this function was broken for its own purposes as well as breaking callers.) And change a few places that were using not-per-project-style messages, such as "could not read directory" when "could not open directory" is more correct. Back-patch the exporting of ReadDirExtended, in case we have occasion to back-patch some fix that makes use of it; it's not needed right now but surely making it global is pretty harmless. Also back-patch the restoreTwoPhaseData and CheckXLogRemoved fixes. The rest of this is essentially cosmetic and need not get back-patched. Michael Paquier, with a bit of additional work by me Discussion: https://postgr.es/m/CAB7nPqRpOCxjiirHmebEFhXVTK7V5Jvw4bz82p7Oimtsm3TyZA@mail.gmail.com
1 parent ab6eaee commit 2069e6f

File tree

17 files changed

+102
-153
lines changed

17 files changed

+102
-153
lines changed

‎contrib/adminpack/adminpack.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,7 @@ pg_logdir_ls(PG_FUNCTION_ARGS)
319319
if (!fctx->dirdesc)
320320
ereport(ERROR,
321321
(errcode_for_file_access(),
322-
errmsg("could not read directory \"%s\": %m",
322+
errmsg("could not open directory \"%s\": %m",
323323
fctx->location)));
324324

325325
funcctx->user_fctx = fctx;

‎src/backend/access/transam/twophase.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1735,8 +1735,8 @@ restoreTwoPhaseData(void)
17351735
DIR *cldir;
17361736
struct dirent *clde;
17371737

1738-
cldir = AllocateDir(TWOPHASE_DIR);
17391738
LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
1739+
cldir = AllocateDir(TWOPHASE_DIR);
17401740
while ((clde = ReadDir(cldir, TWOPHASE_DIR)) != NULL)
17411741
{
17421742
if (strlen(clde->d_name) == 8 &&

‎src/backend/access/transam/xlog.c

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3764,10 +3764,16 @@ PreallocXlogFiles(XLogRecPtr endptr)
37643764
* existed while the server has been running, as this function always
37653765
* succeeds if no WAL segments have been removed since startup.
37663766
* 'tli' is only used in the error message.
3767+
*
3768+
* Note: this function guarantees to keep errno unchanged on return.
3769+
* This supports callers that use this to possibly deliver a better
3770+
* error message about a missing file, while still being able to throw
3771+
* a normal file-access error afterwards, if this does return.
37673772
*/
37683773
void
37693774
CheckXLogRemoved(XLogSegNo segno, TimeLineID tli)
37703775
{
3776+
int save_errno = errno;
37713777
XLogSegNo lastRemovedSegNo;
37723778

37733779
SpinLockAcquire(&XLogCtl->info_lck);
@@ -3779,11 +3785,13 @@ CheckXLogRemoved(XLogSegNo segno, TimeLineID tli)
37793785
char filename[MAXFNAMELEN];
37803786

37813787
XLogFileName(filename, tli, segno, wal_segment_size);
3788+
errno = save_errno;
37823789
ereport(ERROR,
37833790
(errcode_for_file_access(),
37843791
errmsg("requested WAL segment %s has already been removed",
37853792
filename)));
37863793
}
3794+
errno = save_errno;
37873795
}
37883796

37893797
/*
@@ -3837,13 +3845,6 @@ RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr PriorRedoPtr, XLogRecPtr endptr)
38373845
struct dirent *xlde;
38383846
char lastoff[MAXFNAMELEN];
38393847

3840-
xldir = AllocateDir(XLOGDIR);
3841-
if (xldir == NULL)
3842-
ereport(ERROR,
3843-
(errcode_for_file_access(),
3844-
errmsg("could not open write-ahead log directory \"%s\": %m",
3845-
XLOGDIR)));
3846-
38473848
/*
38483849
* Construct a filename of the last segment to be kept. The timeline ID
38493850
* doesn't matter, we ignore that in the comparison. (During recovery,
@@ -3854,6 +3855,8 @@ RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr PriorRedoPtr, XLogRecPtr endptr)
38543855
elog(DEBUG2, "attempting to remove WAL segments older than log file %s",
38553856
lastoff);
38563857

3858+
xldir = AllocateDir(XLOGDIR);
3859+
38573860
while ((xlde = ReadDir(xldir, XLOGDIR)) != NULL)
38583861
{
38593862
/* Ignore files that are not XLOG segments */
@@ -3912,13 +3915,6 @@ RemoveNonParentXlogFiles(XLogRecPtr switchpoint, TimeLineID newTLI)
39123915

39133916
XLByteToPrevSeg(switchpoint, endLogSegNo, wal_segment_size);
39143917

3915-
xldir = AllocateDir(XLOGDIR);
3916-
if (xldir == NULL)
3917-
ereport(ERROR,
3918-
(errcode_for_file_access(),
3919-
errmsg("could not open write-ahead log directory \"%s\": %m",
3920-
XLOGDIR)));
3921-
39223918
/*
39233919
* Construct a filename of the last segment to be kept.
39243920
*/
@@ -3927,6 +3923,8 @@ RemoveNonParentXlogFiles(XLogRecPtr switchpoint, TimeLineID newTLI)
39273923
elog(DEBUG2, "attempting to remove WAL segments newer than log file %s",
39283924
switchseg);
39293925

3926+
xldir = AllocateDir(XLOGDIR);
3927+
39303928
while ((xlde = ReadDir(xldir, XLOGDIR)) != NULL)
39313929
{
39323930
/* Ignore files that are not XLOG segments */
@@ -4108,11 +4106,6 @@ CleanupBackupHistory(void)
41084106
char path[MAXPGPATH + sizeof(XLOGDIR)];
41094107

41104108
xldir = AllocateDir(XLOGDIR);
4111-
if (xldir == NULL)
4112-
ereport(ERROR,
4113-
(errcode_for_file_access(),
4114-
errmsg("could not open write-ahead log directory \"%s\": %m",
4115-
XLOGDIR)));
41164109

41174110
while ((xlde = ReadDir(xldir, XLOGDIR)) != NULL)
41184111
{

‎src/backend/access/transam/xlogfuncs.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,9 @@ pg_start_backup(PG_FUNCTION_ARGS)
8989
dir = AllocateDir("pg_tblspc");
9090
if (!dir)
9191
ereport(ERROR,
92-
(errmsg("could not open directory \"%s\": %m", "pg_tblspc")));
92+
(errcode_for_file_access(),
93+
errmsg("could not open directory \"%s\": %m",
94+
"pg_tblspc")));
9395

9496
if (exclusive)
9597
{

‎src/backend/postmaster/pgarch.c

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -673,11 +673,6 @@ pgarch_readyXlog(char *xlog)
673673

674674
snprintf(XLogArchiveStatusDir, MAXPGPATH, XLOGDIR "/archive_status");
675675
rldir = AllocateDir(XLogArchiveStatusDir);
676-
if (rldir == NULL)
677-
ereport(ERROR,
678-
(errcode_for_file_access(),
679-
errmsg("could not open archive status directory \"%s\": %m",
680-
XLogArchiveStatusDir)));
681676

682677
while ((rlde = ReadDir(rldir, XLogArchiveStatusDir)) != NULL)
683678
{

‎src/backend/replication/basebackup.c

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -367,9 +367,6 @@ perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
367367
XLogFileName(lastoff, ThisTimeLineID, endsegno, wal_segment_size);
368368

369369
dir = AllocateDir("pg_wal");
370-
if (!dir)
371-
ereport(ERROR,
372-
(errmsg("could not open directory \"%s\": %m", "pg_wal")));
373370
while ((de = ReadDir(dir, "pg_wal")) != NULL)
374371
{
375372
/* Does it look like a WAL segment, and is it in the range? */
@@ -713,7 +710,9 @@ SendBaseBackup(BaseBackupCmd *cmd)
713710
dir = AllocateDir("pg_tblspc");
714711
if (!dir)
715712
ereport(ERROR,
716-
(errmsg("could not open directory \"%s\": %m", "pg_tblspc")));
713+
(errcode_for_file_access(),
714+
errmsg("could not open directory \"%s\": %m",
715+
"pg_tblspc")));
717716

718717
perform_base_backup(&opt, dir);
719718

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

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,6 @@ copydir(char *fromdir, char *todir, bool recurse)
4747
errmsg("could not create directory \"%s\": %m", todir)));
4848

4949
xldir = AllocateDir(fromdir);
50-
if (xldir == NULL)
51-
ereport(ERROR,
52-
(errcode_for_file_access(),
53-
errmsg("could not open directory \"%s\": %m", fromdir)));
5450

5551
while ((xlde = ReadDir(xldir, fromdir)) != NULL)
5652
{
@@ -90,10 +86,6 @@ copydir(char *fromdir, char *todir, bool recurse)
9086
return;
9187

9288
xldir = AllocateDir(todir);
93-
if (xldir == NULL)
94-
ereport(ERROR,
95-
(errcode_for_file_access(),
96-
errmsg("could not open directory \"%s\": %m", todir)));
9789

9890
while ((xlde = ReadDir(xldir, todir)) != NULL)
9991
{

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

Lines changed: 37 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,6 @@ static int FileAccess(File file);
318318
static File OpenTemporaryFileInTablespace(Oid tblspcOid, bool rejectError);
319319
static bool reserveAllocatedDesc(void);
320320
static int FreeDesc(AllocateDesc *desc);
321-
static struct dirent *ReadDirExtended(DIR *dir, const char *dirname, int elevel);
322321

323322
static void AtProcExit_Files(int code, Datum arg);
324323
static void CleanupTempFiles(bool isProcExit);
@@ -2587,6 +2586,10 @@ CloseTransientFile(int fd)
25872586
* necessary to open the directory, and with closing it after an elog.
25882587
* When done, call FreeDir rather than closedir.
25892588
*
2589+
* Returns NULL, with errno set, on failure. Note that failure detection
2590+
* is commonly left to the following call of ReadDir or ReadDirExtended;
2591+
* see the comments for ReadDir.
2592+
*
25902593
* Ideally this should be the *only* direct call of opendir() in the backend.
25912594
*/
25922595
DIR *
@@ -2649,8 +2652,8 @@ AllocateDir(const char *dirname)
26492652
* FreeDir(dir);
26502653
*
26512654
* since a NULL dir parameter is taken as indicating AllocateDir failed.
2652-
* (Make sure errno hasn't been changed since AllocateDir if you use this
2653-
* shortcut.)
2655+
* (Make sure errno isn't changed between AllocateDir and ReadDir if you
2656+
* use this shortcut.)
26542657
*
26552658
* The pathname passed to AllocateDir must be passed to this routine too,
26562659
* but it is only used for error reporting.
@@ -2662,10 +2665,15 @@ ReadDir(DIR *dir, const char *dirname)
26622665
}
26632666

26642667
/*
2665-
* Alternate version that allows caller to specify the elevel for any
2666-
* error report. If elevel < ERROR, returns NULL on any error.
2668+
* Alternate version of ReadDir that allows caller to specify the elevel
2669+
* for any error report (whether it's reporting an initial failure of
2670+
* AllocateDir or a subsequent directory read failure).
2671+
*
2672+
* If elevel < ERROR, returns NULL after any error. With the normal coding
2673+
* pattern, this will result in falling out of the loop immediately as
2674+
* though the directory contained no (more) entries.
26672675
*/
2668-
static struct dirent *
2676+
struct dirent *
26692677
ReadDirExtended(DIR *dir, const char *dirname, int elevel)
26702678
{
26712679
struct dirent *dent;
@@ -2695,14 +2703,22 @@ ReadDirExtended(DIR *dir, const char *dirname, int elevel)
26952703
/*
26962704
* Close a directory opened with AllocateDir.
26972705
*
2698-
* Note we do not check closedir's return value --- it is up to the caller
2699-
* to handle close errors.
2706+
* Returns closedir's return value (with errno set if it's not 0).
2707+
* Note we do not check the return value --- it is up to the caller
2708+
* to handle close errors if wanted.
2709+
*
2710+
* Does nothing if dir == NULL; we assume that directory open failure was
2711+
* already reported if desired.
27002712
*/
27012713
int
27022714
FreeDir(DIR *dir)
27032715
{
27042716
int i;
27052717

2718+
/* Nothing to do if AllocateDir failed */
2719+
if (dir == NULL)
2720+
return 0;
2721+
27062722
DO_DB(elog(LOG, "FreeDir: Allocated %d", numAllocatedDescs));
27072723

27082724
/* Remove dir from list of allocated dirs, if it's present */
@@ -3043,9 +3059,10 @@ RemovePgTempFilesInDir(const char *tmpdirname, bool unlink_all)
30433059
{
30443060
/* anything except ENOENT is fishy */
30453061
if (errno != ENOENT)
3046-
elog(LOG,
3047-
"could not open temporary-files directory \"%s\": %m",
3048-
tmpdirname);
3062+
ereport(LOG,
3063+
(errcode_for_file_access(),
3064+
errmsg("could not open directory \"%s\": %m",
3065+
tmpdirname)));
30493066
return;
30503067
}
30513068

@@ -3099,9 +3116,10 @@ RemovePgTempRelationFiles(const char *tsdirname)
30993116
{
31003117
/* anything except ENOENT is fishy */
31013118
if (errno != ENOENT)
3102-
elog(LOG,
3103-
"could not open tablespace directory \"%s\": %m",
3104-
tsdirname);
3119+
ereport(LOG,
3120+
(errcode_for_file_access(),
3121+
errmsg("could not open directory \"%s\": %m",
3122+
tsdirname)));
31053123
return;
31063124
}
31073125

@@ -3136,16 +3154,8 @@ RemovePgTempRelationFilesInDbspace(const char *dbspacedirname)
31363154
char rm_path[MAXPGPATH * 2];
31373155

31383156
dbspace_dir = AllocateDir(dbspacedirname);
3139-
if (dbspace_dir == NULL)
3140-
{
3141-
/* we just saw this directory, so it really ought to be there */
3142-
elog(LOG,
3143-
"could not open dbspace directory \"%s\": %m",
3144-
dbspacedirname);
3145-
return;
3146-
}
31473157

3148-
while ((de = ReadDir(dbspace_dir, dbspacedirname)) != NULL)
3158+
while ((de = ReadDirExtended(dbspace_dir, dbspacedirname, LOG)) != NULL)
31493159
{
31503160
if (!looks_like_temp_rel_name(de->d_name))
31513161
continue;
@@ -3310,13 +3320,6 @@ walkdir(const char *path,
33103320
struct dirent *de;
33113321

33123322
dir = AllocateDir(path);
3313-
if (dir == NULL)
3314-
{
3315-
ereport(elevel,
3316-
(errcode_for_file_access(),
3317-
errmsg("could not open directory \"%s\": %m", path)));
3318-
return;
3319-
}
33203323

33213324
while ((de = ReadDirExtended(dir, path, elevel)) != NULL)
33223325
{
@@ -3356,9 +3359,11 @@ walkdir(const char *path,
33563359
/*
33573360
* It's important to fsync the destination directory itself as individual
33583361
* file fsyncs don't guarantee that the directory entry for the file is
3359-
* synced.
3362+
* synced. However, skip this if AllocateDir failed; the action function
3363+
* might not be robust against that.
33603364
*/
3361-
(*action) (path, true, elevel);
3365+
if (dir)
3366+
(*action) (path, true, elevel);
33623367
}
33633368

33643369

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

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -111,9 +111,10 @@ ResetUnloggedRelationsInTablespaceDir(const char *tsdirname, int op)
111111
{
112112
/* anything except ENOENT is fishy */
113113
if (errno != ENOENT)
114-
elog(LOG,
115-
"could not open tablespace directory \"%s\": %m",
116-
tsdirname);
114+
ereport(LOG,
115+
(errcode_for_file_access(),
116+
errmsg("could not open directory \"%s\": %m",
117+
tsdirname)));
117118
return;
118119
}
119120

@@ -164,9 +165,10 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
164165
dbspace_dir = AllocateDir(dbspacedirname);
165166
if (dbspace_dir == NULL)
166167
{
167-
elog(LOG,
168-
"could not open dbspace directory \"%s\": %m",
169-
dbspacedirname);
168+
ereport(LOG,
169+
(errcode_for_file_access(),
170+
errmsg("could not open directory \"%s\": %m",
171+
dbspacedirname)));
170172
return;
171173
}
172174

@@ -226,9 +228,10 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
226228
dbspace_dir = AllocateDir(dbspacedirname);
227229
if (dbspace_dir == NULL)
228230
{
229-
elog(LOG,
230-
"could not open dbspace directory \"%s\": %m",
231-
dbspacedirname);
231+
ereport(LOG,
232+
(errcode_for_file_access(),
233+
errmsg("could not open directory \"%s\": %m",
234+
dbspacedirname)));
232235
hash_destroy(hash);
233236
return;
234237
}
@@ -296,9 +299,10 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
296299
if (dbspace_dir == NULL)
297300
{
298301
/* we just saw this directory, so it really ought to be there */
299-
elog(LOG,
300-
"could not open dbspace directory \"%s\": %m",
301-
dbspacedirname);
302+
ereport(LOG,
303+
(errcode_for_file_access(),
304+
errmsg("could not open directory \"%s\": %m",
305+
dbspacedirname)));
302306
return;
303307
}
304308

@@ -349,9 +353,10 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
349353
if (dbspace_dir == NULL)
350354
{
351355
/* we just saw this directory, so it really ought to be there */
352-
elog(LOG,
353-
"could not open dbspace directory \"%s\": %m",
354-
dbspacedirname);
356+
ereport(LOG,
357+
(errcode_for_file_access(),
358+
errmsg("could not open directory \"%s\": %m",
359+
dbspacedirname)));
355360
return;
356361
}
357362

0 commit comments

Comments
 (0)