Re: pg_control is missing a field for LOBLKSIZE

Lists: pgsql-hackers
From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 14:03:09
Message-ID: 19076.1401890589@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

I just chanced to notice that if someone were to change the value for
LOBLKSIZE and recompile, there'd be nothing to stop him from starting
that postmaster against an existing database, even though it would
completely misinterpret and mangle any data in pg_largeobject.

I think there ought to be a guard for that, for exactly the same reasons
that we check TOAST_MAX_CHUNK_SIZE: correct interpretation of on-disk
data requires that this value match the original database configuration.

Obviously it's too late to do anything about this in existing branches,
but I propose to add a field to pg_control after we branch off 9.4.

regards, tom lane


From: Andrew Dunstan <andrew(at)dunslane(dot)net>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgreSQL(dot)org
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 14:25:07
Message-ID: 538F2C43.3040506@dunslane.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On 06/04/2014 10:03 AM, Tom Lane wrote:
> I just chanced to notice that if someone were to change the value for
> LOBLKSIZE and recompile, there'd be nothing to stop him from starting
> that postmaster against an existing database, even though it would
> completely misinterpret and mangle any data in pg_largeobject.
>
> I think there ought to be a guard for that, for exactly the same reasons
> that we check TOAST_MAX_CHUNK_SIZE: correct interpretation of on-disk
> data requires that this value match the original database configuration.
>
> Obviously it's too late to do anything about this in existing branches,
> but I propose to add a field to pg_control after we branch off 9.4.
>
>

If we did an initdb-requiring change for 9.4 could we piggy-back this
onto it?

cheers

andrew


From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Andrew Dunstan <andrew(at)dunslane(dot)net>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers(at)postgreSQL(dot)org
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 14:27:59
Message-ID: 20140604142759.GB10482@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2014-06-04 10:25:07 -0400, Andrew Dunstan wrote:
> If we did an initdb-requiring change for 9.4 could we piggy-back this onto
> it?

Do you know of a problem requiring that?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Andrew Dunstan <andrew(at)dunslane(dot)net>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers(at)postgreSQL(dot)org
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 14:30:04
Message-ID: 20140604143004.GA2556@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

* Andrew Dunstan (andrew(at)dunslane(dot)net) wrote:
> On 06/04/2014 10:03 AM, Tom Lane wrote:
> >I just chanced to notice that if someone were to change the value for
> >LOBLKSIZE and recompile, there'd be nothing to stop him from starting
> >that postmaster against an existing database, even though it would
> >completely misinterpret and mangle any data in pg_largeobject.
> >
> >I think there ought to be a guard for that, for exactly the same reasons
> >that we check TOAST_MAX_CHUNK_SIZE: correct interpretation of on-disk
> >data requires that this value match the original database configuration.
> >
> >Obviously it's too late to do anything about this in existing branches,
> >but I propose to add a field to pg_control after we branch off 9.4.
> >
> >
>
> If we did an initdb-requiring change for 9.4 could we piggy-back
> this onto it?

I was thinking more-or-less the same thing...

Then again, I've never heard of a field complaint regarding this, so
pehraps it's not worth it.

Thanks,

Stephen


From: Andrew Dunstan <andrew(at)dunslane(dot)net>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers(at)postgreSQL(dot)org
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 14:35:00
Message-ID: 538F2E94.2030408@dunslane.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On 06/04/2014 10:27 AM, Andres Freund wrote:
> On 2014-06-04 10:25:07 -0400, Andrew Dunstan wrote:
>> If we did an initdb-requiring change for 9.4 could we piggy-back this onto
>> it?
> Do you know of a problem requiring that?
>

No, just thinking ahead.

cheers

andrew


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 14:42:57
Message-ID: 19967.1401892977@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Andrew Dunstan (andrew(at)dunslane(dot)net) wrote:
>> On 06/04/2014 10:03 AM, Tom Lane wrote:
>>> I just chanced to notice that if someone were to change the value for
>>> LOBLKSIZE and recompile, there'd be nothing to stop him from starting
>>> that postmaster against an existing database, even though it would
>>> completely misinterpret and mangle any data in pg_largeobject.

