Thread: pg_verify_checksums vs windows

While trying to debug a recent bug report on hash indexes [1], I
noticed that pg_verify_checksums don't work on Windows (or at least in
my environment).

initdb -k ..\..\data
pg_verify_checksums.exe  ..\..\Data
pg_verify_checksums: short read of block 0 in file
"..\..\Data/global/1136", got only 15 bytes

I have debugged and found that below code is the culprit.

scan_file(char *fn, int segmentno)
{
..
f = open(fn, 0);
..
int r = read(f, buf, BLCKSZ);

if (r == 0)
break;

if (r != BLCKSZ)
{
fprintf(stderr, _("%s: short read of block %d in file \"%s\", got only
%d bytes\n"),
progname, blockno, fn, r);
exit(1);
}
..
}

We are opening the file in text mode and trying to read the BLCKSZ
bytes, however, if there is any Control-Z char, it is treated as EOF.
This problem has been mentioned in the comments in c.h as follows:
/*
 * 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.
 */

So, I think we need to open the file in binary mode as in other parts
of the code.  The attached patch fixes the problem for me.

Thoughts?

[1] - https://www.postgresql.org/message-id/5d03686d-727c-dbf8-0064-bf8b97ffe850%402ndquadrant.com
-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Aug 29, 2018 at 5:05 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Wed, Aug 29, 2018 at 1:31 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> So, I think we need to open the file in binary mode as in other parts
>> of the code.  The attached patch fixes the problem for me.
>>
>> Thoughts?
>
>
> Yikes. Yes, I believe you are correct, and that looks like the correct fix.
>
> I wonder why this was not caught on the buildfarm. We do have regression tests for it, AFAIK?
>

I am not able to find regression tests for it, but maybe I am not
seeing it properly.  By any chance, you have removed it during revert
of ""Allow on-line enabling and disabling of data checksums".

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

On Wed, Aug 29, 2018 at 5:17 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Wed, Aug 29, 2018 at 1:44 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Wed, Aug 29, 2018 at 5:05 PM Magnus Hagander <magnus@hagander.net> wrote:
>> >
>> > On Wed, Aug 29, 2018 at 1:31 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> >>
>> >> So, I think we need to open the file in binary mode as in other parts
>> >> of the code.  The attached patch fixes the problem for me.
>> >>
>> >> Thoughts?
>> >
>> >
>> > Yikes. Yes, I believe you are correct, and that looks like the correct fix.
>> >
>> > I wonder why this was not caught on the buildfarm. We do have regression tests for it, AFAIK?
>> >
>>
>> I am not able to find regression tests for it, but maybe I am not
>> seeing it properly.  By any chance, you have removed it during revert
>> of ""Allow on-line enabling and disabling of data checksums".
>>
>
> Oh meh. You are right, it's in the reverted patch, I was looking in the wrong branch :/ Sorry about that. And that
certainlyexplains why we don't have it.
 
>

Okay.  I will commit this in a day or so after once verifying it on
PG11 as well.  I think this needs to be backpatched, let me know if
you think otherwise.

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

On Thu, Aug 30, 2018 at 5:04 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Thu, Aug 30, 2018 at 1:32 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>>
>> Okay.  I will commit this in a day or so after once verifying it on
>> PG11 as well.  I think this needs to be backpatched, let me know if
>> you think otherwise.
>>
>
> Definitely a bug so yes, it needs backpatching.
>

Okay, pushed!

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