pgsql: Remove pg_class.relhaspkey

Hi,

On 2018-03-14 19:35:40 +0000, Peter Eisentraut wrote:

Remove pg_class.relhaspkey

It is not used for anything internally, and it cannot be relied on for
external uses, so it can just be removed. To correct recommended way to
check for a primary key is in pg_index.

Discussion: /messages/by-id/b1a24c6c-6913-f89c-674e-0704f0ed69db@2ndquadrant.com

I'm not clear on the mechanics, and the animal doesn't show the critical
log. But this might have caused the issue, being the only commit between
runs:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2018-03-14%2019%3A37%3A20

Andrew, any chance you could modify the module to capture
pg_upgrade_dump_*.log?

Greetings,

Andres Freund

Greetings,

* Andres Freund (andres@anarazel.de) wrote:

On 2018-03-14 19:35:40 +0000, Peter Eisentraut wrote:

Remove pg_class.relhaspkey

It is not used for anything internally, and it cannot be relied on for
external uses, so it can just be removed. To correct recommended way to
check for a primary key is in pg_index.

Discussion: /messages/by-id/b1a24c6c-6913-f89c-674e-0704f0ed69db@2ndquadrant.com

I'm not clear on the mechanics, and the animal doesn't show the critical
log. But this might have caused the issue, being the only commit between
runs:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2018-03-14%2019%3A37%3A20

Andrew, any chance you could modify the module to capture
pg_upgrade_dump_*.log?

I've not quite tracked it down, but I would caution against blaming this
commit- when doing some parallel regression test runs, I was seeing
failures also, but they've not been consistent and I was trying to get
something else done so didn't focus on them, so they may have been
failures due to my environment, but might also be some kind of race
condition in an earlier commit (my guess at the moment is actually the
INOUT arguments for procedures commit...).

I'll try doing some more runs to see if I can reproduce it, but wanted
to mention it, just to encourage people to consider looking more broadly
than just this commit if they run into similar issues.

Thanks!

Stephen

On 2018-03-14 18:09:07 -0400, Stephen Frost wrote:

Greetings,

* Andres Freund (andres@anarazel.de) wrote:

On 2018-03-14 19:35:40 +0000, Peter Eisentraut wrote:

Remove pg_class.relhaspkey

It is not used for anything internally, and it cannot be relied on for
external uses, so it can just be removed. To correct recommended way to
check for a primary key is in pg_index.

Discussion: /messages/by-id/b1a24c6c-6913-f89c-674e-0704f0ed69db@2ndquadrant.com

I'm not clear on the mechanics, and the animal doesn't show the critical
log. But this might have caused the issue, being the only commit between
runs:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2018-03-14%2019%3A37%3A20

Andrew, any chance you could modify the module to capture
pg_upgrade_dump_*.log?

I've not quite tracked it down, but I would caution against blaming this
commit- when doing some parallel regression test runs, I was seeing
failures also, but they've not been consistent and I was trying to get
something else done so didn't focus on them, so they may have been
failures due to my environment, but might also be some kind of race
condition in an earlier commit (my guess at the moment is actually the
INOUT arguments for procedures commit...).

I'll try doing some more runs to see if I can reproduce it, but wanted
to mention it, just to encourage people to consider looking more broadly
than just this commit if they run into similar issues.

It failed identically twice in a row now, preceded by a significant
number of successful runs:
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=crake&br=HEAD
and afaik there's no concurrency during the cross-version test. So
sure, it's possible that something else is to blame, but I'd not bet on
it.

Greetings,

Andres Freund

On Thu, Mar 15, 2018 at 7:43 AM, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2018-03-14 19:35:40 +0000, Peter Eisentraut wrote:

Remove pg_class.relhaspkey

It is not used for anything internally, and it cannot be relied on for
external uses, so it can just be removed. To correct recommended way to
check for a primary key is in pg_index.

Discussion: /messages/by-id/b1a24c6c-6913-

f89c-674e-0704f0ed69db@2ndquadrant.com