> Then again, I've never heard of a field complaint regarding this, so
> pehraps it's not worth it.

I've not heard one either, but there was just somebody asking in
pgsql-general about changing LOBLKSIZE, so he's going to be at risk.
That's not a big enough sample size to make me panic about getting a
hasty fix into 9.4, but I do think we should fix this going forward.

regards, tom lane


From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 14:43:32
Message-ID: 20140604144332.GC2556@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Andrew Dunstan (andrew(at)dunslane(dot)net) wrote:
> >> On 06/04/2014 10:03 AM, Tom Lane wrote:
> >>> I just chanced to notice that if someone were to change the value for
> >>> LOBLKSIZE and recompile, there'd be nothing to stop him from starting
> >>> that postmaster against an existing database, even though it would
> >>> completely misinterpret and mangle any data in pg_largeobject.
>
> > Then again, I've never heard of a field complaint regarding this, so
> > pehraps it's not worth it.
>
> I've not heard one either, but there was just somebody asking in
> pgsql-general about changing LOBLKSIZE, so he's going to be at risk.
> That's not a big enough sample size to make me panic about getting a
> hasty fix into 9.4, but I do think we should fix this going forward.

Agreed.

Thanks,

Stephen


From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgreSQL(dot)org
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 15:03:05
Message-ID: 20140604150305.GC10482@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2014-06-04 10:03:09 -0400, Tom Lane wrote:
> I just chanced to notice that if someone were to change the value for
> LOBLKSIZE and recompile, there'd be nothing to stop him from starting
> that postmaster against an existing database, even though it would
> completely misinterpret and mangle any data in pg_largeobject.
>
> I think there ought to be a guard for that, for exactly the same reasons
> that we check TOAST_MAX_CHUNK_SIZE: correct interpretation of on-disk
> data requires that this value match the original database configuration.
>
> Obviously it's too late to do anything about this in existing branches,
> but I propose to add a field to pg_control after we branch off 9.4.

Btw, I had wondered before if we shouldn't also add sizeof(long) to
pg_control to catch cases where a database is copied between a LLP64
(64bit windows) and an LP64 (nearly every other 64bit system) system. I
have my doubts that we're completely clean about the size
difference. Not to speak of extension datatypes.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 15:08:42
Message-ID: 20577.1401894522@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> Btw, I had wondered before if we shouldn't also add sizeof(long) to
> pg_control to catch cases where a database is copied between a LLP64
> (64bit windows) and an LP64 (nearly every other 64bit system) system. I
> have my doubts that we're completely clean about the size
> difference. Not to speak of extension datatypes.

I don't believe that this is necessary. It's certainly true that some
in-memory structures will be laid out differently, but not on-disk.

regards, tom lane


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 17:21:46
Message-ID: 31562.1401902506@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> I've not heard one either, but there was just somebody asking in
>> pgsql-general about changing LOBLKSIZE, so he's going to be at risk.
>> That's not a big enough sample size to make me panic about getting a
>> hasty fix into 9.4, but I do think we should fix this going forward.

> Agreed.

BTW, just comparing the handling of TOAST_MAX_CHUNK_SIZE and LOBLKSIZE,
I noticed that the tuptoaster.c functions are reasonably paranoid about
checking that toast chunks are the expected size, but the large object
functions are not: the latter have either no check at all, or just an
Assert that the size is not more than expected. So we could provide at
least a partial guard against a wrong LOBLKSIZE configuration by making
all the large-object functions throw elog(ERROR) if the length of a LO
chunk is more than LOBLKSIZE. Unfortunately, length *less* than LOBLKSIZE
is an expected case, so this would only help in one direction. Still,
it'd be an easy and back-patchable change that would provide at least some
defense, so I'm thinking of doing it.

regards, tom lane


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 17:33:16
Message-ID: CA+TgmoZKRox3pLfFHP3x1UWy30dJH34pZgzcbASibb6popjonQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Jun 4, 2014 at 1:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>>> I've not heard one either, but there was just somebody asking in
>>> pgsql-general about changing LOBLKSIZE, so he's going to be at risk.
>>> That's not a big enough sample size to make me panic about getting a
>>> hasty fix into 9.4, but I do think we should fix this going forward.
>
>> Agreed.
>
> BTW, just comparing the handling of TOAST_MAX_CHUNK_SIZE and LOBLKSIZE,
> I noticed that the tuptoaster.c functions are reasonably paranoid about
> checking that toast chunks are the expected size, but the large object
> functions are not: the latter have either no check at all, or just an
> Assert that the size is not more than expected. So we could provide at
> least a partial guard against a wrong LOBLKSIZE configuration by making
> all the large-object functions throw elog(ERROR) if the length of a LO
> chunk is more than LOBLKSIZE. Unfortunately, length *less* than LOBLKSIZE
> is an expected case, so this would only help in one direction. Still,
> it'd be an easy and back-patchable change that would provide at least some
> defense, so I'm thinking of doing it.

This seems like a pretty weak argument for adding run-time overhead.
Maybe it can be justified on the grounds that it would catch corrupted
TOAST data, but I've never heard of anyone changing LOBLKSIZE and see
no reason to get agitated about it.

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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 17:50:47
Message-ID: 32181.1401904247@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Jun 4, 2014 at 1:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> BTW, just comparing the handling of TOAST_MAX_CHUNK_SIZE and LOBLKSIZE,
>> I noticed that the tuptoaster.c functions are reasonably paranoid about
>> checking that toast chunks are the expected size, but the large object
>> functions are not: the latter have either no check at all, or just an
>> Assert that the size is not more than expected. So we could provide at
>> least a partial guard against a wrong LOBLKSIZE configuration by making
>> all the large-object functions throw elog(ERROR) if the length of a LO
>> chunk is more than LOBLKSIZE. Unfortunately, length *less* than LOBLKSIZE
>> is an expected case, so this would only help in one direction. Still,
>> it'd be an easy and back-patchable change that would provide at least some
>> defense, so I'm thinking of doing it.

> This seems like a pretty weak argument for adding run-time overhead.
> Maybe it can be justified on the grounds that it would catch corrupted
> TOAST data, but I've never heard of anyone changing LOBLKSIZE and see
> no reason to get agitated about it.

One if-test per fetched tuple hardly seems likely to add measurable
overhead. As for "never heard of", see today's thread in pgsql-general:
http://www.postgresql.org/message-id/flat/CAGou9Mg=9qPYTdh18NDO3LTJtwQN8uRdTwABfkcyMRUt6D_fJw(at)mail(dot)gmail(dot)com
There was a similar gripe a few months ago:
http://www.postgresql.org/message-id/CACg6vWXy_84ShCCXzNCsz9xLfWnx5sZvQRU6aNcrR0c3XW1Bxg@mail.gmail.com

There are at least two places in inv_api.c where we have
"Assert(pagelen <= LOBLKSIZE)" that is protecting a subsequent memcpy
into a local variable of size LOBLKSIZE, so that the only thing standing
between us and a stack-smash security issue that's trivially exploitable
in production builds is that on-disk data conforms to our expectation
about LOBLKSIZE. I think it's definitely worth promoting these checks
to regular runtime-if-test-and-elog.

regards, tom lane


From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 17:57:19
Message-ID: 20140604175719.GK2556@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> There are at least two places in inv_api.c where we have
> "Assert(pagelen <= LOBLKSIZE)" that is protecting a subsequent memcpy
> into a local variable of size LOBLKSIZE, so that the only thing standing
> between us and a stack-smash security issue that's trivially exploitable
> in production builds is that on-disk data conforms to our expectation
> about LOBLKSIZE. I think it's definitely worth promoting these checks
> to regular runtime-if-test-and-elog.

Agreed. Promoting that to a run-time check seems well worth it to me.

Thanks,

Stephen


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-04 22:57:31
Message-ID: 15930.1401922651@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> There are at least two places in inv_api.c where we have
>> "Assert(pagelen <= LOBLKSIZE)" that is protecting a subsequent memcpy
>> into a local variable of size LOBLKSIZE, so that the only thing standing
>> between us and a stack-smash security issue that's trivially exploitable
>> in production builds is that on-disk data conforms to our expectation
>> about LOBLKSIZE. I think it's definitely worth promoting these checks
>> to regular runtime-if-test-and-elog.

> Agreed. Promoting that to a run-time check seems well worth it to me.

Here's a draft patch for this. Barring objections I'll commit the whole
thing to HEAD, and the inv_api.c changes to the back branches as well.

regards, tom lane

Attachment Content-Type Size
check-loblksize-1.patch text/x-diff 9.9 KB

From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-17 16:15:10
Message-ID: 20140617161510.GE3666@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Jun 4, 2014 at 06:57:31PM -0400, Tom Lane wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> >> There are at least two places in inv_api.c where we have
> >> "Assert(pagelen <= LOBLKSIZE)" that is protecting a subsequent memcpy
> >> into a local variable of size LOBLKSIZE, so that the only thing standing
> >> between us and a stack-smash security issue that's trivially exploitable
> >> in production builds is that on-disk data conforms to our expectation
> >> about LOBLKSIZE. I think it's definitely worth promoting these checks
> >> to regular runtime-if-test-and-elog.
>
> > Agreed. Promoting that to a run-time check seems well worth it to me.
>
> Here's a draft patch for this. Barring objections I'll commit the whole
> thing to HEAD, and the inv_api.c changes to the back branches as well.

Uh, I think pg_upgrade needs to check that they match too.

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

+ Everyone has their own god. +


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-17 16:28:46
Message-ID: 2695.1403022526@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Uh, I think pg_upgrade needs to check that they match too.

Possibly. What do you think it should do when examining a pg_control
version that lacks the field?

regards, tom lane


From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-17 17:12:55
Message-ID: 20140617171255.GF3666@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Tue, Jun 17, 2014 at 12:28:46PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Uh, I think pg_upgrade needs to check that they match too.
>
> Possibly. What do you think it should do when examining a pg_control
> version that lacks the field?

Good question. I have existing cases where fields were removed, but not
ones that were added. As we have no way to query the old cluster's
value for LOBLKSIZE, I think I will just add code to compare them if
they _both_ exist.

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

+ Everyone has their own god. +


From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Stephen Frost <sfrost(at)snowman(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-17 19:55:02
Message-ID: 20140617195502.GJ18688@eldon.alvh.no-ip.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Bruce Momjian wrote:
> On Tue, Jun 17, 2014 at 12:28:46PM -0400, Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > Uh, I think pg_upgrade needs to check that they match too.
> >
> > Possibly. What do you think it should do when examining a pg_control
> > version that lacks the field?
>
> Good question. I have existing cases where fields were removed, but not
> ones that were added. As we have no way to query the old cluster's
> value for LOBLKSIZE, I think I will just add code to compare them if
> they _both_ exist.

Can't you compare it to the historic default value? I mean, add an
assumption that people thus far has never tweaked it.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Stephen Frost <sfrost(at)snowman(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-17 23:01:38
Message-ID: 20140617230138.GG3666@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Tue, Jun 17, 2014 at 03:55:02PM -0400, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Tue, Jun 17, 2014 at 12:28:46PM -0400, Tom Lane wrote:
> > > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > > Uh, I think pg_upgrade needs to check that they match too.
> > >
> > > Possibly. What do you think it should do when examining a pg_control
> > > version that lacks the field?
> >
> > Good question. I have existing cases where fields were removed, but not
> > ones that were added. As we have no way to query the old cluster's
> > value for LOBLKSIZE, I think I will just add code to compare them if
> > they _both_ exist.
>
> Can't you compare it to the historic default value? I mean, add an
> assumption that people thus far has never tweaked it.

Well, if they did tweak it, then they would be unable to use pg_upgrade
because it would complain about a mismatch if they actually matched the
old and new servers.

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

+ Everyone has their own god. +


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-17 23:12:16
Message-ID: 19478.1403046736@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> On Tue, Jun 17, 2014 at 03:55:02PM -0400, Alvaro Herrera wrote:
>> Can't you compare it to the historic default value? I mean, add an
>> assumption that people thus far has never tweaked it.

> Well, if they did tweak it, then they would be unable to use pg_upgrade
> because it would complain about a mismatch if they actually matched the
> old and new servers.

What about comparing to the symbolic value LOBLKSIZE? This would make
pg_upgrade assume that the old installation had been tweaked the same
as in its own build. This ends up being the same as what you said,
ie, effectively no comparison ... but it might be less complicated to
code/understand.

regards, tom lane


From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-18 00:46:00
Message-ID: 20140618004600.GH3666@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Tue, Jun 17, 2014 at 07:12:16PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > On Tue, Jun 17, 2014 at 03:55:02PM -0400, Alvaro Herrera wrote:
> >> Can't you compare it to the historic default value? I mean, add an
> >> assumption that people thus far has never tweaked it.
>
> > Well, if they did tweak it, then they would be unable to use pg_upgrade
> > because it would complain about a mismatch if they actually matched the
> > old and new servers.
>
> What about comparing to the symbolic value LOBLKSIZE? This would make
> pg_upgrade assume that the old installation had been tweaked the same
> as in its own build. This ends up being the same as what you said,
> ie, effectively no comparison ... but it might be less complicated to
> code/understand.

OK, assume the compiled-in default is the value for an old cluster that
has no value --- yeah, I could do that.

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

+ Everyone has their own god. +


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-18 18:28:24
Message-ID: CA+TgmoZvjyNDninbEXswr2_pBGF1W9E_E11d9NR6S4JWhy2Y+A@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Tue, Jun 17, 2014 at 8:46 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Tue, Jun 17, 2014 at 07:12:16PM -0400, Tom Lane wrote:
>> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> > On Tue, Jun 17, 2014 at 03:55:02PM -0400, Alvaro Herrera wrote:
>> >> Can't you compare it to the historic default value? I mean, add an
>> >> assumption that people thus far has never tweaked it.
>>
>> > Well, if they did tweak it, then they would be unable to use pg_upgrade
>> > because it would complain about a mismatch if they actually matched the
>> > old and new servers.
>>
>> What about comparing to the symbolic value LOBLKSIZE? This would make
>> pg_upgrade assume that the old installation had been tweaked the same
>> as in its own build. This ends up being the same as what you said,
>> ie, effectively no comparison ... but it might be less complicated to
>> code/understand.
>
> OK, assume the compiled-in default is the value for an old cluster that
> has no value --- yeah, I could do that.

I'm not really sure why this is better than Bruce's original proposal, though.

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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Bruce Momjian <bruce(at)momjian(dot)us>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-18 18:55:10
Message-ID: 26065.1403117710@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Jun 17, 2014 at 8:46 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> On Tue, Jun 17, 2014 at 07:12:16PM -0400, Tom Lane wrote:
>>> What about comparing to the symbolic value LOBLKSIZE? This would make
>>> pg_upgrade assume that the old installation had been tweaked the same
>>> as in its own build. This ends up being the same as what you said,
>>> ie, effectively no comparison ... but it might be less complicated to
>>> code/understand.

>> OK, assume the compiled-in default is the value for an old cluster that
>> has no value --- yeah, I could do that.

> I'm not really sure why this is better than Bruce's original proposal, though.

The net behavior would be the same, but I thought it might be easier to
code by thinking of it this way. Or maybe it wouldn't --- it's just a
suggestion.

regards, tom lane


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Bruce Momjian <bruce(at)momjian(dot)us>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-18 19:19:21
Message-ID: CA+Tgmob1D+k+YiCWF4CQpJaz2UGQ0r9CO7ZsAwOSZX0RxsS_+Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Jun 18, 2014 at 2:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Jun 17, 2014 at 8:46 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>>> On Tue, Jun 17, 2014 at 07:12:16PM -0400, Tom Lane wrote:
>>>> What about comparing to the symbolic value LOBLKSIZE? This would make
>>>> pg_upgrade assume that the old installation had been tweaked the same
>>>> as in its own build. This ends up being the same as what you said,
>>>> ie, effectively no comparison ... but it might be less complicated to
>>>> code/understand.
>
>>> OK, assume the compiled-in default is the value for an old cluster that
>>> has no value --- yeah, I could do that.
>
>> I'm not really sure why this is better than Bruce's original proposal, though.
>
> The net behavior would be the same, but I thought it might be easier to
> code by thinking of it this way. Or maybe it wouldn't --- it's just a
> suggestion.

Well, the difference is that if we just don't check it, there can
never be an error. Basically, it's the user's job to DTRT. If we
check it against some semi-arbitrary value, we'll catch the case where
the old cluster was modified with a custom setting and the new one was
not - but couldn't we also get false positives under obscure
circumstances?

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


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Bruce Momjian <bruce(at)momjian(dot)us>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-18 19:28:46
Message-ID: 27073.1403119726@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Jun 18, 2014 at 2:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The net behavior would be the same, but I thought it might be easier to
>> code by thinking of it this way. Or maybe it wouldn't --- it's just a
>> suggestion.

> Well, the difference is that if we just don't check it, there can
> never be an error. Basically, it's the user's job to DTRT. If we
> check it against some semi-arbitrary value, we'll catch the case where
> the old cluster was modified with a custom setting and the new one was
> not - but couldn't we also get false positives under obscure
> circumstances?

Huh? What we'd be checking is the LOBLKSIZE compiled into pg_upgrade
versus that stored into pg_control by the new postmaster. If those
are different, then pg_upgrade didn't come from the same build as the
new postmaster, which is already a pretty hazardous situation (especially
if the user is fooling with low-level stuff like this).

regards, tom lane


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Bruce Momjian <bruce(at)momjian(dot)us>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-06-18 19:50:14
Message-ID: CA+TgmoaP=zWnenQ81r=9XH4nCc1wWuLT3wSACDv86OhQuNOLyQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Jun 18, 2014 at 3:28 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Jun 18, 2014 at 2:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> The net behavior would be the same, but I thought it might be easier to
>>> code by thinking of it this way. Or maybe it wouldn't --- it's just a
>>> suggestion.
>
>> Well, the difference is that if we just don't check it, there can
>> never be an error. Basically, it's the user's job to DTRT. If we
>> check it against some semi-arbitrary value, we'll catch the case where
>> the old cluster was modified with a custom setting and the new one was
>> not - but couldn't we also get false positives under obscure
>> circumstances?
>
> Huh? What we'd be checking is the LOBLKSIZE compiled into pg_upgrade
> versus that stored into pg_control by the new postmaster. If those
> are different, then pg_upgrade didn't come from the same build as the
> new postmaster, which is already a pretty hazardous situation (especially
> if the user is fooling with low-level stuff like this).

OK, I agree that checking that wouldn't hurt anything.

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


From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_control is missing a field for LOBLKSIZE
Date: 2014-09-10 23:25:12
Message-ID: 20140910232512.GB16199@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Tue, Jun 17, 2014 at 08:46:00PM -0400, Bruce Momjian wrote:
> On Tue, Jun 17, 2014 at 07:12:16PM -0400, Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > On Tue, Jun 17, 2014 at 03:55:02PM -0400, Alvaro Herrera wrote:
> > >> Can't you compare it to the historic default value? I mean, add an
> > >> assumption that people thus far has never tweaked it.
> >
> > > Well, if they did tweak it, then they would be unable to use pg_upgrade
> > > because it would complain about a mismatch if they actually matched the
> > > old and new servers.
> >
> > What about comparing to the symbolic value LOBLKSIZE? This would make
> > pg_upgrade assume that the old installation had been tweaked the same
> > as in its own build. This ends up being the same as what you said,
> > ie, effectively no comparison ... but it might be less complicated to
> > code/understand.
>
> OK, assume the compiled-in default is the value for an old cluster that
> has no value --- yeah, I could do that.

Turns out I already had values that could be missing in the old cluster,
so I just used the same format for this, rather than testing for
LOBLKSIZE.

Attached patch applied.

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

+ Everyone has their own god. +

Attachment Content-Type Size
lo.diff text/x-diff 3.3 KB