Re: Fix for pg_upgrade migrating pg_largeobject_metadata

I have applied the attached four patches which simplify pg_upgrade
processing --- less code means fewer bugs. The commit message is at the
top of each patch.

The last patch fixes a problem where I was not migrating
pg_largeobject_metadata and its index for 9.0+ migrations, which of
course would only affect migrations to 9.1 and 9.0 to 9.0 migrations, so
I backpatched that to 9.0.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> The last patch fixes a problem where I was not migrating
> pg_largeobject_metadata and its index for 9.0+ migrations, which of
> course would only affect migrations to 9.1 and 9.0 to 9.0 migrations, so
> I backpatched that to 9.0.

That isn't going to work. At least not unless you start trying to force
roles to have the same OIDs in the new installation.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > The last patch fixes a problem where I was not migrating
> > pg_largeobject_metadata and its index for 9.0+ migrations, which of
> > course would only affect migrations to 9.1 and 9.0 to 9.0 migrations, so
> > I backpatched that to 9.0.
>
> That isn't going to work. At least not unless you start trying to force
> roles to have the same OIDs in the new installation.

Uh, don't we store the pg_shadow.usesysid in pg_largeobject_metadata?
If so I can use the CREATE ROLE ... SYSID clause when doing a binary
upgrade.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> That isn't going to work. At least not unless you start trying to force
>> roles to have the same OIDs in the new installation.

> If so I can use the CREATE ROLE ... SYSID clause when doing a binary
> upgrade.

Oh, I had forgotten we still had that wart in the grammar.
It doesn't actually work:

else if (strcmp(defel->defname, "sysid") == 0)
{
ereport(NOTICE,
(errmsg("SYSID can no longer be specified")));
}

Not sure if it's better to try to make that work again than to add
another hack in pg_upgrade_support. On the whole that's a keyword
I'd rather see us drop someday soon.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Tom Lane wrote:
> >> That isn't going to work. At least not unless you start trying to force
> >> roles to have the same OIDs in the new installation.
>
> > If so I can use the CREATE ROLE ... SYSID clause when doing a binary
> > upgrade.
>
> Oh, I had forgotten we still had that wart in the grammar.
> It doesn't actually work:
>
> else if (strcmp(defel->defname, "sysid") == 0)
> {
> ereport(NOTICE,
> (errmsg("SYSID can no longer be specified")));
> }
>
> Not sure if it's better to try to make that work again than to add
> another hack in pg_upgrade_support. On the whole that's a keyword
> I'd rather see us drop someday soon.

OK, let me work on adding it to pg_upgrade_support. Glad you saw this.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > Tom Lane wrote:
> > >> That isn't going to work. At least not unless you start trying to force
> > >> roles to have the same OIDs in the new installation.
> >
> > > If so I can use the CREATE ROLE ... SYSID clause when doing a binary
> > > upgrade.
> >
> > Oh, I had forgotten we still had that wart in the grammar.
> > It doesn't actually work:
> >
> > else if (strcmp(defel->defname, "sysid") == 0)
> > {
> > ereport(NOTICE,
> > (errmsg("SYSID can no longer be specified")));
> > }
> >
> > Not sure if it's better to try to make that work again than to add
> > another hack in pg_upgrade_support. On the whole that's a keyword
> > I'd rather see us drop someday soon.
>
> OK, let me work on adding it to pg_upgrade_support. Glad you saw this.

I have fixed the bug by using pg_upgrade_support. It was a little
complicated because you need to install the pg_upgrade_support functions
in the super-user database so it is available when you create the users
in the first step of restoring the pg_dumpall file.

I am afraid we have to batckpatch this to fix to 9.0 for 9.0 to 9.0
upgrades. It does not apply when coming from pre-9.0 because there was
no pg_largeobject_metadata.

For testing I did this:

CREATE DATABASE lo;
\c lo
SELECT lo_import('/etc/motd');
\set loid `psql -qt -c 'select loid from pg_largeobject' lo`
CREATE ROLE user1;
CREATE ROLE user2;
-- force user2 to have a different user id on restore
DROP ROLE user1;
GRANT ALL ON LARGE OBJECT :loid TO user2;

The fixed version shows:

lo=> select * from pg_largeobject_metadata;
lomowner | lomacl
----------+------------------------------------------
10 | {postgres=rw/postgres,user2=rw/postgres}
(1 row)

In the broken version, 'user2' was a raw oid, obviously wrong.

Fortunately this was found during my testing and not reported as a bug
by a pg_upgrade user.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Patch applied.

I did not backpatch to 9.0 because you can't migrate from 9.0 to 9.0
with the same catversion (because of tablespace conflict), and a pre-9.0
migration to 9.0 has not large object permissions to migrate. In
summary, it didn't seem worth the risk, and was hard to test.

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

Bruce Momjian wrote:
> Bruce Momjian wrote:
> > Tom Lane wrote:
> > > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > > Tom Lane wrote:
> > > >> That isn't going to work. At least not unless you start trying to force
> > > >> roles to have the same OIDs in the new installation.
> > >
> > > > If so I can use the CREATE ROLE ... SYSID clause when doing a binary
> > > > upgrade.
> > >
> > > Oh, I had forgotten we still had that wart in the grammar.
> > > It doesn't actually work:
> > >
> > > else if (strcmp(defel->defname, "sysid") == 0)
> > > {
> > > ereport(NOTICE,
> > > (errmsg("SYSID can no longer be specified")));
> > > }
> > >
> > > Not sure if it's better to try to make that work again than to add
> > > another hack in pg_upgrade_support. On the whole that's a keyword
> > > I'd rather see us drop someday soon.
> >
> > OK, let me work on adding it to pg_upgrade_support. Glad you saw this.
>
> I have fixed the bug by using pg_upgrade_support. It was a little
> complicated because you need to install the pg_upgrade_support functions
> in the super-user database so it is available when you create the users
> in the first step of restoring the pg_dumpall file.
>
> I am afraid we have to batckpatch this to fix to 9.0 for 9.0 to 9.0
> upgrades. It does not apply when coming from pre-9.0 because there was
> no pg_largeobject_metadata.
>
> For testing I did this:
>
> CREATE DATABASE lo;
> \c lo
> SELECT lo_import('/etc/motd');
> \set loid `psql -qt -c 'select loid from pg_largeobject' lo`
> CREATE ROLE user1;
> CREATE ROLE user2;
> -- force user2 to have a different user id on restore
> DROP ROLE user1;
> GRANT ALL ON LARGE OBJECT :loid TO user2;
>
> The fixed version shows:
>
> lo=> select * from pg_largeobject_metadata;
> lomowner | lomacl
> ----------+------------------------------------------
> 10 | {postgres=rw/postgres,user2=rw/postgres}
> (1 row)
>
> In the broken version, 'user2' was a raw oid, obviously wrong.
>
> Fortunately this was found during my testing and not reported as a bug
> by a pg_upgrade user.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +