On Tue, 17 Sep 2019 at 21:19, Andres Freund <and...@anarazel.de> wrote: > On 2019-09-14 14:34:21 -0400, Tom Lane wrote: > > Amit Khandekar <amitdkhan...@gmail.com> writes: > > > Yeah, something like the attached patch. I think this tracking of > > > offsets would have been cleaner if we add in-built support in VFD. But > > > yeah, for bank branches at least, we need to handle it outside of VFD. > > > Or may be we would add it if we find one more use-case. > > > > Again, we had that and removed it, for what seem to me to be solid > > reasons. It adds cycles when we're forced to close/reopen a file, > > and it also adds failure modes that we could do without (ie, failure > > of either the ftell or the lseek, which are particularly nasty because > > they shouldn't happen according to the VFD abstraction).
Ok. So you mean, when the caller would call FileRead() for sequential reading, underneath VFD would do a pread(), but if pread() returns error, the errno can belong to read() or it might as well belong to lseek(). If it's due to lseek(), it's not expected from the caller because for the caller it's just a sequential read. Yeah, makes sense. >> I do not > > think there is going to be any argument strong enough to make us > > put it back, especially not for non-mainstream callers like logical > > decoding. Ok. Also, more about putting back is in the below comments ... > > Yea, I think that's the right call. Avoiding kernel seeks is quite > worthwhile, and we shouldn't undo it just because of this usecase. And > that'll become more and more important performance-wise (and has already > done so, with all the intel fixes making syscalls much slower). By the way, I was not thinking about adding back the read() and lseek() calls. I was saying we continue to use the pread() call, so it's just a single system call. FileReadAt(..., offset) would do pread() with user-supplied offset, and FileRead() would do pread() using internally tracked offset. So for the user, FileReadAt() is like pread(), and FileRead() would be like read(). But I agree with Tom's objection about having to unnecessarily handle lseek error codes. > > I could see an argument for adding a separate generic layer providing > position tracking ontop of the VFD abstraction however. Seems quite > possible that there's some other parts of the system that could benefit > from using VFDs rather than plain fds. And they'd probably also need the > positional tracking. Yeah, that also could be done. Probably, for now at least, what everyone seems to agree is to take my earlier attached patch forward. I am going to see if I can add a TAP test for the patch, and will add the patch into the commitfest soon. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company