Thread: pageinspect's infomask and infomask2 as smallint

pageinspect's infomask and infomask2 as smallint

From
Alvaro Herrera
Date:
Thanks to Noah Misch's review of the keylock patch I noticed that
pageinspect's heap_page_items(bytea) function returns infomask and
infomask2 as smallint (signed).  But the fields in the tuple header are
16 bits unsigned, so if the high (16th) bit is set, it returns negative
values which seem hard to handle.  Not a problem for infomask, because
the high bit is used for a VACUUM FULL-era flag; but in infomask2 it is
used.

This seems hard to fix for existing installations with the unpackaged
module already loaded -- IIRC it's not acceptable to drop a function,
which is what would need to be done here.

I report this because it seems the first case to test upgradability of a
module.

-- 
Álvaro Herrera <alvherre@alvh.no-ip.org>


Re: pageinspect's infomask and infomask2 as smallint

From
Heikki Linnakangas
Date:
On 14.02.2011 21:49, Alvaro Herrera wrote:
> Thanks to Noah Misch's review of the keylock patch I noticed that
> pageinspect's heap_page_items(bytea) function returns infomask and
> infomask2 as smallint (signed).  But the fields in the tuple header are
> 16 bits unsigned, so if the high (16th) bit is set, it returns negative
> values which seem hard to handle.  Not a problem for infomask, because
> the high bit is used for a VACUUM FULL-era flag; but in infomask2 it is
> used.
>
> This seems hard to fix for existing installations with the unpackaged
> module already loaded -- IIRC it's not acceptable to drop a function,
> which is what would need to be done here.

pageinspect is just a debugging aid, so I think we should change it from 
smallint to int4 in 9.1, and not bother backporting.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com


Re: pageinspect's infomask and infomask2 as smallint

From
Tom Lane
Date:
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> On 14.02.2011 21:49, Alvaro Herrera wrote:
>> Thanks to Noah Misch's review of the keylock patch I noticed that
>> pageinspect's heap_page_items(bytea) function returns infomask and
>> infomask2 as smallint (signed).  But the fields in the tuple header are
>> 16 bits unsigned, so if the high (16th) bit is set, it returns negative
>> values which seem hard to handle.  Not a problem for infomask, because
>> the high bit is used for a VACUUM FULL-era flag; but in infomask2 it is
>> used.
>> 
>> This seems hard to fix for existing installations with the unpackaged
>> module already loaded -- IIRC it's not acceptable to drop a function,
>> which is what would need to be done here.

> pageinspect is just a debugging aid, so I think we should change it from 
> smallint to int4 in 9.1, and not bother backporting.

I don't see any reason that the old version of the function couldn't be
dropped in the upgrade script.  It's not likely anything would be
depending on it, is it?
        regards, tom lane


Re: pageinspect's infomask and infomask2 as smallint

From
Robert Haas
Date:
On Tue, Feb 15, 2011 at 10:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> On 14.02.2011 21:49, Alvaro Herrera wrote:
>>> Thanks to Noah Misch's review of the keylock patch I noticed that
>>> pageinspect's heap_page_items(bytea) function returns infomask and
>>> infomask2 as smallint (signed).  But the fields in the tuple header are
>>> 16 bits unsigned, so if the high (16th) bit is set, it returns negative
>>> values which seem hard to handle.  Not a problem for infomask, because
>>> the high bit is used for a VACUUM FULL-era flag; but in infomask2 it is
>>> used.
>>>
>>> This seems hard to fix for existing installations with the unpackaged
>>> module already loaded -- IIRC it's not acceptable to drop a function,
>>> which is what would need to be done here.
>
>> pageinspect is just a debugging aid, so I think we should change it from
>> smallint to int4 in 9.1, and not bother backporting.
>
> I don't see any reason that the old version of the function couldn't be
> dropped in the upgrade script.  It's not likely anything would be
> depending on it, is it?

I don't see much point in taking the risk.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: pageinspect's infomask and infomask2 as smallint

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Feb 15, 2011 at 10:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't see any reason that the old version of the function couldn't be
>> dropped in the upgrade script. �It's not likely anything would be
>> depending on it, is it?

> I don't see much point in taking the risk.

What risk?  And at least we'd be trying to do it cleanly, in a manner
that should work for at least 99% of users.  AFAICT, Heikki's proposal
is "break it for everyone, and damn the torpedoes".
        regards, tom lane


Re: pageinspect's infomask and infomask2 as smallint

From
Robert Haas
Date:
On Tue, Feb 15, 2011 at 10:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Feb 15, 2011 at 10:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I don't see any reason that the old version of the function couldn't be
>>> dropped in the upgrade script.  It's not likely anything would be
>>> depending on it, is it?
>
>> I don't see much point in taking the risk.
>
> What risk?  And at least we'd be trying to do it cleanly, in a manner
> that should work for at least 99% of users.  AFAICT, Heikki's proposal
> is "break it for everyone, and damn the torpedoes".

