Re: [PATCH] psql: \dn+ to show size of each schema (and \dA+ for AMs) - Mailing list pgsql-hackers

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.