Обсуждение: [PATCH] psql: \dn+ to show size of each schema..

\db+ and \l+ show sizes of tablespaces and databases, so I was surprised in the
past that \dn+ didn't show sizes of schemas.  I would find that somewhat
convenient, and I assume other people would use it even more useful.

\db+ and \l+ seem to walk the filesystem, and this is distinguished from those
cases.  (Also, schemas are per-DB, not global).

Maybe it's an issue if \dn+ is slow and expensive, since that's how to display
ACL.  But \db+ has the same issue.  Maybe there should be a \db++ and \dn++ to
allow \dn+ to showing the ACL but not the size.

pg_relation_size() only includes one fork, and the other functions include
toast, which should be in its separate schema, so it has to be summed across
forks.

postgres=# \dnS+
 child              | postgres |                      |                                  | 946 MB
 information_schema | postgres | postgres=UC/postgres+|                                  | 88 kB
                    |          | =U/postgres          |                                  | 
 pg_catalog         | postgres | postgres=UC/postgres+| system catalog schema            | 42 MB
                    |          | =U/postgres          |                                  | 
 pg_toast           | postgres |                      | reserved schema for TOAST tables | 3908 MB
 public             | postgres | postgres=UC/postgres+| standard public schema           | 5627 MB
                    |          | =UC/postgres         |                                  | 

From c2d68eb54f785c759253d4100460aa1af9cbc676 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Tue, 13 Jul 2021 21:25:48 -0500
Subject: [PATCH] psql: \dn+ to show size of each schema..

See also: 358a897fa, 528ac10c7
---
 src/bin/psql/describe.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 2abf255798..6b9b6ea34a 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -5036,6 +5036,11 @@ listSchemas(const char *pattern, bool verbose, bool showSystem)
         appendPQExpBuffer(&buf,
                           ",\n  pg_catalog.obj_description(n.oid, 'pg_namespace') AS \"%s\"",
                           gettext_noop("Description"));
+
+        appendPQExpBuffer(&buf,
+                          ",\n  (SELECT pg_catalog.pg_size_pretty(sum(pg_relation_size(oid,fork))) FROM
pg_catalog.pg_classc,\n"
 
+                          "     (VALUES('main'),('fsm'),('vm'),('init')) AS fork(fork) WHERE c.relnamespace = n.oid)
AS\"%s\"",
 
+                          gettext_noop("Size"));
     }
 
     appendPQExpBufferStr(&buf,
-- 
2.17.0

Hi

2021年7月14日(水) 12:07 Justin Pryzby <pryzby@telsasoft.com>:
>
> \db+ and \l+ show sizes of tablespaces and databases, so I was surprised in the
> past that \dn+ didn't show sizes of schemas.  I would find that somewhat
> convenient, and I assume other people would use it even more useful.

It's something which would be useful to have. But see this previous proposal:

   https://www.postgresql.org/message-id/flat/2d6d2ebf-4dbc-4f74-17d8-05461f4782e2%40dalibo.com


Regards

Ian Barwick

--
EnterpriseDB: https://www.enterprisedb.com

On Wed, 2021-07-14 at 14:05 +0900, Ian Lawrence Barwick wrote:
> 2021年7月14日(水) 12:07 Justin Pryzby <pryzby@telsasoft.com>:
> > \db+ and \l+ show sizes of tablespaces and databases, so I was surprised in the
> > past that \dn+ didn't show sizes of schemas.  I would find that somewhat
> > convenient, and I assume other people would use it even more useful.
> 
> It's something which would be useful to have. But see this previous proposal:
> 
>    https://www.postgresql.org/message-id/flat/2d6d2ebf-4dbc-4f74-17d8-05461f4782e2%40dalibo.com

Right, I would not like to cause a lot of I/O activity just to look at the
permissions on a schema...

Besides, schemas are not physical, but logical containers.  So I see a point in
measuring the storage used in a certain tablespace, but not so much by all objects
in a certain schema.  It might be useful for accounting purposes, though.
But I don't expect it to be in frequent enough demand to add a psql command.

What about inventing a function pg_schema_size(regnamespace)?

Yours,
Laurenz Albe

On Wed, Jul 14, 2021 at 02:05:29PM +0900, Ian Lawrence Barwick wrote:
> 2021年7月14日(水) 12:07 Justin Pryzby <pryzby@telsasoft.com>:
> >
> > \db+ and \l+ show sizes of tablespaces and databases, so I was surprised in the
> > past that \dn+ didn't show sizes of schemas.  I would find that somewhat
> > convenient, and I assume other people would use it even more useful.
> 
> It's something which would be useful to have. But see this previous proposal:
> 
>    https://www.postgresql.org/message-id/flat/2d6d2ebf-4dbc-4f74-17d8-05461f4782e2%40dalibo.com

Thanks for finding that.

It sounds like the objections were:
1) it may be too slow - I propose the size should be shown only with \n++;
I think \db and \l should get the same treatment, and probably everywhere
should change to use the "int verbose".  I moved the ++ columns to the
right-most column.

