Thread: Remove source code display from \df+?

I find \df+ much less useful than it should be because it tends to be cluttered up with source code. Now that we have \sf, would it be reasonable to remove the source code from the \df+ display? This would make it easier to see function permissions and comments. If somebody wants to see the full definition of a function they can always invoke \sf on it.

If there is consensus on the idea in principle I will write up a patch.

st 11. 1. 2023 v 17:50 odesílatel Isaac Morland <isaac.morland@gmail.com> napsal:

I find \df+ much less useful than it should be because it tends to be cluttered up with source code. Now that we have \sf, would it be reasonable to remove the source code from the \df+ display? This would make it easier to see function permissions and comments. If somebody wants to see the full definition of a function they can always invoke \sf on it.

If there is consensus on the idea in principle I will write up a patch.

+1

On Wed, Jan 11, 2023 at 6:19 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:

st 11. 1. 2023 v 17:50 odesílatel Isaac Morland <isaac.morland@gmail.com> napsal:

I find \df+ much less useful than it should be because it tends to be cluttered up with source code. Now that we have \sf, would it be reasonable to remove the source code from the \df+ display? This would make it easier to see function permissions and comments. If somebody wants to see the full definition of a function they can always invoke \sf on it.

If there is consensus on the idea in principle I will write up a patch.

+1

+1 but maybe with a twist. For any functions in a procedural language like plpgsql, it makes it pretty useless today. But when viewing an internal or C language function, it's short enough and still actually useful. Maybe some combination where it would keep showing those for such language, but would show "use \sf to view source" for procedural languages?

 

--

 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

Right, for internal or C functions it just gives a symbol name or something similar. I've never been annoyed seeing that, just having pages of PL/PGSQL (I use a lot of that, possibly biased towards the “too much” direction) take up all the space.

A bit hacky, but what about only showing the first line of the source code? Then you would see link names for that type of function but the main benefit of suppressing the full source code would be obtained. Or, show source if it is a single line, otherwise “…” (as in, literally an ellipsis).

On Wed, 11 Jan 2023 at 12:31, Pavel Stehule <pavel.stehule@gmail.com> wrote:

st 11. 1. 2023 v 18:25 odesílatel Magnus Hagander <magnus@hagander.net> napsal:

On Wed, Jan 11, 2023 at 6:19 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:

st 11. 1. 2023 v 17:50 odesílatel Isaac Morland <isaac.morland@gmail.com> napsal:

I find \df+ much less useful than it should be because it tends to be cluttered up with source code. Now that we have \sf, would it be reasonable to remove the source code from the \df+ display? This would make it easier to see function permissions and comments. If somebody wants to see the full definition of a function they can always invoke \sf on it.

If there is consensus on the idea in principle I will write up a patch.

+1

+1 but maybe with a twist. For any functions in a procedural language like plpgsql, it makes it pretty useless today. But when viewing an internal or C language function, it's short enough and still actually useful. Maybe some combination where it would keep showing those for such language, but would show "use \sf to view source" for procedural languages?

yes, it is almost necessary for C functions or functions in external languages. Maybe it can be specified in pg_language if prosrc is really source code or some reference.

 

--

 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

please, don't send top post replies - https://en.wikipedia.org/wiki/Posting_style

I don't think printing a few first rows is a good idea - usually there is nothing interesting (same is PL/Perl, PL/Python, ...)

If the proposed feature can be generic, then this information should be stored somewhere in pg_language. Or we can redesign usage of prosrc and probin columns - but this can be a much more massive change.

On Wed, 11 Jan 2023 at 13:11, Pavel Stehule <pavel.stehule@gmail.com> wrote:

please, don't send top post replies - https://en.wikipedia.org/wiki/Posting_style

Sorry about that; I do know to do it properly and usually get it right. GMail doesn’t seem to have an option (that I can find) to leave no space at the top and put my cursor at the bottom; it nudges pretty firmly in the direction of top-posting. Thanks for the reminder.

 

I don't think printing a few first rows is a good idea - usually there is nothing interesting (same is PL/Perl, PL/Python, ...)

If the proposed feature can be generic, then this information should be stored somewhere in pg_language. Or we can redesign usage of prosrc and probin columns - but this can be a much more massive change.

I’m looking for a quick win. So I think that means either drop the source column entirely, or show single-line source values only and nothing or a placeholder for anything that is more than one line, unless somebody comes up with another suggestion. Originally I was thinking just to remove entirely, but I’ve seen a couple of comments suggesting that people would find it unfortunate if the source weren’t shown for internal/C functions, so now I’m leaning towards showing single-line values only.