I'm not clear on the mechanics, and the animal doesn't show the critical
log. But this might have caused the issue, being the only commit between
runs:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=
crake&dt=2018-03-14%2019%3A37%3A20

Andrew, any chance you could modify the module to capture
pg_upgrade_dump_*.log?

I will look at it. Meanwhile I have fixed a typo in the module. The
database in question is supposed to have been dropped as it has caused
other issues in the past, but due to the typo it wasn't.

Basing an MV on pg_class could always be difficult for pg_upgrade. Maybe
that's not a brilliant thing to do in a test (or maybe the test should drop
the MV after it's done).

cheers

andrew

Stephen Frost <sfrost@snowman.net> writes:

I've not quite tracked it down, but I would caution against blaming this
commit- when doing some parallel regression test runs, I was seeing
failures also, but they've not been consistent and I was trying to get
something else done so didn't focus on them, so they may have been
failures due to my environment, but might also be some kind of race
condition in an earlier commit (my guess at the moment is actually the
INOUT arguments for procedures commit...).

crake has now failed twice at the same spot, so it's looking reproducible
to me. I've enabled TestUpgradeXversion on longfin and will try to
reproduce the problem there, if Andrew doesn't manage to scrape the
relevant log out of crake first. (It'll probably take a few hours
for me to get results.)

The broader picture is that we've been having irreproducible failures on
the buildfarm since around December. Some of them are in postgres_fdw,
and I'd originally suspected a problem there, but we've also seen more
than one select_parallel failure like this one:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&amp;dt=2018-03-11%2023%3A25%3A46

My gut instinct right now is that those are related, since they both
look like "plan change with no visible triggering cause". I have no
ideas about a plausible explanation.

regards, tom lane

Andrew Dunstan <andrew@dunslane.net> writes:

Basing an MV on pg_class could always be difficult for pg_upgrade. Maybe
that's not a brilliant thing to do in a test (or maybe the test should drop
the MV after it's done).

OH. Is that what it's doing? The cause of the failure is immediately
obvious then. pg_class now lacks a relhaspkey column, but the matview
is still dependent on one being there.

I concur that this is not well-considered test design.

regards, tom lane

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Basing an MV on pg_class could always be difficult for pg_upgrade. Maybe
that's not a brilliant thing to do in a test (or maybe the test should drop
the MV after it's done).

OH. Is that what it's doing? The cause of the failure is immediately
obvious then. pg_class now lacks a relhaspkey column, but the matview
is still dependent on one being there.

I concur that this is not well-considered test design.

It seems very strange to me that the test_ddl_deparse database would be
used by a pg_upgrade test, but OK. I'll change it.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Mar 15, 2018 at 9:41 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Basing an MV on pg_class could always be difficult for pg_upgrade. Maybe
that's not a brilliant thing to do in a test (or maybe the test should drop
the MV after it's done).

OH. Is that what it's doing? The cause of the failure is immediately
obvious then. pg_class now lacks a relhaspkey column, but the matview
is still dependent on one being there.

I concur that this is not well-considered test design.

It seems very strange to me that the test_ddl_deparse database would be
used by a pg_upgrade test, but OK. I'll change it.

As I mentioned upthread, it's not supposed to be, but was being due to
a typo that I have fixed. You should see this error cleared on the
dashboard in a few minutes.

However, in general the module tries to do a maximal test. That
includes almost all the contrib databases. That approach has helped
reveal problems several times in the past.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan wrote:

As I mentioned upthread, it's not supposed to be, but was being due to
a typo that I have fixed. You should see this error cleared on the
dashboard in a few minutes.

However, in general the module tries to do a maximal test. That
includes almost all the contrib databases. That approach has helped
reveal problems several times in the past.

Well, maybe we should dictate policy that it must be possible to
pg_upgrade this database too; then we'll just have to work so that it
always works. I have fixed the current problem and I'm open to the idea
of keeping it working.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Well, maybe we should dictate policy that it must be possible to
pg_upgrade this database too; then we'll just have to work so that it
always works. I have fixed the current problem and I'm open to the idea
of keeping it working.

Name of the matview now seems a bit confusing ...

regards, tom lane