Re: pg_verify_checksums vs windows

[Amit Kapila]

On Thu, Aug 30, 2018 at 5:04 PM Magnus Hagander  wrote:
>
> On Thu, Aug 30, 2018 at 1:32 PM, Amit Kapila  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

Re: pg_verify_checksums vs windows

[Magnus Hagander]

On Thu, Aug 30, 2018 at 1:32 PM, Amit Kapila 
wrote:

> On Wed, Aug 29, 2018 at 5:17 PM Magnus Hagander 
> wrote:
> >
> > On Wed, Aug 29, 2018 at 1:44 PM, Amit Kapila 
> wrote:
> >>
> >> On Wed, Aug 29, 2018 at 5:05 PM Magnus Hagander 
> wrote:
> >> >
> >> > On Wed, Aug 29, 2018 at 1:31 PM, Amit Kapila 
> 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 certainly explains 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.
>
>
Definitely a bug so yes, it needs backpatching.


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: pg_verify_checksums vs windows

[Amit Kapila]

On Wed, Aug 29, 2018 at 5:17 PM Magnus Hagander  wrote:
>
> On Wed, Aug 29, 2018 at 1:44 PM, Amit Kapila  wrote:
>>
>> On Wed, Aug 29, 2018 at 5:05 PM Magnus Hagander  wrote:
>> >
>> > On Wed, Aug 29, 2018 at 1:31 PM, Amit Kapila  
>> > 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 certainly explains 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

Re: pg_verify_checksums vs windows

[Magnus Hagander]

On Wed, Aug 29, 2018 at 1:44 PM, Amit Kapila 
wrote:

> On Wed, Aug 29, 2018 at 5:05 PM Magnus Hagander 
> wrote:
> >
> > On Wed, Aug 29, 2018 at 1:31 PM, Amit Kapila 
> 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 certainly explains why we don't
have it.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: pg_verify_checksums vs windows

[Amit Kapila]

On Wed, Aug 29, 2018 at 5:05 PM Magnus Hagander  wrote:
>
> On Wed, Aug 29, 2018 at 1:31 PM, Amit Kapila  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

Re: pg_verify_checksums vs windows

[Magnus Hagander]

On Wed, Aug 29, 2018 at 1:31 PM, Amit Kapila 
wrote:

> 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?
>

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? Or maybe we just lucked out there because there was no
^Z char in the files there?


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/