2) it may fail or be misleading if user lacks permissions.  
I think Tom's concern was that at some point we might decide to avoid showing a
relation's size to a user who has no access to the rel, and then \dn+ would
show misleading information, or fail.
I implemented this a server-side function for super-user/monitoring role only.  

I think \dn++ is also a reasonable way to address the second concern - if
someone asksk for "very verbose" outpu, they get more of an internal,
implementation dependant output, which might be more likely to change in future
releases.  For example, if we move the ++ columns to the right, someone might
jusifiably think that the \n and \n+ columns would be less likely to change in
the future than the \n++ columns.

I imagine ++ would find more uses in the future.  Like, say, size of an access
methods \dA++.  I'll add that in a future revision - I hope that PG15 will also
have create table like (INCLUDE ACCESS METHOD), ALTER TABLE SET ACCESS METHOD,
and pg_restore --no-tableam.

++ may also allow improved testing of psql features - platform dependent stuff
like size can be in ++, allowing better/easier/testing of +.

-- 
Justin

On Wed, Jul 14, 2021 at 07:42:33AM +0200, Laurenz Albe wrote:
> Besides, schemas are not physical, but logical containers.  So I see a point in
> measuring the storage used in a certain tablespace, but not so much by all objects
> in a certain schema.  It might be useful for accounting purposes, though.

We use only a few schemas, 1) to hide child tables; 2) to exclude some extended
stats from backups, and 1-2 other things.  But it's useful to be able to see
how storage is used by schema, and better to do it conveniently.

I think it'd be even more useful for people who use schemas more widely than we
do:
    "Who's using all our space?"
    \dn++
    "Oh, it's that one - let me clean that up..."

Or, "what's the pg_toast stuff, and do I need to do something about it?"

> But I don't expect it to be in frequent enough demand to add a psql command.
> 
> What about inventing a function pg_schema_size(regnamespace)?

But for "physical" storage it's also possible to get the size from the OS, much
more efficiently, using /bin/df or zfs list (assuming nothing else is using
those filesystems).  The pg_*_size functions are inefficient, but psql \db+ and
\l+ already call them anyway.

For schemas, there's no way to get the size from the OS, so it's nice to make
the size available from psql, conveniently.

v3 patch:
 - fixes an off by one in forkNum loop;
 - removes an unnecessary subquery in describe.c;
 - returns 0 rather than NULL if the schema is empty;
 - adds pg_am_size;

regression=# \dA++
                                  List of access methods
  Name  | Type  |       Handler        |              Description               |  Size   
--------+-------+----------------------+----------------------------------------+---------
 brin   | Index | brinhandler          | block range index (BRIN) access method | 744 kB
 btree  | Index | bthandler            | b-tree index access method             | 21 MB
 gin    | Index | ginhandler           | GIN index access method                | 2672 kB
 gist   | Index | gisthandler          | GiST index access method               | 2800 kB
 hash   | Index | hashhandler          | hash index access method               | 2112 kB
 heap   | Table | heap_tableam_handler | heap table access method               | 60 MB
 heap2  | Table | heap_tableam_handler |                                        | 120 kB
 spgist | Index | spghandler           | SP-GiST index access method            | 5840 kB
(8 rows)

regression=# \dn++
                                   List of schemas
        Name        |  Owner  | Access privileges  |      Description       |  Size   
--------------------+---------+--------------------+------------------------+---------
 fkpart3            | pryzbyj |                    |                        | 168 kB
 fkpart4            | pryzbyj |                    |                        | 104 kB
 fkpart5            | pryzbyj |                    |                        | 40 kB
 fkpart6            | pryzbyj |                    |                        | 48 kB
 mvtest_mvschema    | pryzbyj |                    |                        | 16 kB
 public             | pryzbyj | pryzbyj=UC/pryzbyj+| standard public schema | 69 MB
                    |         | =UC/pryzbyj        |                        | 
 regress_indexing   | pryzbyj |                    |                        | 48 kB
 regress_rls_schema | pryzbyj |                    |                        | 0 bytes
 regress_schema_2   | pryzbyj |                    |                        | 0 bytes
 testxmlschema      | pryzbyj |                    |                        | 24 kB
(10 rows)

-- 
Justin

On Thu, 2021-07-15 at 20:16 -0500, Justin Pryzby wrote:
> On Wed, Jul 14, 2021 at 07:42:33AM +0200, Laurenz Albe wrote:
> > Besides, schemas are not physical, but logical containers.  So I see a point in
> > measuring the storage used in a certain tablespace, but not so much by all objects
> > in a certain schema.  It might be useful for accounting purposes, though.
> >
> > But I don't expect it to be in frequent enough demand to add a psql command.
> 
> But for "physical" storage it's also possible to get the size from the OS, much
> more efficiently, using /bin/df or zfs list (assuming nothing else is using
> those filesystems).  The pg_*_size functions are inefficient, but psql \db+ and
> \l+ already call them anyway.

Hm, yes, the fact that \l+ does something similar detracts from my argument.
It seems somewhat inconsistent to have the size in \l+, but not in \dn+.

Still, there is a difference: I never need \l+, because \l already shows
the permissions on the database, but I often need \dn+ to see the permissions
on schemas.  And I don't want to measure the size when I do that.

The problem is that our backslash commands are not totally consistent in
that respect, and we can hardly fix that.

Yours,
Laurenz Albe

Rebased

rebased

As discussed in [1], we're taking this opportunity to return some
patchsets that don't appear to be getting enough reviewer interest.

This is not a rejection, since we don't necessarily think there's
anything unacceptable about the entry, but it differs from a standard
"Returned with Feedback" in that there's probably not much actionable
feedback at all. Rather than code changes, what this patch needs is more
community interest. You might

- ask people for help with your approach,
- see if there are similar patches that your code could supplement,
- get interested parties to agree to review your patch in a CF, or
- possibly present the functionality in a way that's easier to review
  overall.

(Doing these things is no guarantee that there will be interest, but
it's hopefully better than endlessly rebasing a patchset that is not
receiving any feedback from the community.)

Once you think you've built up some community support and the patchset
is ready for review, you (or any interested party) can resurrect the
patch entry by visiting

    https://commitfest.postgresql.org/38/3256/

and changing the status to "Needs Review", and then changing the
status again to "Move to next CF". (Don't forget the second step;
hopefully we will have streamlined this in the near future!)

Thanks,
--Jacob

[1] https://postgr.es/m/86140760-8ba5-6f3a-3e6e-5ca6c060bd24@timescale.com

