Subject: Re: readdir is incorrectly implemented at Windows
To: Michael Paquier <michael@paquier.xyz>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: <2cad7829-8d66-e39c-b937-ac825db5203d@postgrespro.ru>
 <20190228021553.GG1617@paquier.xyz> <20190301061329.GA31098@paquier.xyz>
From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Message-ID: <000f5395-a787-100b-ceaa-88d86d8be2fc@postgrespro.ru>
Date: Fri, 1 Mar 2019 10:23:02 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <20190301061329.GA31098@paquier.xyz>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 01.03.2019 9:13, Michael Paquier wrote:
> On Thu, Feb 28, 2019 at 11:15:53AM +0900, Michael Paquier wrote:
>> Could you add it to the next commit fest as a bug fix please?  I think
>> that I will be able to look at that in details soon, but if not it
>> would be better to not lose track of your fix.
> Okay, I have looked at your patch.  And double-checked on Windows.  To
> put it short, I agree with the approach you are taking.  I have been
> curious about the mention to MinGW in the existing code as well as in
> the patch you are proposing, and I have checked if what your patch and
> what the current state are correct, and I think that HEAD is wrong on
> one thing.
>
> First mingw64 code can be found here:
> https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/
> https://github.com/Alexpux/mingw-w64/
>
> Then, the implementation of readdir/opendir/closedir can be found in
> mingw-w64-crt/misc/dirent.c.  Looking at their implementation of
> readdir, I can see two things:
> 1) When beginning a search in a directory, _tfindfirst gets used,
> which returns ENOENT as error if no matches are found:
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/findfirst-functions?view=vs-2017
> So from this point of view your patch is right: you make readdir()
> return errno=0 which matches the normal *nix behaviors.  And MinGW
> does not do that.
> 2) On follow-up lookups, MinGW code uses _tfindnext, and actually
> *enforces* errno=0 when seeing ERROR_NO_MORE_FILES.  So from this
> point of view the code's comment in HEAD is incorrect as of today.
> The current implementation exists since 399a36a so perhaps MinGW was
> not like that when dirent.c has been added in src/port/, but that's
> not true today. So let's fix the comment at the same time.
>
> Attached is an updated patch with my suggestions.  Does it look fine
> to you?
Yes, certainly.

>
> Also, I think that we should credit Yuri Kurenkov for the discovery of
> the issue, with yourself, Konstantin, as the author of the patch.
> Are there other people involved which should be credited?  Like
> Grigory?

Yes,  Yuri Kurenkov and Grigory Smalking did a lot in investigation of 
this problem.
(the irony is that the problem detected by Yuri was caused by another 
bug in pg_probackup, but we thought
that it was related with permissions and come to this issue).

> --
> Michael

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company