Re: dynamic shared memory

On Mon, Sep 23, 2013 at 12:34 AM, Noah Misch <noah@leadboat.com> wrote:
> On Sun, Sep 22, 2013 at 01:17:52PM +0530, Amit Kapila wrote:
>> On Fri, Sep 20, 2013 at 5:14 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > On 2013-09-19 11:44:34 -0400, Robert Haas wrote:
>> >> On Wed, Sep 18, 2013 at 1:42 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> >> >> +     /* Create or truncate the file. */
>> >> >> +     statefd = open(PG_DYNSHMEM_NEW_STATE_FILE, O_RDWR|O_CREAT|O_TRUNC, 0600);
>> >> >
>> >> > Doesn't this need a | PG_BINARY?
>> >>
>> >> It's a text file.  Do we need PG_BINARY anyway?
>> >
>> > I'd say yes. Non binary mode stuff on windows does stuff like
>> > transforming LF <=> CRLF on reading/writing, which makes sizes not match
>> > up and similar ugliness.
>> > Imo there's little reason to use non-binary mode for anything written
>> > for postgres' own consumption.
>>
>> On checking about this in code, I found the below comment which
>> suggests LF<=> CRLF is not an issue (in windows it uses pgwin32_open
>> to open a file):
>>
>> /*
>>  * NOTE:  this is also used for opening text files.
>>  * WIN32 treats Control-Z as EOF in files opened in text mode.
>>  * Therefore, we open files in binary mode on Win32 so we can read
>>  * literal control-Z. The other affect is that we see CRLF, but
>>  * that is OK because we can already handle those cleanly.
>>  */
>
> That comment appears at the definition of PG_BINARY.  You only get what it
> describes when you use PG_BINARY.
>
>> Second instance, I noticed in code as below which again suggests CRLF
>> should not be an issue until file mode is specifically set to TEXT
>> mode which is not the case with current usage of open in dynamic
>> shared memory code.
>>
>> #ifdef WIN32
>> /* use CRLF line endings on Windows */
>> _setmode(_fileno(fh), _O_TEXT);
>> #endif
>
> I suspect that call (in logfile_open()) has no effect.  The file is already in
> text mode.

Won't this be required when we have to open a new file due to log
rotation based on time?

The basic point, I wanted to make is that until you use _O_TEXT mode
explicitly, the problem LF<=>CRLF will not happen. CreateFile() API
which is used  for windows implementation of open doesn't take any
parameter which specifies it as text or binary, only by using
_setmode, we need to set the file mode as Text or Binary.
I checked fcntl.h where there is below comment above definition of
_O_TEXT and _O_BINARY which again is pointing to what I said above.
/* O_TEXT files have <cr><lf> sequences translated to <lf> on read()'s,
** and <lf> sequences translated to <cr><lf> on write()'s
*/

One more point, this issue has only chance of occurring when somebody
takes the file from unix system to windows and then may be back, do
you think dsm state file should be allowed in cross platform backup, I
think pg_basebackup should  disallow the backup of this file.
However user can use some other custom utility to take filesystem
level backup where this can happen, but still as per my understanding
it should not create problem.

I think to be on safe side we can use PG_BINARY, but it would be
better if we are sure that this problem can occur then only we should
use it.
If you think cross platform backup's can create such issues, then I
can once test this as well.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com