Обсуждение: pageinspect's infomask and infomask2 as smallint
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>
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
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
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
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
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
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
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
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
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