pgsql: Assert that we don't invent relfilenodes or type OIDs in binary

Uh, is there a reason this is only an Assert(), meaning it only checks
in assert builds. pg_upgrade already has a lot of checks and they are
all fatal.

---------------------------------------------------------------------------

On Tue, Jun 13, 2017 at 12:04:48AM +0000, Tom Lane wrote:

Assert that we don't invent relfilenodes or type OIDs in binary upgrade.

During pg_upgrade's restore run, all relfilenode choices should be
overridden by commands in the dump script. If we ever find ourselves
choosing a relfilenode in the ordinary way, someone blew it. Likewise for
pg_type OIDs. Since pg_upgrade might well succeed anyway, if there happens
not to be a conflict during the regression test run, we need assertions
here to keep us on the straight and narrow.

We might someday be able to remove the assertion in GetNewRelFileNode,
if pg_upgrade is rewritten to remove its assumption that old and new
relfilenodes always match. But it's hard to see how to get rid of the
pg_type OID constraint, since those OIDs are embedded in user tables
in some cases.

Back-patch as far as 9.5, because of the risk of back-patches breaking
something here even if it works in HEAD. I'd prefer to go back further,
but 9.4 fails both assertions due to get_rel_infos()'s use of a temporary
table. We can't use the later-branch solution of a CTE for compatibility
reasons (cf commit 5d16332e9), and it doesn't seem worth inventing some
other way to do the query. (I did check, by dint of changing the Asserts
to elog(WARNING), that there are no other cases of unwanted OID assignments
during 9.4's regression test run.)

Discussion: /messages/by-id/19785.1497215827@sss.pgh.pa.us

Branch
------
REL9_6_STABLE

Details
-------
https://git.postgresql.org/pg/commitdiff/318fd99ce7e775158c87b8515780f2663d2df429

Modified Files
--------------
src/backend/catalog/catalog.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Bruce Momjian <bruce@momjian.us> writes:

Uh, is there a reason this is only an Assert(), meaning it only checks
in assert builds. pg_upgrade already has a lot of checks and they are
all fatal.

Yeah, I didn't think it was worth adding overhead to production builds
for it.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Tue, Jun 13, 2017 at 01:39:34PM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Uh, is there a reason this is only an Assert(), meaning it only checks
in assert builds. pg_upgrade already has a lot of checks and they are
all fatal.

Yeah, I didn't think it was worth adding overhead to production builds
for it.

Uh, you realize there are already many pg_upgrade sanity checks in the
backend that are not asserts, right?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On 2017-06-13 13:45:12 -0400, Bruce Momjian wrote:

On Tue, Jun 13, 2017 at 01:39:34PM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Uh, is there a reason this is only an Assert(), meaning it only checks
in assert builds. pg_upgrade already has a lot of checks and they are
all fatal.

Yeah, I didn't think it was worth adding overhead to production builds
for it.

Uh, you realize there are already many pg_upgrade sanity checks in the
backend that are not asserts, right?

And? Not that it'll make a huge difference, but GetNewOidWithIndex() is
a relatively hot-path in some workloads (e.g. with lots of toasted
data), so it actually can make a difference. And for debugging asserts
are often actually more useful, because you get a backtrace.

I'm not sure what you're actually concerned about here?

- Andres

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Tue, Jun 13, 2017 at 11:52:04AM -0700, Andres Freund wrote:

On 2017-06-13 13:45:12 -0400, Bruce Momjian wrote:

On Tue, Jun 13, 2017 at 01:39:34PM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Uh, is there a reason this is only an Assert(), meaning it only checks
in assert builds. pg_upgrade already has a lot of checks and they are
all fatal.

Yeah, I didn't think it was worth adding overhead to production builds
for it.

Uh, you realize there are already many pg_upgrade sanity checks in the
backend that are not asserts, right?

And? Not that it'll make a huge difference, but GetNewOidWithIndex() is
a relatively hot-path in some workloads (e.g. with lots of toasted
data), so it actually can make a difference. And for debugging asserts
are often actually more useful, because you get a backtrace.

I'm not sure what you're actually concerned about here?

I am concerned a non-assert build will not error out, but if no one else
is concerned about that, I am fine.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Bruce Momjian <bruce@momjian.us> writes:

On Tue, Jun 13, 2017 at 11:52:04AM -0700, Andres Freund wrote:

I'm not sure what you're actually concerned about here?

I am concerned a non-assert build will not error out, but if no one else
is concerned about that, I am fine.

I think in a production situation, we actually don't want it to error
out. The odds are fairly good that the run would complete successfully
(ie, the potential OID collision never actually materializes). So all
we're doing is converting a possible failure into an unavoidable one.

Where we want to hear about the problem is in development. So really
an Assert is the right thing.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Tue, Jun 13, 2017 at 03:10:16PM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Tue, Jun 13, 2017 at 11:52:04AM -0700, Andres Freund wrote:

I'm not sure what you're actually concerned about here?

I am concerned a non-assert build will not error out, but if no one else
is concerned about that, I am fine.

I think in a production situation, we actually don't want it to error
out. The odds are fairly good that the run would complete successfully
(ie, the potential OID collision never actually materializes). So all
we're doing is converting a possible failure into an unavoidable one.

Where we want to hear about the problem is in development. So really
an Assert is the right thing.

OK, but my point is that all the other places, which seems similar,
error out in production.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers