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

On 2020-Sep-11, Ranier Vilela wrote:

> Hi,
>
> read_binary_file seems a bit complicated when we want to read the rest of
> the file (-1 for bytes_to_read).

This code was very recently rewritten in 96d1f423f95d, and I doubt that
taking out half the algorithm without studying how it got that way is a
great idea.

On 2020-09-11 14:10:31 -0300, Ranier Vilela wrote:
> Em sex., 11 de set. de 2020 às 14:01, Alvaro Herrera <
> alvherre@2ndquadrant.com> escreveu:
> > On 2020-Sep-11, Ranier Vilela wrote:
> > This code was very recently rewritten in 96d1f423f95d, and I doubt that
> > taking out half the algorithm without studying how it got that way is a
> > great idea.
> >
> Works with all regress tests (199, same as the HEAD with msvc 2019 64 bits
> except partition_prune)
> Works with pgbench (pgbench -U postgres -c 50 -j 2 -t 10000 example)
> And works with local installation, without adverse effects, for now.

Have your read the commit message for 96d1f423f95d ?

Ranier Vilela <ranier.vf@gmail.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?

* bytes_to_read == 0 is a legal case.

* "Assert(seek_offset != 0)" is an equally awful idea.

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

* 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.

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.

Ranier Vilela <ranier.vf@gmail.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.

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.

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

Ranier Vilela <ranier.vf@gmail.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.

I think you meant _IONBF instead of _IOFBF -- otherwise it's at odds
with the comment you add.  But what is the justification for that
addition?  I don't see us doing that anywhere else.