I agree that showing the first line or couple of lines isn't likely to be very useful. The way I format my functions, the first line is always blank anyway: I write the bodies like this:

$$

BEGIN

    …

END;

$$;

Even if somebody uses a different style, the first line is probably just "BEGIN" or something equally formulaic.

On Wed, Jan 11, 2023 at 7:24 PM Isaac Morland <isaac.morland@gmail.com> wrote:

On Wed, 11 Jan 2023 at 13:11, Pavel Stehule <pavel.stehule@gmail.com> wrote:

please, don't send top post replies - https://en.wikipedia.org/wiki/Posting_style

Sorry about that; I do know to do it properly and usually get it right. GMail doesn’t seem to have an option (that I can find) to leave no space at the top and put my cursor at the bottom; it nudges pretty firmly in the direction of top-posting. Thanks for the reminder.

 

I don't think printing a few first rows is a good idea - usually there is nothing interesting (same is PL/Perl, PL/Python, ...)

If the proposed feature can be generic, then this information should be stored somewhere in pg_language. Or we can redesign usage of prosrc and probin columns - but this can be a much more massive change.

I’m looking for a quick win. So I think that means either drop the source column entirely, or show single-line source values only and nothing or a placeholder for anything that is more than one line, unless somebody comes up with another suggestion. Originally I was thinking just to remove entirely, but I’ve seen a couple of comments suggesting that people would find it unfortunate if the source weren’t shown for internal/C functions, so now I’m leaning towards showing single-line values only.

I agree that showing the first line or couple of lines isn't likely to be very useful. The way I format my functions, the first line is always blank anyway: I write the bodies like this:

$$

BEGIN

    …

END;

$$;

Even if somebody uses a different style, the first line is probably just "BEGIN" or something equally formulaic.

This is only about Internal and C, isn't it? Isn't the oid of these static, and identified by INTERNALlanguageId and ClanguageId respectively? So we could just have the query show the prosrc column if the language oid is one of those two, and otherwise show "Please use \sf to view the source"? 

--

 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

Pavel Stehule <pavel.stehule@gmail.com> writes:
> st 11. 1. 2023 v 19:31 odesílatel Magnus Hagander <magnus@hagander.net>
> napsal:
>> This is only about Internal and C, isn't it? Isn't the oid of these
>> static, and identified by INTERNALlanguageId and ClanguageId respectively?
>> So we could just have the query show the prosrc column if the language oid
>> is one of those two, and otherwise show "Please use \sf to view the
>> source"?

> I think it can be acceptable - maybe we can rename the column "source code"
> like "internal name" or some like that.

Yeah, "source code" has always been kind of a misnomer for these
languages.

> again I don't think printing  "Please use \sf to view the source"? " often
> can be user friendly.  \? is clear and \sf is easy to use

We could shorten it to "See \sf" or something like that.  But if we change
the column header to "internal name" or the like, then the column just
obviously doesn't apply for non-internal languages, so leaving it null
should be fine.

                        regards, tom lane

st 11. 1. 2023 v 22:11 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Pavel Stehule <pavel.stehule@gmail.com> writes:
> st 11. 1. 2023 v 19:31 odesílatel Magnus Hagander <magnus@hagander.net>
> napsal:
>> This is only about Internal and C, isn't it? Isn't the oid of these
>> static, and identified by INTERNALlanguageId and ClanguageId respectively?
>> So we could just have the query show the prosrc column if the language oid
>> is one of those two, and otherwise show "Please use \sf to view the
>> source"?

> I think it can be acceptable - maybe we can rename the column "source code"
> like "internal name" or some like that.

Yeah, "source code" has always been kind of a misnomer for these
languages.

> again I don't think printing  "Please use \sf to view the source"? " often
> can be user friendly.  \? is clear and \sf is easy to use

We could shorten it to "See \sf" or something like that.  But if we change
the column header to "internal name" or the like, then the column just
obviously doesn't apply for non-internal languages, so leaving it null
should be fine.

+1

We could shorten it to "See \sf" or something like that.  But if we change
the column header to "internal name" or the like, then the column just
obviously doesn't apply for non-internal languages, so leaving it null
should be fine.

+1

Sure, that works for me as well. I agree the suggested text was way too long, I was more thinking of "something in this direction" rather than that exact text. But yes, with a change of names, we can leave it NULL as well. 

