Thread: [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
st 14. 7. 2021 v 7:42 odesílatel Laurenz Albe <laurenz.albe@cybertec.at> napsal:
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)?
+1 good idea
Pavel
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
Attachment
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
Attachment
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
pá 17. 9. 2021 v 11:10 odesílatel Justin Pryzby <pryzby@telsasoft.com> napsal:
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)
I tested this patch. It looks well. The performance is good enough. I got the result for a schema with 100K tables in 3 seconds.
I am not sure if using \dt+ and \dP+ without change is a good idea. I can imagine \dt+ and \dt++. \dP can exist just only in ++ form or we can ignore it (like now, and support \dP+ and \dP++) with same result
I can live with the proposed patch, and I understand why ++ was introduced. But I am still not sure it is really user friendly. I prefer to extend \dA and \dn with some columns (\dA has only two columns and \dn has two columns too), and then we don't need special ++ variants for sizes. Using three levels of detail looks not too practical (more when the basic reports \dA and \dn) are really very simple).
Regards
Pavel
--
Justin
út 28. 9. 2021 v 4:46 odesílatel Justin Pryzby <pryzby@telsasoft.com> napsal:
On Fri, Sep 17, 2021 at 12:05:04PM +0200, Pavel Stehule wrote:
> I can live with the proposed patch, and I understand why ++ was
> introduced. But I am still not sure it is really user friendly. I prefer to
> extend \dA and \dn with some columns (\dA has only two columns and \dn has
> two columns too), and then we don't need special ++ variants for sizes.
> Using three levels of detail looks not too practical (more when the basic
> reports \dA and \dn) are really very simple).
You're suggesting to include the ACL+description in \dn and handler+description
and \dA.
yes
Another option is to add pg_schema_size() and pg_am_size() without shortcuts in
psql. That would avoid showing a potentially huge ACL when all one wants is
the schema size, and would serve my purposes well enough to write
| SELECT pg_namespace_size(oid), nspname FROM pg_namespace ORDER BY 1 DESC LIMIT 9;
It can work too.
I think the long ACL is a customer design issue, but can be. But the same problem is in \dt+, and I don't see an objection against this design.
Maybe I am too subjective, because 4 years I use pspg, and wide reports are not a problem for me. When the size is on the end, then it is easy to see it in pspg.
I like to see size in \dn+ report, and I like to use pg_namespace_size separately too. Both can be very practical functionality.
I think so \dt+ and \l+ is working very well now, and I am not too happy to break it (partially break it). Although the proposed change is very minimalistic.
But your example "SELECT pg_namespace_size(oid), nspname FROM pg_namespace ORDER BY 1 DESC LIMIT 9" navigates me to the second idea (that just enhances the previous). Can be nice if you can have prepared views on the server side that are +/- equivalent to psql reports, and anybody can simply write their own custom reports.
some like
SELECT schema, tablename, owner, pg_size_pretty(size) FROM pg_description.tables ORDER BY size DESC LIMIT 10
SELECT schema, owner, pg_size_pretty(size) FROM pg_description.schemas ORDER BY size DESC LIMIT 10
In the future, it can simplify psql code, and it allows pretty nice customization in any client for a lot of purposes.
Regards
Pavel
--
Justin
Hi
I like this feature, but I don't like the introduction of double + too much. I think it is confusing.
Is it really necessary? Cannot be enough just reorganization of \dn and \dn+.
Regards
Pavel
pá 14. 1. 2022 v 17:35 odesílatel Justin Pryzby <pryzby@telsasoft.com> napsal:
Rebased before Julian asks.
Rebased
Attachment
rebased
Attachment
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
Attachment
čt 15. 12. 2022 v 17:13 odesílatel Justin Pryzby <pryzby@telsasoft.com> napsal:
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.
+1
This format makes sense to me.
Regards
Pavel
--
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.
Attachment
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
Attachment
> 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
Attachment
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
Attachment
po 3. 6. 2024 v 16:10 odesílatel Justin Pryzby <pryzby@telsasoft.com> napsal:
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.
I think showing size in \dX+ command is consistent with any other + commands and the introduction ++ variant is inconsistent and not too intuitive.
So I personally vote just for \dX+ without the introduction ++ command. Any time, in this case we can introduce ++ in future when we see some performance problems.
> 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.