Rebased on c727f511b.

This patch record was "closed for lack of interest", but I think what's
actually needed is committer review of which approach to take.

 - add backend functions but do not modify psql ?
 - add to psql slash-plus commnds ?
 - introduce psql double-plus commands for new options ?
 - change pre-existing psql plus commands to only show size with
   double-plus ?
 - go back to the original, two-line client-side sum() ?

Until then, the patchset is organized with those questions in mind.

-- 
Justin

On Thu, Dec 15, 2022 at 10:13:23AM -0600, Justin Pryzby wrote:
> Rebased on c727f511b.

Rebased on 30a53b792.
With minor changes including fixes to an intermediate patch.

> This patch record was "closed for lack of interest", but I think what's
> actually needed is committer review of which approach to take.
> 
>  - add backend functions but do not modify psql ?
>  - add to psql slash-plus commnds ?
>  - introduce psql double-plus commands for new options ?
>  - change pre-existing psql plus commands to only show size with
>    double-plus ?
>  - go back to the original, two-line client-side sum() ?
> 
> Until then, the patchset is organized with those questions in mind.

I added documentation for the SQL functions in 001.
And updated to say 170000

I'm planning to set this patch as ready - it has not changed
significantly in 18 months.  Not for the first time, I've implemented a
workaround at a higher layer.

-- 
Justin

> On 24 May 2023, at 23:05, Justin Pryzby <pryzby@telsasoft.com> wrote:

> I'm planning to set this patch as ready

This is marked RfC so I'm moving this to the next CF, but the patch no longer
applies so it needs a rebase.

--
Daniel Gustafsson

On Thu, Dec 15, 2022 at 10:13:23AM -0600, Justin Pryzby wrote:
> This patch record was "closed for lack of interest", but I think what's
> actually needed is committer review of which approach to take.

On Tue, Aug 01, 2023 at 09:54:34AM +0200, Daniel Gustafsson wrote:
> > On 24 May 2023, at 23:05, Justin Pryzby <pryzby@telsasoft.com> wrote:
> 
> > I'm planning to set this patch as ready
> 
> This is marked RfC so I'm moving this to the next CF, but the patch no longer
> applies so it needs a rebase.

I was still hoping to receive some feedback on which patches to squish.

-- 
Justin

2024-01 Commitfest.

Hi, This patch had a CF status of "Ready for Committer", but the
thread has been inactive for 5+ months.

Since the last post from Justin said "hoping to receive some feedback"
I have changed the CF status back to "Needs Review" [1].

======
[1] https://commitfest.postgresql.org/46/3256/

Kind Regards,
Peter Smith.

Hi Justin,

Thanks for the patch and the work on it.  In reviewing the basic
feature, I think this is something that has utility and is worthwhile
at the high level.

A few more specific notes:

The pg_namespace_size() function can stand on its own, and probably
has some utility for the released Postgres versions.