Thanks everybody. So based on the latest discussion I will:

1) rename the column from “Source code” to “Internal name”; and
2) change the contents to NULL except when the language (identified by oid) is INTERNAL or C.

Patch forthcoming, I hope.

On Thu, 12 Jan 2023 at 12:06, Isaac Morland <isaac.morland@gmail.com> wrote:

Thanks everybody. So based on the latest discussion I will:

1) rename the column from “Source code” to “Internal name”; and
2) change the contents to NULL except when the language (identified by oid) is INTERNAL or C.

Patch forthcoming, I hope.

I've attached a patch for this. It turns out to simplify the existing code in one way because the recently added call to pg_get_function_sqlbody() is no longer needed since it applies only to SQL functions, which will now display as a blank column.

I implemented the change and was surprised to see that no tests failed. Turns out that while there are several tests for \df, there were none for \df+. I added a couple, just using \df+ on some functions that appear to me to be present on plain vanilla Postgres.

I was initially concerned about translation support for the column heading, but it turns out that \dT+ already has a column with the exact same name so it appears we don’t need any new translations.

I welcome comments and feedback. Now to try to find something manageable to review.

út 17. 1. 2023 v 20:29 odesílatel Isaac Morland <isaac.morland@gmail.com> napsal:

I welcome comments and feedback. Now to try to find something manageable to review.

looks well

you miss update psql documentation

https://www.postgresql.org/docs/current/app-psql.html

If the form \df+ is used, additional information about each function is shown, including volatility, parallel safety, owner, security classification, access privileges, language, source code and description.

you should to assign your patch to commitfest app

https://commitfest.postgresql.org/

On Wed, 18 Jan 2023 at 00:00, Pavel Stehule <pavel.stehule@gmail.com> wrote:

út 17. 1. 2023 v 20:29 odesílatel Isaac Morland <isaac.morland@gmail.com> napsal:

I welcome comments and feedback. Now to try to find something manageable to review.

looks well

you miss update psql documentation

https://www.postgresql.org/docs/current/app-psql.html

If the form \df+ is used, additional information about each function is shown, including volatility, parallel safety, owner, security classification, access privileges, language, source code and description.

Thanks, and sorry about that, it just completely slipped my mind. I’ve attached a revised patch with a slight update of the psql documentation. 

you should to assign your patch to commitfest app

https://commitfest.postgresql.org/

I thought I had: https://commitfest.postgresql.org/42/4133/

Did I miss something?

On Wed, Jan 18, 2023 at 10:27:46AM -0500, Isaac Morland wrote:
>
> I thought I had: https://commitfest.postgresql.org/42/4133/

This is failing tests:
http://cfbot.cputube.org/isaac-morland.html

It seems like any "make check" would fail .. but did you also try
cirrusci from your own github account?
./src/tools/ci/README

BTW, I think "ELSE NULL" could be omitted.

On Thu, 19 Jan 2023 at 11:30, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Wed, Jan 18, 2023 at 10:27:46AM -0500, Isaac Morland wrote:
>
> I thought I had: https://commitfest.postgresql.org/42/4133/

This is failing tests:
http://cfbot.cputube.org/isaac-morland.html

It seems like any "make check" would fail .. but did you also try
cirrusci from your own github account?
./src/tools/ci/README

I definitely ran "make check" but I did not realize there is also cirrusci. I will look at that. I'm having trouble interpreting the cfbot page to which you linked but I'll try to run cirrusci myself before worrying too much about that.

Running "make check" the first time I was surprised to see no failures - so I added tests for \df+, which passed when I did "make check".

BTW, I think "ELSE NULL" could be omitted.

Looks like; I'll update. Might as well reduce the visual size of the code.

On Sun, Jan 22, 2023 at 12:18:34AM -0500, Isaac Morland wrote:

> It turns out that my tests wanted the owner to be “vagrant” rather than
> “postgres”. This is apparently because I was running as that user (in a
> Vagrant VM) when running the tests. Then I took that output and just made
> it the expected output. I’ve re-worked my build environment a bit so that I
> run as “postgres” inside the Vagrant VM.
>
> What I don’t understand is why that didn’t break all the other tests.

Probably because the other tests avoid showing the owner, exactly
because it varies depending on who runs the tests.  Most of the "plus"
commands aren't tested, at least in the sql regression tests.

We should probably change one of the CI usernames to something other
than "postgres" to catch the case that someone hardcodes "postgres".

