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

po 3. 6. 2024 v 16:10 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)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
>