Thread: 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
Attachment
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/
On Fri, 17 May 2024 at 19:19, David Rowley <dgrowleyml@gmail.com> wrote: > > On Thu, 16 May 2024 at 07:20, Peter Geoghegan <pg@bowt.ie> wrote: > > 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've attached an updated patch which updates the comments and also removes the misplaced Asserts from BufFileAppend. If there are no objections or additional feedback, I'll push this patch soon. David
Attachment
On Wed, 3 Jul 2024 at 08:06, David Rowley <dgrowleyml@gmail.com> wrote: > > On Fri, 17 May 2024 at 19:19, David Rowley <dgrowleyml@gmail.com> wrote: > > > > On Thu, 16 May 2024 at 07:20, Peter Geoghegan <pg@bowt.ie> wrote: > > > 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've attached an updated patch which updates the comments and also > removes the misplaced Asserts from BufFileAppend. > > If there are no objections or additional feedback, I'll push this patch soon. Finding this thread after reviewing [0], this does explain why the Assert was changed in that patch. LGTM. Kind regards, Matthias van de Meent Neon (https://neon.tech) [0] https://www.postgresql.org/message-id/flat/CAApHDvp5Py9g4Rjq7_inL3-MCK1Co2CRt_YWFwTU2zfQix0p4A%40mail.gmail.com
David Rowley <dgrowleyml@gmail.com> writes:
> I've attached an updated patch which updates the comments and also
> removes the misplaced Asserts from BufFileAppend.
> If there are no objections or additional feedback, I'll push this patch soon.
- * Return the current fileset based BufFile size.
+ * Returns the size if the given BufFile in bytes.
"Returns the size of", no doubt?
A shade less nit-pickily, I wonder if "size" is sufficient.
It's not really obvious that this means the amount of data
in the file, rather than say sizeof(BufFile). How about
+ * Returns the amount of data in the given BufFile, in bytes.
LGTM other than that point.
regards, tom lane
On Thu, 4 Jul 2024 at 04:19, Tom Lane <tgl@sss.pgh.pa.us> wrote: > - * Return the current fileset based BufFile size. > + * Returns the size if the given BufFile in bytes. > > "Returns the size of", no doubt? Yes, thanks. > A shade less nit-pickily, I wonder if "size" is sufficient. > It's not really obvious that this means the amount of data > in the file, rather than say sizeof(BufFile). How about > > + * Returns the amount of data in the given BufFile, in bytes. I've used that. Thank you. > LGTM other than that point. Pushed. David