BUG #18866: Running pg_freespace() on views triggers an Abort

PG Bug reporting form <noreply@postgresql.org> 于2025年3月25日周二 22:42写道：

The following bug has been logged on the website:

Bug reference: 18866
Logged by: Robins Tharakan
Email address: tharakan@gmail.com
PostgreSQL version: Unsupported/Unknown
Operating system: Ubuntu
Description:

Hi,

Passing a view to pg_freespace() triggers an Abort on HEAD. This has been
so
since the beginning (049ef3398d05c9dc8f48aa9a6d68440661cfeb87). Given that
this is just an assert, feel free to skip - but thought I'd bring it up, in
case this needs a review.

SQL / Backtrace / Error Log excerpt given below:

SQL
===
$ cat crashing.sql
CREATE EXTENSION pg_freespacemap;
SELECT pg_freespace('pg_roles', 0);

Yeah, it would crash if you input a foreign table, for example:
create extension postgres_fdw;
CREATE SERVER d FOREIGN DATA WRAPPER postgres_fdw;
CREATE FOREIGN TABLE f (g text) SERVER d;
SELECT pg_freespace('f', 0); -- will crash too

I think we can remove the Assert in smgropen().
Any thoughts?

--
Thanks,
Tender Wang

Tender Wang <tndrwang@gmail.com> writes:

PG Bug reporting form <noreply@postgresql.org> 于2025年3月25日周二 22:42写道：

Passing a view to pg_freespace() triggers an Abort on HEAD.

I think we can remove the Assert in smgropen().
Any thoughts?

That seems like a "solution" that will allow other bugs. pg_freespace
should be fixed to verify RELKIND_HAS_STORAGE() before it tries to
access said storage.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> 于2025年3月26日周三 00:37写道：

Tender Wang <tndrwang@gmail.com> writes:

PG Bug reporting form <noreply@postgresql.org> 于2025年3月25日周二 22:42写道：

Passing a view to pg_freespace() triggers an Abort on HEAD.

I think we can remove the Assert in smgropen().
Any thoughts?

That seems like a "solution" that will allow other bugs. pg_freespace
should be fixed to verify RELKIND_HAS_STORAGE() before it tries to
access said storage.

Thanks for the advice. Please see the attached patch.

--
Thanks,
Tender Wang

On Tue, Mar 25, 2025, at 10:18 PM, Tender Wang wrote:

Thanks for the advice. Please see the attached patch.

Your patch needs some adjustments. There is no need to include pg_class.h. I
don't like the proposed error message. I prefer saying the relation cannot be
opened because that's what will happen if it reaches this code path. This is an
existing message so no need for translation. The output will be like:

postgres=# SELECT pg_freespace('f', 0);
ERROR: cannot open relation "f"
DETAIL: This operation is not supported for foreign tables.
postgres=# SELECT pg_freespace('pg_roles', 0);
ERROR: cannot open relation "pg_roles"
DETAIL: This operation is not supported for views.
postgres=# SELECT pg_freespace('pg_class', 0);
pg_freespace
--------------
7520
(1 row)

I'm attaching a patch that includes these modifications.

--
Euler Taveira
EDB https://www.enterprisedb.com/

"Euler Taveira" <euler@eulerto.com> writes:

Your patch needs some adjustments. There is no need to include pg_class.h. I
don't like the proposed error message. I prefer saying the relation cannot be
opened because that's what will happen if it reaches this code path.

I don't care for that proposal either: we just did open the relation ;-)

Looking at other places where we throw an error for !RELKIND_HAS_STORAGE:

rawpage.c:
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot get raw page from relation \"%s\"",

pgstatindex.c:

