Обсуждение: Incorrect Assert in BufFileSize()?
I'm trying to figure out why BufFileSize() Asserts that file->fileset isn't NULL, per 1a990b207. The discussion for that commit is in [1], but I don't see any explanation of the Assert in the discussion or commit message and there's no comment explaining why it's there. The code that comes after the Assert does not look at the fileset field. With the code as it is, it doesn't seem possible to get the file size of a non-shared BufFile and I don't see any reason for that. Should the Assert be checking file->files != NULL? David [1] https://postgr.es/m/CAH2-Wzn0ZNLZs3DhCYdLMv4xn1fnM8ugVHPvWz67dSUh1s_%3D2Q%40mail.gmail.com
On Fri, 3 May 2024 at 16:03, David Rowley <dgrowleyml@gmail.com> wrote: > I'm trying to figure out why BufFileSize() Asserts that file->fileset > isn't NULL, per 1a990b207. I was hoping to get some feedback here. Here's a patch to remove the Assert(). Changing it to Assert(file->files != NULL); doesn't do anything useful. David
Вложения
On Tue, May 14, 2024 at 6:58 AM David Rowley <dgrowleyml@gmail.com> wrote: > On Fri, 3 May 2024 at 16:03, David Rowley <dgrowleyml@gmail.com> wrote: > > I'm trying to figure out why BufFileSize() Asserts that file->fileset > > isn't NULL, per 1a990b207. > > I was hoping to get some feedback here. Notice that comments above BufFileSize() say "Return the current fileset based BufFile size". There are numerous identical assertions at the start of several other functions within the same file. I'm not sure what it would take for this function to support OpenTemporaryFile()-based BufFiles -- possibly not very much at all. I assume that that's what you're actually interested in doing here (you didn't say). If it is, you'll need to update the function's contract -- just removing the assertion isn't enough. -- Peter Geoghegan
On Thu, 16 May 2024 at 07:20, Peter Geoghegan <pg@bowt.ie> wrote: > > On Tue, May 14, 2024 at 6:58 AM David Rowley <dgrowleyml@gmail.com> wrote: > > On Fri, 3 May 2024 at 16:03, David Rowley <dgrowleyml@gmail.com> wrote: > > > I'm trying to figure out why BufFileSize() Asserts that file->fileset > > > isn't NULL, per 1a990b207. > > > > I was hoping to get some feedback here. > > Notice that comments above BufFileSize() say "Return the current > fileset based BufFile size". There are numerous identical assertions > at the start of several other functions within the same file. hmm, unfortunately the comment and existence of numerous other assertions does not answer my question. It just leads to more. The only Assert I see that looks like it might be useful is BufFileExportFileSet() as fileset is looked at inside extendBufFile(). It kinda looks to me that it was left over fragments from the development of a patch when it was written some other way? Looking at the other similar Asserts in BufFileAppend(), I can't figure out what those ones are for either. > I'm not sure what it would take for this function to support > OpenTemporaryFile()-based BufFiles -- possibly not very much at all. I > assume that that's what you're actually interested in doing here (you > didn't say). If it is, you'll need to update the function's contract > -- just removing the assertion isn't enough. I should have explained, I just wasn't quite done with what I was working on when I sent the original email. In [1] I was working on adding additional output in EXPLAIN for the "Material" node to show the memory or disk space used by the tuplestore. The use case there uses a BufFile with no fileset. Calling BufFileSize(state->myfile) from tuplestore.c results in an Assert failure. David [1] https://commitfest.postgresql.org/48/4968/