Errands around AllocateDir()

From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>
Subject: Errands around AllocateDir()
Date: 2017-12-04 07:13:41
Message-ID: CAB7nPqRpOCxjiirHmebEFhXVTK7V5Jvw4bz82p7Oimtsm3TyZA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi all,

On the recent following thread problems around the use of
AllocateDir() have been diagnosed with its use in the backend code:
https://www.postgresql.org/message-id/20171127093107.1473.70477@wrigleys.postgresql.org

I had a close look at all the callers of AllocateDir() and noticed a
couple of unwelcome things (Tom noticed some of those in the thread
mentioned above, I found others):
- Some elog() instead of ereport(), which is bad for the error code
and translation.
- Some use ereport(LOG), and could take advantage of ReadDirExtended,
which could get exposed instead of being contained in fd.c. Those
elog(LOG) can be removed when there is no further operation in the
routine where the folder read is happening.
- perform_base_backup() makes the mistake of not saving errno before
CheckXLogRemoved() when AllocateDir returns NULL, which can lead to an
incorrect error message.
- restoreTwoPhaseData() calls ReadDir() with LWLockAcquire()
in-between, which can lead to an incorrect errno used.
- Some code paths can simply rely on the error generated by ReadDir(),
so some code is useless.
- Some code paths use non-generic error messages, like
RemoveOldXlogFiles(). Here more consistent error string would I think
make sense.

Some issues are real bugs, like what's in perform_base_backup() and
restoreTwoPhaseData(), and some incorrect error codes. Missing
translation of some messages is also wrong IMO. Making error messages
more consistent is nice and cosmetic. My monitoring of all those
issues is leading me to the attached patch for HEAD. Thoughts?
--
Michael

Attachment Content-Type Size
allocatedir-errands.patch application/octet-stream 15.1 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alexander Korotkov 2017-12-04 08:21:43 Re: [HACKERS] proposal: psql command \graw
Previous Message Amit Kapila 2017-12-04 06:33:35 Re: Would a BGW need shmem_access or database_connection to enumerate databases?