Horton

Thread

Commits

  1. Remove incorrect Asserts in buffile.c

  1. Incorrect Assert in BufFileSize()?

    David Rowley <dgrowleyml@gmail.com> — 2024-05-03T04:03:45Z

    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

  2. Re: Incorrect Assert in BufFileSize()?

    David Rowley <dgrowleyml@gmail.com> — 2024-05-14T10:58:29Z

    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

  3. Re: Incorrect Assert in BufFileSize()?

    Peter Geoghegan <pg@bowt.ie> — 2024-05-15T19:19:39Z

    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

  4. Re: Incorrect Assert in BufFileSize()?

    David Rowley <dgrowleyml@gmail.com> — 2024-05-17T07:19:20Z

    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/

  5. Re: Incorrect Assert in BufFileSize()?

    David Rowley <dgrowleyml@gmail.com> — 2024-07-03T06:05:50Z

    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

  6. Re: Incorrect Assert in BufFileSize()?

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2024-07-03T08:50:21Z

    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

  7. Re: Incorrect Assert in BufFileSize()?

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-03T16:19:18Z

    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

  8. Re: Incorrect Assert in BufFileSize()?

    David Rowley <dgrowleyml@gmail.com> — 2024-07-03T21:46:24Z

    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