Subject: Re: Remove source code display from \df+?

From: Alvaro Herrera <[email-masked]>
Date: Sun, 22 Jan 2023 20:15:49 +0100

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)

---

From: Isaac Morland <[email-masked]>
Date: Sun, 22 Jan 2023 14:53:48 -0500

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

---

From: Tom Lane <[email-masked]>
Date: Sun, 22 Jan 2023 15:04:14 -0500

> Updated patch attached. Informally, I am adopting the regress_* policy for

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

---

From: Isaac Morland <[email-masked]>
Date: Sun, 22 Jan 2023 16:28:21 -0500

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

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 22 Jan 2023 15:56:28 -0600

> 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

---

From: Isaac Morland <[email-masked]>
Date: Sun, 22 Jan 2023 17:04:50 -0500

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

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 22 Jan 2023 16:27:51 -0600

> [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