Re: Transactional sequence stuff breaks pg_upgrade

On 2017-06-12 17:13:34 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2017-06-11 20:03:28 -0400, Tom Lane wrote:
> >> @@ -391,6 +391,13 @@ GetNewRelFileNode(Oid reltablespace, Rel
> >> bool collides;
> >> BackendId backend;
> >>
> >> + /*
> >> + * If we ever get here during pg_upgrade, there's something wrong; all
> >> + * relfilenode assignments during a binary-upgrade run should be
> >> + * determined by commands in the dump script.
> >> + */
> >> + Assert(!IsBinaryUpgrade);
> >> +
>
> > I'm very doubtful that a) this doesn't get hit in practice, and b) that
> > we can rely on it going forward. At least until we change toasting to
> > not use the global oid counter.
>
> This is not about assignments from the global OID counter; the function
> it's touching is GetNewRelFileNode() not GetNewObjectId().

Ah, that makes more sense. You'd put the backtrace() into
GetNewObjectId() your original message, that's probably why I thought
about it.

> We don't care, for the most part. But we *do* care about relfilenode
> assignments, for precisely the reason seen in this bug.

Even there I don't think that's a sane assumption *for the future*. We
just need a slight change in the rules about when a toast table is needed
- and that stuff seriously need overhauling - and it doesn't work
anymore.

In my opinion the problem of:
> assignments of relfilenodes have to be shortcircuited by pg_upgrade
> override calls during a binary-restore run, or we risk filename
> collisions.

should instead be solved by simply not even trying to preserve
relfilenodes. We can "just" copy/link files to the the new
relfilenodes, there's no need to preserve them, in contrast to
pg_class.oid etc. But that's obviously something for the future.

> I intend to not only commit this, but back-patch it. There's enough
> changes in relevant code paths that logic that is fine in HEAD might
> not be fine in back branches.

Hm.

Greetings,

Andres Freund