Re: Transactional sequence stuff breaks pg_upgrade

Hi,

On 2017-06-11 20:03:28 -0400, Tom Lane wrote:
> I wrote:
> > I believe I've identified the reason why skink and some other buildfarm
> > members have been failing the pg_upgrade test recently.
> > ...
> > Not sure what we want to do about it. One idea is to make
> > ALTER SEQUENCE not so transactional when in binary-upgrade mode.
>
> On closer inspection, the only thing that pg_upgrade needs is to be
> able to do ALTER SEQUENCE OWNED BY without a relfilenode bump.

That indeed seems appropriate. More on the reliability of that below,
though.

> PFA two versions of a patch that fixes this problem, at least to the
> extent that it gets through check-world without triggering the Assert
> I added to GetNewRelFileNode (which HEAD doesn't). The first one
> is a minimally-invasive hack; the second one puts the responsibility
> for deciding if a sequence rewrite is needed into init_params. That's
> bulkier but might be useful if we ever grow additional ALTER SEQUENCE
> options that don't need a rewrite. OTOH, I'm not very clear on what
> such options might look like, so maybe the extra flexibility is useless.
> Thoughts?

I think the flexibility isn't a bad idea, there's certainly other
options (cycle, cache, and with more work additional ones) that could be
changed without a rewrite.

> diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
> index 11ee536..43ef4cd 100644
> --- a/src/backend/catalog/catalog.c
> +++ b/src/backend/catalog/catalog.c
> @@ -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. A number of catalog tables have
toastable columns, and the decision when to toast will every now and
then change. Even without changing the compression algorithm, added
columns will push things across boundaries.

I'm not yet sure what the best fix for that will be, but I don't think
we should bake in the assumption that the oid counter won't be touched.

- Andres