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