Remove source code display from \df+?

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

Pavel

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/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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.

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:

Hi

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

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).

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.

Regards

Pavel

Or, only show the source in \df++. But it'd be a bit unfortunate if the
C language function wasn't shown in \df+

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.

<http://www.redpill-linpro.com/&gt;

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.

<http://www.redpill-linpro.com/&gt;

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/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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

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.

<http://www.redpill-linpro.com/&gt;

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"?

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

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

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

On Thu, Jan 12, 2023 at 6:23 AM Pavel Stehule <pavel.stehule@gmail.com>
wrote:

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

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.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On Thu, 12 Jan 2023 at 10:04, Magnus Hagander <magnus@hagander.net> wrote:

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.

Hi

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

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.

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/

Regards

Pavel

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?

st 18. 1. 2023 v 16:27 odesílatel Isaac Morland <isaac.morland@gmail.com>
napsal:

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/

ok

Did I miss something?

it looks well

regards

Pavel

On Wed, Jan 18, 2023 at 10:27:46AM -0500, Isaac Morland wrote:

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/

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.

--
Justin

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 Thu, 19 Jan 2023 at 13:02, Isaac Morland <isaac.morland@gmail.com> wrote:

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".

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. I
would have expected “postgres” to show up all over the expected results and
be changed to “vagrant” by what I did; so I should have seen all sorts of
test failures in the existing tests. Anyway, my new tests now have the
proper value in the Owner column so let’s see what CI does with it.

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

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

I did this. I’m ambivalent about this because I usually think of CASE and
similar constructs as needing to explicitly cover all possible cases but
the language does provide for the NULL default case so may as well use the
feature where applicable.

Isaac Morland <isaac.morland@gmail.com> writes:

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.

Nope, that is not going to get past the buildfarm (hint: a lot of the
BF animals run under "buildfarm" or some similar username). You have
to make sure that your tests do not care what the name of the bootstrap
superuser is.

What I don’t understand is why that didn’t break all the other tests.

Because all the committed tests are independent of that.

regards, tom lane

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

On Thu, 19 Jan 2023 at 13:02, Isaac Morland <isaac.morland@gmail.com> wrote:

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".

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.

--
Justin

On Sun, 22 Jan 2023 at 00:45, Justin Pryzby <pryzby@telsasoft.com> wrote:

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.

Thanks for your patience. I didn’t think about it enough but everything you
both said makes sense.

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. I pondered the
notion of going with the flow and just leaving out the tests but that
seemed like giving up too easily.

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.

Yes, I will try to get this working before I try to make another
contribution.

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.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"At least to kernel hackers, who really are human, despite occasional
rumors to the contrary" (LWN.net)

On Sun, 22 Jan 2023 at 14:26, Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

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_".

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.

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.

regards, tom lane

On Sun, 22 Jan 2023 at 15:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

I already used a test-specific prefix, then added "regress_" in front.
Point taken, however, on the difference between global and non-global
objects.

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 +++
[20:11:17.624] [20:09:11] t/001_basic.pl ....... ok 106 ms ( 0.00 usr
0.00 sys + 0.06 cusr 0.02 csys = 0.08 CPU)
[20:11:17.624]
[20:11:17.624] # Failed test 'old and new dumps match after pg_upgrade'
[20:11:17.624] # at t/002_pg_upgrade.pl line 362.
[20:11:17.624] # got: '1'
[20:11:17.624] # expected: '0'
[20:11:17.624] # Looks like you failed 1 test of 13.
[20:11:17.624] [20:11:17] t/002_pg_upgrade.pl ..
[20:11:17.624] Dubious, test returned 1 (wstat 256, 0x100)
[20:11:17.624] Failed 1/13 subtests
[20:11:17.624] [20:11:17]
[20:11:17.624]
[20:11:17.624] Test Summary Report
[20:11:17.624] -------------------
[20:11:17.624] t/002_pg_upgrade.pl (Wstat: 256 Tests: 13 Failed: 1)
[20:11:17.624] Failed test: 13
[20:11:17.624] Non-zero exit status: 1
[20:11:17.624] Files=2, Tests=21, 126 wallclock secs ( 0.01 usr 0.00 sys +
6.65 cusr 3.95 csys = 10.61 CPU)
[20:11:17.624] Result: FAIL
[20:11:17.624] make[2]: *** [Makefile:55: check] Error 1
[20:11:17.625] make[1]: *** [Makefile:43: check-pg_upgrade-recurse] Error 2

As far as I can tell this is the only failure and doesn’t have anything to
do with my change. Unless the objects I added are messing it up? Unlike
when the psql regression test was failing, I don’t see an indication of
where I can see the diffs.

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

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.

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".

--
Justin

On Sun, 22 Jan 2023 at 16:56, Justin Pryzby <pryzby@telsasoft.com> wrote:

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".

Tom is talking about my informal policy of prefixing all objects. Only
global objects need to be prefixed with regress_, but I prefixed everything
I created (functions as well as the role). I actually called the
role regress_psql_df and used that entire role name as the prefix of my
function names, so I think it unlikely that I’ll collide with anything else.

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:

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.

I already used a test-specific prefix, then added "regress_" in front.
Point taken, however, on the difference between global and non-global
objects.

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 ?

--
Justin

On Sun, 22 Jan 2023 at 17:27, Justin Pryzby <pryzby@telsasoft.com> wrote:

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.

OK, thanks. I'd say I've uncovered a bug related to pg_upgrade, unless I’m
missing something. However, I've adjusted my patch so that nothing it
creates is kept. This seems tidier even without the test failure.

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 ?

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.

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.

One difference from running it locally is that cirrus runs tests under
four OSes. Another is that it has a bunch of compilation flags and
variations to help catch errors (although it's currently missing
ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS, so that wouldn't have been
caught). And another reason is that it runs in a "clean" environment,
so (for example) it'd probably catch if you have local, uncommited
changes, or if you assumed that the username is "postgres" (earlier I
said that it didn't, but actually the mac task runs as "admin").

The old way of doing things was for cfbot to "inject" the cirrus.yml
file and then push a branch to cirrusci to run tests; it made some sense
for people to mail a patch to the list to cause cfbot to run the tests
under cirrusci. The current/new way is that .cirrus.yml is in the
source tree, so anyone with a github account can do that. IMO it no
longer makes sense to send patches to the list "to see" if it passes
tests. I encouraging those who haven't to try it.

--
Justin

On Sun, 22 Jan 2023 at 21:37, Justin Pryzby <pryzby@telsasoft.com> wrote:

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.

Thanks very much. I should have remembered check-world, and of course the
fact that the CI tests multiple platforms. I’ll go and do some
reading/re-reading; now that I’ve gone through some parts of the process
I’ll probably understand more.

The latest submission appears to have passed:

http://cfbot.cputube.org/isaac-morland.html

However, one of the jobs (Windows - Server 2019, MinGW64 - Meson) is paused
and appears never to have run:

https://cirrus-ci.com/task/6687014536347648

Other than that, I think this is passing the tests.

On Sun, Jan 22, 2023 at 09:50:29PM -0500, Isaac Morland wrote:

However, one of the jobs (Windows - Server 2019, MinGW64 - Meson) is paused
and appears never to have run:

https://cirrus-ci.com/task/6687014536347648

Yeah, mingw is currently set to run only when manually "triggered" by
the repository owner (because it's slow). There's no mechanism to tell
cfbot to trigger the task, but you can do it if you run from your own
github. Anyway, there's no reason to think this patch needs extra
platform-specific coverage.

--
Justin

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?

regards, tom lane

On Thu, 2 Mar 2023 at 17:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Isaac Morland <isaac.morland@gmail.com> writes:

[ 0001-Remove-source-code-display-from-df-v6.patch ]

Pushed after some editorialization on the test case.

Thanks!

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?

I think it’s OK. Right now \df+ claims that the source code for an
aggregate function is “aggregate_dummy”; that’s probably more untrue than
saying that its internal name is “aggregate_dummy”. There are several
features of aggregate functions that are always defined the same way by the
creation process; who’s to say they don’t all have a shared dummy internal
name?