po 3. 6. 2024 v 16:10 odesÃlatel Justin Pryzby <pry...@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 >