I must be confused.  I thought Heikki's proposal was "fix it in 9.1,
because incompatibilities are an expected part of major release
upgrades, but don't break it in 9.0 and prior, because it's not
particularly important and we don't want to change behavior or risk
breaking things in minor releases".

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: pageinspect's infomask and infomask2 as smallint

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Feb 15, 2011 at 10:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What risk? �And at least we'd be trying to do it cleanly, in a manner
>> that should work for at least 99% of users. �AFAICT, Heikki's proposal
>> is "break it for everyone, and damn the torpedoes".

> I must be confused.  I thought Heikki's proposal was "fix it in 9.1,
> because incompatibilities are an expected part of major release
> upgrades, but don't break it in 9.0 and prior, because it's not
> particularly important and we don't want to change behavior or risk
> breaking things in minor releases".

No, nobody was proposing changing it before 9.1 (or at least I didn't
think anybody was).  What's under discussion is how much effort to put
into making a 9.0-to-9.1 upgrade go smoothly for people who have the
function installed.
        regards, tom lane


Re: pageinspect's infomask and infomask2 as smallint

From
Robert Haas
Date:
On Tue, Feb 15, 2011 at 11:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Feb 15, 2011 at 10:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> What risk?  And at least we'd be trying to do it cleanly, in a manner
>>> that should work for at least 99% of users.  AFAICT, Heikki's proposal
>>> is "break it for everyone, and damn the torpedoes".
>
>> I must be confused.  I thought Heikki's proposal was "fix it in 9.1,
>> because incompatibilities are an expected part of major release
>> upgrades, but don't break it in 9.0 and prior, because it's not
>> particularly important and we don't want to change behavior or risk
>> breaking things in minor releases".
>
> No, nobody was proposing changing it before 9.1 (or at least I didn't
> think anybody was).  What's under discussion is how much effort to put
> into making a 9.0-to-9.1 upgrade go smoothly for people who have the
> function installed.

Oh, I see, never mind me then...  feel free to make that go smoothly.  :-)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: pageinspect's infomask and infomask2 as smallint

From
Heikki Linnakangas
Date:
On 15.02.2011 18:03, Tom Lane wrote:
> Robert Haas<robertmhaas@gmail.com>  writes:
>> On Tue, Feb 15, 2011 at 10:53 AM, Tom Lane<tgl@sss.pgh.pa.us>  wrote:
>>> What risk?  And at least we'd be trying to do it cleanly, in a manner
>>> that should work for at least 99% of users.  AFAICT, Heikki's proposal
>>> is "break it for everyone, and damn the torpedoes".
>
>> I must be confused.  I thought Heikki's proposal was "fix it in 9.1,
>> because incompatibilities are an expected part of major release
>> upgrades, but don't break it in 9.0 and prior, because it's not
>> particularly important and we don't want to change behavior or risk
>> breaking things in minor releases".

Right, that's what I meant.

> No, nobody was proposing changing it before 9.1 (or at least I didn't
> think anybody was).  What's under discussion is how much effort to put
> into making a 9.0-to-9.1 upgrade go smoothly for people who have the
> function installed.

Oh, never mind then.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com


Re: pageinspect's infomask and infomask2 as smallint

From
Alvaro Herrera
Date:
Excerpts from Tom Lane's message of mar feb 15 12:42:00 -0300 2011:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

> > pageinspect is just a debugging aid, so I think we should change it from 
> > smallint to int4 in 9.1, and not bother backporting.
> 
> I don't see any reason that the old version of the function couldn't be
> dropped in the upgrade script.  It's not likely anything would be
> depending on it, is it?

Okay, so I changed the C code, and there's working upgrade support that
takes you from the 9.0 version to the 9.1 version.

I tested this by creating a 9.0 database with pageinspect loaded, then
pg_upgrade'd it to 9.1, then rancreate extension pageinspect from unpackaged;

If I run the function before running the CREATE EXTENSION command, it
works but still displays the negative numbers.  After that, it behaves
as expected.

I was a bit surprised that I had to remove the ALTER EXTENSION/ADD
FUNCTION command from the upgrade script, but in hindsight it makes
perfect sense -- those commands are being run inside the
"create_extension" context and so the function being created already
belongs to the extension.  Still, maybe we should make ALTER
EXTENSION/ADD idempotent.

I considered the idea of calling this version 1.1 and shipping a new
pageinspect--1.0--1.1.sql script (which works perfectly, provided you
run ALTER EXTENSION/DROP FUNCTION before dropping the function, then
ALTER/ADD later), but decided that this was overkill.  We can still
change it if people thinks that'd be better, of course, but ...

Nice work on extensions and their upgradability, overall.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support