Re: Simplified version of read_binary_file (src/backend/utils/adt/genfile.c)

Em sex., 11 de set. de 2020 às 17:44, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> escreveu:

> Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> writes:
> > New version, with support to read Virtual File (pipe, FIFO and socket).
> > With assert, in case, erroneous, of trying to read a pipe, with offset.
>
> Really, could you do a little more thinking and testing of your own,
> rather than expecting the rest of us to point out holes in your thinking?
>
Yes, I can.

> * bytes_to_read == 0 is a legal case.
>
Ok. Strange call to read a file with zero bytes.

> * "Assert(seek_offset != 0)" is an equally awful idea.
>
I was trying to predict the case of reading a Virtual File, with
bytes_to_read == -1 and seek_offset == 0,
which is bound to fail in fseeko.
In any case, 96d1f423f95d it will fail with any Virtual File.

>
> * Removing the seek code from the negative-bytes_to_read path
> is just broken.
>
Ok.

> * The only reason this code is shorter than the previous version is
> you took out all the comments explaining what it was doing, and
> failed to replace them. This is just as subtle as before, so I
> don't find that acceptable.
>
It was not my intention.

>
> I do agree that it might be worth skipping the fseeko call in the
> probably-common case where seek_offset is zero. Otherwise I don't
> see much reason to change what's there.
>
Well, I think that v3 is a little better, but it’s just my opinion.
v3 try to copy directly in the final buffer, which will certainly be a
little faster.

regards,
Ranier Vilela