I do think the psql implementation for the \dn+ or \dA+ commands
shouldn't need to use this same function; it's a straightforward
expansion of the SQL query that can be run in a way that will be
backwards-compatible with any connected postgres version, so no reason
to exclude this information for this cases.  (This may have been in an
earlier revision of the patchset; I didn't check everything.)

I think the \dX++ command versions add code complexity without a real
need for it.  We have precedence with \l(+) to show permissions on the
basic display and size on the extended display, and I think this is
sufficient in this case here.  While moving the permissions to \dn is
a behavior change, it's adding information, not taking it away, and as
an interactive command it is unlikely to introduce significant
breakage in any scripting scenario.

(In reviewing the patch we've also seen a bit of odd behavior/possible
bug with the existing extended + commands, which introducing
significant ++ overloading might be confusing, but not the
fault/concern of this patch.)

Quickie summary:

0001-Add-pg_am_size-pg_namespace_size.patch
- fine, but needs rebase to work
0002-psql-add-convenience-commands-dA-and-dn.patch
- split into just + variant; remove \l++
- make the \dn show permission and \dn+ show size
0003-f-convert-the-other-verbose-to-int-too.patch
- unneeded
0004-Move-the-double-plus-Size-columns-to-the-right.patch
- unneeded

Docs on the first patch seemed fine; I do think we'll need docs
changes for the psql changes.

Best,

David

On Thu, May 30, 2024 at 10:59:06AM -0700, David Christensen wrote:
> Hi Justin,
> 
> Thanks for the patch and the work on it.  In reviewing the basic
> feature, I think this is something that has utility and is worthwhile
> at the high level.

Thanks for looking.

> A few more specific notes:
> 
> The pg_namespace_size() function can stand on its own, and probably
> has some utility for the released Postgres versions.

Are you suggesting to add the C function retroactively in back branches?
I don't think anybody would consider doing that.

It wouldn't be used by anything internally, and any module that wanted
to use it would have to check the minor version, instead of just the
major version, which is wrong.

> I do think the psql implementation for the \dn+ or \dA+ commands
> shouldn't need to use this same function; it's a straightforward
> expansion of the SQL query that can be run in a way that will be
> backwards-compatible with any connected postgres version, so no reason
> to exclude this information for this cases.  (This may have been in an
> earlier revision of the patchset; I didn't check everything.)

I think you're suggesting to write the query in SQL rather than in C.

But I did that in the first version of the patch, and the response was
that maybe in the future someone would want to add permission checks
that would compromize the ability to get correct results from SQL, so
then I presented the functionality writen in C.

I recommend that reviewers try to read the existing communication on the
thread, otherwise we end up going back and forth about the same things.

> I think the \dX++ command versions add code complexity without a real
> need for it.

If you view this as a way to "show schema sizes", then you're right,
there's no need.  But I don't want this patch to necessary further
embrace the idea that it's okay for "plus commands to be slow and show
nonportable results".  If there were a consensus that it'd be fine in a
plus command, I would be okay with that, though.

> We have precedence with \l(+) to show permissions on the
> basic display and size on the extended display, and I think this is
> sufficient in this case here.

You also have the precedence that \db doesn't show the ACL, and you
can't get it without also computing the sizes.  That's 1) inconsistent
with \l and 2) pretty inconvenient for someone who wants to show the
ACL (as mentioned in the first message on this thread).

> 0001-Add-pg_am_size-pg_namespace_size.patch
> - fine, but needs rebase to work

I suggest reviewers to consider sending a rebased patch, optionally with
any proposed changes in a separate patch.

> 0002-psql-add-convenience-commands-dA-and-dn.patch
> - split into just + variant; remove \l++
> - make the \dn show permission and \dn+ show size

> 0003-f-convert-the-other-verbose-to-int-too.patch
> - unneeded
> 0004-Move-the-double-plus-Size-columns-to-the-right.patch
> - unneeded

You say they're unneeded, but what I've been hoping for is a committer
interested enough to at least suggest whether to run with 001, 001+002,
001+002+003, or 001+002+003+004.  They're intentionally presented as
such.

I've also thought about submitting a patch specifically dedicated to
"moving size out of + and into ++".  I find the idea compelling, for the
reasons I wrote in the the patch description.  That'd be like presenting
003+004 first.

I'm opened to changing the behavior or the implementation.  But changing
the patch as I've presented it based on one suggestion I think will lead
to incoherent code trashing.  I need to hear a wider agreement.

-- 
Justin

On Mon, Jun 3, 2024 at 9:10 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, May 30, 2024 at 10:59:06AM -0700, David Christensen wrote:
> > Hi Justin,
> >
> > Thanks for the patch and the work on it.  In reviewing the basic
> > feature, I think this is something that has utility and is worthwhile
> > at the high level.
>
> Thanks for looking.
>
> > A few more specific notes:
> >
> > The pg_namespace_size() function can stand on its own, and probably
> > has some utility for the released Postgres versions.
>
> Are you suggesting to add the C function retroactively in back branches?
> I don't think anybody would consider doing that.
>
> It wouldn't be used by anything internally, and any module that wanted
> to use it would have to check the minor version, instead of just the
> major version, which is wrong.

