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

From: Isaac Morland <[email-masked]>
Date: Sun, 22 Jan 2023 20:23:25 -0500

On Sun, 22 Jan 2023 at 17:27, Justin Pryzby <[email-masked]> wrote:

> On Sun, Jan 22, 2023 at 04:28:21PM -0500, Isaac Morland wrote:
> > On Sun, 22 Jan 2023 at 15:04, Tom Lane <[email-masked]> 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.

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 22 Jan 2023 20:37:32 -0600

> understand the full situation so I will try to use this when I get some

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

---

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

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

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 22 Jan 2023 22:09:54 -0600

> 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

---

From: Tom Lane <[email-masked]>
Date: Thu, 02 Mar 2023 17:20:15 -0500

Isaac Morland <[email-masked]> 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

---

From: Isaac Morland <[email-masked]>
Date: Thu, 2 Mar 2023 23:49:16 -0500

> Pushed after some editorialization on the test case.

Thanks!

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