> proper value in the Owner column so let’s see what CI does with it.

Or better: see above about using it from your github account.

On 2023-Jan-22, Isaac Morland wrote:

> I’ve re-written the tests to create a test-specific role and functions so
> there is no longer a dependency on the superuser name.

This one would fail the sanity check that all roles created by
regression tests need to have names that start with "regress_".

> I pondered the notion of going with the flow and just leaving out the
> tests but that seemed like giving up too easily.

I think avoiding even more untested code is good, so +1 for keeping at
it.

Isaac Morland <isaac.morland@gmail.com> writes:
> On Sun, 22 Jan 2023 at 14:26, Alvaro Herrera <alvherre@alvh.no-ip.org>
> wrote:
>> This one would fail the sanity check that all roles created by
>> regression tests need to have names that start with "regress_".

> Thanks for the correction. Now I feel like I've skipped some of the
> readings!
> Updated patch attached. Informally, I am adopting the regress_* policy for
> all object types.

That's excessive.  The policy Alvaro mentions applies to globally-visible
object names (i.e., database, role, and tablespace names), and it's there
to try to ensure that doing "make installcheck" against a live
installation won't clobber any non-test-created objects.  There's no point
in having such a policy within a test database --- its most likely effect
there would be to increase the risk that different test scripts step on
each others' toes.  If you feel a need for a name prefix for non-global
objects, use something based on the name of your test script.

On Sun, Jan 22, 2023 at 03:04:14PM -0500, Tom Lane wrote:

> That's excessive.  The policy Alvaro mentions applies to globally-visible
> object names (i.e., database, role, and tablespace names), and it's there
> to try to ensure that doing "make installcheck" against a live
> installation won't clobber any non-test-created objects.  There's no point
> in having such a policy within a test database --- its most likely effect
> there would be to increase the risk that different test scripts step on
> each others' toes.  If you feel a need for a name prefix for non-global
> objects, use something based on the name of your test script.

But we *are* talking about the role to be created to allow stable output
of \df+ , so it's necessary to name it "regress_*".  To appease
ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS, and to avoid clobbering
global objects during "installcheck".

On Sun, Jan 22, 2023 at 04:28:21PM -0500, Isaac Morland wrote:
> On Sun, 22 Jan 2023 at 15:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> But now I'm having a problem I don't understand: the CI are still failling,
> but not in the psql test. Instead, I get this:
>
> [20:11:17.624] +++ tap check in src/bin/pg_upgrade +++

You'll find the diff in the "artifacts", but not a separate "diff" file.
https://api.cirrus-ci.com/v1/artifact/task/6146418377752576/testrun/build/testrun/pg_upgrade/002_pg_upgrade/log/regress_log_002_pg_upgrade

 CREATE FUNCTION public.regress_psql_df_sql() RETURNS void
     LANGUAGE sql
     BEGIN ATOMIC
- SELECT NULL::text;
+ SELECT NULL::text AS text;
 END;

It's failing because after restoring the function, the column is named
"text" - maybe it's a bug.

Tom's earlier point was that neither the function nor its owner needs to
be preserved (as is done to exercise pg_dump/restore/upgrade - surely
functions are already tested).  Dropping it when you're done running \df
will avoid any possible issue with pg_upgrade.

Were you able to test with your own github account ?

On Sun, Jan 22, 2023 at 08:23:25PM -0500, Isaac Morland wrote:
> > Were you able to test with your own github account ?
>
> I haven’t had a chance to try this. I must confess to being a bit confused
> by the distinction between running the CI tests and doing "make check";
> ideally I would like to be able to run all the tests on my own machine
> without any external resources. But at the same time I don’t pretend to
> understand the full situation so I will try to use this when I get some
> time.

First: "make check" only runs the sql tests, and not the perl tests
(including pg_upgrade) or isolation tests.  check-world runs everything.

Isaac Morland <isaac.morland@gmail.com> writes:
> [ 0001-Remove-source-code-display-from-df-v6.patch ]

Pushed after some editorialization on the test case.

One thing I noticed while testing is that if you apply \df+ to an
aggregate function, it will show "Internal name" of "aggregate_dummy".
While that's an accurate description of what's in prosrc, it seems
not especially useful and perhaps indeed confusing to novices.
So I thought about suppressing it.  However, that would require
a server-version-dependent test and I wasn't quite convinced it'd
be worth the trouble.  Any thoughts on that?