Ah, I meant once released it would be useful going forward, but
re-reading it can see the ambiguity.  No, definitely not suggesting
adding to back branches.

> > I do think the psql implementation for the \dn+ or \dA+ commands
> > shouldn't need to use this same function; it's a straightforward
> > expansion of the SQL query that can be run in a way that will be
> > backwards-compatible with any connected postgres version, so no reason
> > to exclude this information for this cases.  (This may have been in an
> > earlier revision of the patchset; I didn't check everything.)
>
> I think you're suggesting to write the query in SQL rather than in C.

Yes.

> But I did that in the first version of the patch, and the response was
> that maybe in the future someone would want to add permission checks
> that would compromize the ability to get correct results from SQL, so
> then I presented the functionality writen in C.
>
> I recommend that reviewers try to read the existing communication on the
> thread, otherwise we end up going back and forth about the same things.

Yes, I reviewed the whole thread, just not all of the patch contents.
I'd agree that suggestion flapping is not useful, but just presenting
my take on this at this time (as well as several others in the Patch
Review workshop at PGConf.dev, which is where this one got revived).

> > I think the \dX++ command versions add code complexity without a real
> > need for it.
>
> If you view this as a way to "show schema sizes", then you're right,
> there's no need.  But I don't want this patch to necessary further
> embrace the idea that it's okay for "plus commands to be slow and show
> nonportable results".  If there were a consensus that it'd be fine in a
> plus command, I would be okay with that, though.

I think this is a separate discussion in terms of introducing
verbosity levels and not needed at this point for this feature.
Certainly revamping all inconsistencies with the psql command
interfaces is out of scope, just thinking about which one we'd want to
model going forward and providing an opinion there (for what it's
worth... :D)

> > We have precedence with \l(+) to show permissions on the
> > basic display and size on the extended display, and I think this is
> > sufficient in this case here.
>
> You also have the precedence that \db doesn't show the ACL, and you
> can't get it without also computing the sizes.  That's 1) inconsistent
> with \l and 2) pretty inconvenient for someone who wants to show the
> ACL (as mentioned in the first message on this thread).

I can't speak to this one, just think that adding more command options
adds to the overall complexity so probably to be avoided unless we
can't.

> > 0001-Add-pg_am_size-pg_namespace_size.patch
> > - fine, but needs rebase to work
>
> I suggest reviewers to consider sending a rebased patch, optionally with
> any proposed changes in a separate patch.

Sure, if you don't have time to do this; agree with your later remarks
that consensus is important before moving forward, so same applies
here.

> > 0002-psql-add-convenience-commands-dA-and-dn.patch
> > - split into just + variant; remove \l++
> > - make the \dn show permission and \dn+ show size
>
> > 0003-f-convert-the-other-verbose-to-int-too.patch
> > - unneeded
> > 0004-Move-the-double-plus-Size-columns-to-the-right.patch
> > - unneeded
>
> You say they're unneeded, but what I've been hoping for is a committer
> interested enough to at least suggest whether to run with 001, 001+002,
> 001+002+003, or 001+002+003+004.  They're intentionally presented as
> such.
>
> I've also thought about submitting a patch specifically dedicated to
> "moving size out of + and into ++".  I find the idea compelling, for the
> reasons I wrote in the the patch description.  That'd be like presenting
> 003+004 first.
>
> I'm opened to changing the behavior or the implementation.  But changing
> the patch as I've presented it based on one suggestion I think will lead
> to incoherent code trashing.  I need to hear a wider agreement.

Agreed that some sort of consensus is important.