(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot get page count of relation \"%s\"",

tid.c:
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot look at latest visible tid for relation \"%s.%s\"",

relcache.c:
elog(ERROR, "relation \"%s\" does not have storage",
RelationGetRelationName(relation));

So the previous proposal was evidently modeled on the first two of
these precedents. Personally I prefer messages that say *why*
something failed, so I'd go with something more like "relation \"%s\"
does not have storage". Use of errdetail_relkind_not_supported is
fine though.

regards, tom lane

On Wed, Mar 26, 2025, at 1:00 PM, Tom Lane wrote:

"Euler Taveira" <euler@eulerto.com> writes:

Your patch needs some adjustments. There is no need to include pg_class.h. I
don't like the proposed error message. I prefer saying the relation cannot be
opened because that's what will happen if it reaches this code path.

I don't care for that proposal either: we just did open the relation ;-)

Fair point.

relcache.c:
elog(ERROR, "relation \"%s\" does not have storage",
RelationGetRelationName(relation));

So the previous proposal was evidently modeled on the first two of
these precedents. Personally I prefer messages that say *why*
something failed, so I'd go with something more like "relation \"%s\"
does not have storage". Use of errdetail_relkind_not_supported is
fine though.

I thought about saying "no storage" but don't know why was convinced by that
proposal. Your suggestion works for me.

--
Euler Taveira
EDB https://www.enterprisedb.com/

On 2025-Mar-25, Euler Taveira wrote:

On Tue, Mar 25, 2025, at 10:18 PM, Tender Wang wrote:

Thanks for the advice. Please see the attached patch.

Your patch needs some adjustments. There is no need to include pg_class.h.

I think including pg_class.h (which is where RELKIND_HAS_STORAGE() is
defined and where errdetail_relkind_not_supported is declared) is better
than including utils/rel.h, which brings in a whole bunch of additional
stuff in addition to pg_class.h :-)

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end." (2nd Commandment for C programmers)

On Wed, Mar 26, 2025, at 2:33 PM, Álvaro Herrera wrote:

On 2025-Mar-25, Euler Taveira wrote:

On Tue, Mar 25, 2025, at 10:18 PM, Tender Wang wrote:

Thanks for the advice. Please see the attached patch.

Your patch needs some adjustments. There is no need to include pg_class.h.

I think including pg_class.h (which is where RELKIND_HAS_STORAGE() is
defined and where errdetail_relkind_not_supported is declared) is better
than including utils/rel.h, which brings in a whole bunch of additional
stuff in addition to pg_class.h :-)

It doesn't work because of RelationGetRelationName() that is defined into
utils/rel.h.

--
Euler Taveira
EDB https://www.enterprisedb.com/

On 2025-Mar-26, Euler Taveira wrote:

It doesn't work because of RelationGetRelationName() that is defined into
utils/rel.h.

Doh, of course.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"I can't go to a restaurant and order food because I keep looking at the
fonts on the menu. Five minutes later I realize that it's also talking
about food" (Donald Knuth)

Tom Lane <tgl@sss.pgh.pa.us> 于2025年3月27日周四 00:01写道：

"Euler Taveira" <euler@eulerto.com> writes:

Your patch needs some adjustments. There is no need to include

pg_class.h. I

don't like the proposed error message. I prefer saying the relation

cannot be

opened because that's what will happen if it reaches this code path.

I don't care for that proposal either: we just did open the relation ;-)

Looking at other places where we throw an error for !RELKIND_HAS_STORAGE:

rawpage.c:
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot get raw page from relation \"%s\"",

pgstatindex.c:

(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot get page count of relation \"%s\"",

tid.c:
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot look at latest visible tid for relation
\"%s.%s\"",

relcache.c:
elog(ERROR, "relation \"%s\" does not have storage",
RelationGetRelationName(relation));

So the previous proposal was evidently modeled on the first two of
these precedents.

Yes, I referred to the rawpage.c.

Personally I prefer messages that say *why*
something failed, so I'd go with something more like "relation \"%s\"
does not have storage". Use of errdetail_relkind_not_supported is
fine though.

PFA for the updated patch file.

--
Thanks,
Tender Wang

On Thu, Mar 27, 2025 at 4:14 PM Tender Wang <tndrwang@gmail.com> wrote:

PFA for the updated patch file.

Not related to this issue but I wonder why blkno is verified after
the relation is opened. It can be verified beforehand, no?

Thanks
Richard

Richard Guo <guofenglinux@gmail.com> writes:

Not related to this issue but I wonder why blkno is verified after
the relation is opened. It can be verified beforehand, no?

It makes sense to me to check the parameters left-to-right, so
I think verifying blkno after relation is fine. It's not like
there's value in optimizing the failure case.

This does, however, suggest that we ought to check the relkind
immediately after opening the rel, before the blkno check.
I'll adjust the patch that way and push.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> 于2025年3月28日周五 00:01写道：

Richard Guo <guofenglinux@gmail.com> writes:

Not related to this issue but I wonder why blkno is verified after
the relation is opened. It can be verified beforehand, no?

It makes sense to me to check the parameters left-to-right, so
I think verifying blkno after relation is fine. It's not like
there's value in optimizing the failure case.

This does, however, suggest that we ought to check the relkind
immediately after opening the rel, before the blkno check.
I'll adjust the patch that way and push.

Thanks for pushing.

--
Thanks,
Tender Wang