Re: BUG #14929: Unchecked AllocateDir() return value in restoreTwoPhaseData()

PanBian <bianpan2016(at)163(dot)com> writes:
> On Mon, Nov 27, 2017 at 07:53:30PM +0900, Michael Paquier wrote:
>> You are missing the fact that ReadDir goes through ReadDirExtended,
>> which drops an ERROR log if the folder allocated is NULL.

> You are right. Its my carelessness. ReadDir will not return back on a
> NULL dir parameter. The code is bug free. Sorry for the trouble.

There are some issues here, though:

1. twophase.c does this:

cldir = AllocateDir(TWOPHASE_DIR);
LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
while ((clde = ReadDir(cldir, TWOPHASE_DIR)) != NULL)
{

which is flat out wrong because LWLockAcquire might well clobber errno.
I don't see any good reason why we couldn't just swap the order of
those two calls.

2. basebackup.c and some other places do things like

dir = AllocateDir("pg_wal");
if (!dir)
ereport(ERROR,
(errmsg("could not open directory \"%s\": %m", "pg_wal")));
while ((de = ReadDir(dir, "pg_wal")) != NULL)

Not only is this a waste of code, because the error message is no better
than what ReadDir would provide, but it's wrong because it omits
errcode_for_file_access(), causing the SQLSTATE to be reported as XX000.
There are other places that are even lazier and use elog(), failing
translatability as well as the errcode test.

There might be some other problems I missed in a quick scan.

So there's definitely room for a cleanup patch here, but the originally
proposed change isn't it.

regards, tom lane