From: | Joe Conway <mail(at)joeconway(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgreSQL(dot)org> |
Subject: | Re: pg_read_file() with virtual files returns empty string |
Date: | 2020-06-28 17:00:29 |
Message-ID: | 239cf821-5f47-ff68-677c-61d15e84e770@joeconway.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 6/27/20 3:43 PM, Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>> The attached patch fixes this for me. I think it ought to be backpatched through
>> pg11.
>
>> Comments?
>
> 1. Doesn't seem to be accounting for the possibility of an error in fread().
>
> 2. Don't we want to remove the stat() call altogether, if we're not
> going to believe its length?
>
> 3. This bit might need to cast the RHS to int64:
> if (bytes_to_read > (MaxAllocSize - VARHDRSZ))
> otherwise it might be treated as an unsigned comparison.
> Or you could check for bytes_to_read < 0 separately.
>
> 4. appendStringInfoString seems like quite the wrong thing to use
> when the input is binary data.
>
> 5. Don't like the comment. Whether the file is virtual or not isn't
> very relevant here.
>
> 6. If the file size exceeds 1GB, I fear we'll get some rather opaque
> failure from the stringinfo infrastructure. It'd be better to
> check for that here and give a file-too-large error.
All good stuff -- I believe the attached checks all the boxes.
I noted while at this, that the current code can never hit this case:
! if (bytes_to_read < 0)
! {
! if (seek_offset < 0)
! bytes_to_read = -seek_offset;
The intention here seems to be that if you pass bytes_to_read = -1 with a
negative offset, it will give you the last offset bytes of the file.
But all of the SQL exposed paths disallow an explicit negative value for
bytes_to_read. This was also not documented as far as I can tell so I eliminated
that case in the attached. Is that actually a case I should fix/support instead?
Separately, it seems to me that a two argument version of pg_read_file() would
be useful:
pg_read_file('filename', offset)
In that case bytes_to_read = -1 could be passed down in order to read the entire
file after the offset. In fact I think that would nicely handle the negative
offset case as well.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Attachment | Content-Type | Size |
---|---|---|
read-virtual-files.01.diff | text/x-patch | 4.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2020-06-28 17:10:59 | Re: bugfix: invalid bit/varbit input causes the log file to be unreadable |
Previous Message | Felix Lechner | 2020-06-28 16:33:52 | Re: Fwd: PostgreSQL: WolfSSL support |