Thread

Ooops ... seems we need a re-release pronto

Tom Lane <tgl@sss.pgh.pa.us> — 2007-02-06T18:27:47Z

As per numerous reports this morning, PG 8.2.2 and 8.1.7 both fail on
fairly simple scenarios involving typmod-bearing columns (varchar,
numeric, etc) with check constraints or functional indexes (and maybe
other cases too, but those are the ones reported so far).  I have not
been able to reproduce the failures in 8.0 but I think it may have the
same issue in a weaker form.  We need a quick re-release I'm afraid.

I have applied a patch that resolves the problem AFAICT, but this time
around it would be nice to get some more eyeballs and testing on it.
Please try CVS HEAD or branch tips this afternoon, if you can.  Core
is currently thinking of wrapping update tarballs this evening (maybe
around midnight UTC?).

Also, if anyone can find a related failure in 8.0.11 please notify us.
I'm not sure whether we need an update in that branch or not ...

			regards, tom lane

Re: Ooops ... seems we need a re-release pronto

Bruno Wolff III <bruno@wolff.to> — 2007-02-06T18:33:40Z

On Tue, Feb 06, 2007 at 13:27:47 -0500,
  Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> I have applied a patch that resolves the problem AFAICT, but this time
> around it would be nice to get some more eyeballs and testing on it.
> Please try CVS HEAD or branch tips this afternoon, if you can.  Core
> is currently thinking of wrapping update tarballs this evening (maybe
> around midnight UTC?).

Is a test going to get added to the regression tests to catch similar
regressions in the future?

Re: Ooops ... seems we need a re-release pronto

Tom Lane <tgl@sss.pgh.pa.us> — 2007-02-06T18:40:41Z

Bruno Wolff III <bruno@wolff.to> writes:
> Is a test going to get added to the regression tests to catch similar
> regressions in the future?

I've been thinking about that.  It seems that the regression tests have
fairly poor coverage of use of typmod-bearing data types in general;
most of our tests of complicated queries tend to use "simple" datatypes
like int or text.  I don't have any immediate thoughts what to do about
that --- massive expansion of the tests doesn't seem justified --- but
this isn't the first bug we've hit in this area.  It's just a bit more
embarrassing than most :-(

			regards, tom lane

Re: Ooops ... seems we need a re-release pronto

decibel <decibel@decibel.org> — 2007-02-06T19:05:25Z

On Feb 6, 2007, at 12:40 PM, Tom Lane wrote:
> Bruno Wolff III <bruno@wolff.to> writes:
>> Is a test going to get added to the regression tests to catch similar
>> regressions in the future?
>
> I've been thinking about that.  It seems that the regression tests  
> have
> fairly poor coverage of use of typmod-bearing data types in general;
> most of our tests of complicated queries tend to use "simple"  
> datatypes
> like int or text.  I don't have any immediate thoughts what to do  
> about
> that --- massive expansion of the tests doesn't seem justified --- but
> this isn't the first bug we've hit in this area.  It's just a bit more
> embarrassing than most :-(

What about the idea that's been floated in the past about a -- 
extensive mode for regression testing that would (generally) only be  
used by the build farm. That would mean others wouldn't have to  
suffer through extremely long make check's.

Or is there another reason not to expand the tests?
--
Jim Nasby                                            jim@nasby.net
EnterpriseDB      http://enterprisedb.com      512.569.9461 (cell)

Re: Ooops ... seems we need a re-release pronto

Tom Lane <tgl@sss.pgh.pa.us> — 2007-02-06T19:16:05Z

Jim Nasby <decibel@decibel.org> writes:
> On Feb 6, 2007, at 12:40 PM, Tom Lane wrote:
>> ... massive expansion of the tests doesn't seem justified

> What about the idea that's been floated in the past about a -- 
> extensive mode for regression testing that would (generally) only be  
> used by the build farm. That would mean others wouldn't have to  
> suffer through extremely long make check's.

> Or is there another reason not to expand the tests?

I'm not concerned so much about the runtime as the development and
maintenance effort...

			regards, tom lane

Re: Ooops ... seems we need a re-release pronto

Michael Paesold <mpaesold@gmx.at> — 2007-02-06T20:19:43Z

Tom Lane wrote:
> Bruno Wolff III <bruno@wolff.to> writes:
>> Is a test going to get added to the regression tests to catch similar
>> regressions in the future?
> 
> I've been thinking about that.  It seems that the regression tests have
> fairly poor coverage of use of typmod-bearing data types in general;
> most of our tests of complicated queries tend to use "simple" datatypes
> like int or text.  I don't have any immediate thoughts what to do about
> that --- massive expansion of the tests doesn't seem justified --- but
> this isn't the first bug we've hit in this area.  It's just a bit more
> embarrassing than most :-(

I think at least the most simple cases should be added. At the very least a 
test that would have caught this issue. This is really the first time that 
I had to pull a minor release and go back to a previous version. ;-)

As far as I understand, it's as simple as this (untested):

CREATE TABLE tab (
	c DECIMAL(5,2) NOT NULL,
         CHECK (c >= 0)
);
INSERT INTO tab ('0');

Right?

Or at least:
UPDATE tab SET c='0';

Best Regards
Michael Paesold

Re: Ooops ... seems we need a re-release pronto

Neil Conway <neilc@samurai.com> — 2007-02-07T19:20:09Z

On Tue, 2007-02-06 at 12:33 -0600, Bruno Wolff III wrote:
> Is a test going to get added to the regression tests to catch similar
> regressions in the future?

While we can modify the regression tests to catch this specific problem
in the future, I wonder if there ought to be more testing of security
releases in the future. When a problem is reported, fixed, tested, and
the resulting security fix is publicly distributed all without public
discussion (e.g. on the -hackers list), that sounds like an invitation
to introduce regressions to me.

-Neil

Subject supressed

Bruce Momjian <bruce@momjian.us> — 2007-02-07T19:34:35Z

Neil Conway wrote:
> On Tue, 2007-02-06 at 12:33 -0600, Bruno Wolff III wrote:
> > Is a test going to get added to the regression tests to catch similar
> > regressions in the future?
> 
> While we can modify the regression tests to catch this specific problem
> in the future, I wonder if there ought to be more testing of security
> releases in the future. When a problem is reported, fixed, tested, and
> the resulting security fix is publicly distributed all without public
> discussion (e.g. on the -hackers list), that sounds like an invitation
> to introduce regressions to me.

Uh, did you have to post with this subject line just as 8.2.3 was being
released:

  Subject: Re: [HACKERS] Ooops ... seems we need a re-release pronto

Trying to give us heart-attacks?  :-)

-- 
  Bruce Momjian  <bruce@momjian.us>          http://momjian.us
  EnterpriseDB                               http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Re: Ooops ... seems we need a re-release pronto

Tom Lane <tgl@sss.pgh.pa.us> — 2007-02-07T19:38:24Z

Neil Conway <neilc@samurai.com> writes:
> While we can modify the regression tests to catch this specific problem
> in the future, I wonder if there ought to be more testing of security
> releases in the future. When a problem is reported, fixed, tested, and
> the resulting security fix is publicly distributed all without public
> discussion (e.g. on the -hackers list), that sounds like an invitation
> to introduce regressions to me.

No doubt about it, but what else do you propose?  This patch was
reviewed by several people, none of whom caught the problem.  (Not that
I want to blame them, it was certainly my bug.)  And we normally don't
have indefinite amounts of time to spend before responding.  With
limited eyes and limited time you're going to have a greater chance of
screw-up; but unless we are willing to flout the conventional wisdom
about keeping security-related bugs secret, I think that's just
something that's got to be lived with.

			regards, tom lane

Re: Ooops ... seems we need a re-release pronto

Jim Nasby <jim@nasby.net> — 2007-02-08T09:27:54Z

On Tue, Feb 06, 2007 at 02:16:05PM -0500, Tom Lane wrote:
> Jim Nasby <decibel@decibel.org> writes:
> > On Feb 6, 2007, at 12:40 PM, Tom Lane wrote:
> >> ... massive expansion of the tests doesn't seem justified
> 
> > What about the idea that's been floated in the past about a -- 
> > extensive mode for regression testing that would (generally) only be  
> > used by the build farm. That would mean others wouldn't have to  
> > suffer through extremely long make check's.
> 
> > Or is there another reason not to expand the tests?
> 
> I'm not concerned so much about the runtime as the development and
> maintenance effort...

I can see development... but are there enough changes where the
maintenance would be an issue?
-- 
Jim Nasby                                            jim@nasby.net
EnterpriseDB      http://enterprisedb.com      512.569.9461 (cell)

Re: Ooops ... seems we need a re-release pronto

Peter Eisentraut <peter_e@gmx.net> — 2007-02-10T09:36:56Z

Tom Lane wrote:
> Jim Nasby <decibel@decibel.org> writes:
> > On Feb 6, 2007, at 12:40 PM, Tom Lane wrote:
> >> ... massive expansion of the tests doesn't seem justified
> >
> > What about the idea that's been floated in the past about a --
> > extensive mode for regression testing that would (generally) only
> > be used by the build farm. That would mean others wouldn't have to
> > suffer through extremely long make check's.
> >
> > Or is there another reason not to expand the tests?
>
> I'm not concerned so much about the runtime as the development and
> maintenance effort...

Shouldn't we at least add the one or two exemplary statements that 
failed so we have some sort of coverage of the problem?

-- 
Peter Eisentraut
http://developer.postgresql.org/~petere/

Re: Ooops ... seems we need a re-release pronto

Tom Lane <tgl@sss.pgh.pa.us> — 2007-02-10T17:20:35Z

Peter Eisentraut <peter_e@gmx.net> writes:
> Shouldn't we at least add the one or two exemplary statements that 
> failed so we have some sort of coverage of the problem?

We could, but I'm unexcited about it.  The known failures are an
extremely narrow case: we're trying to evaluate expressions (either
CHECK constraints or index expressions) over a tuple proposed to be
inserted into a relation.  But the TupleDesc that's been supplied to
the evaluator is not the tuple descriptor of the target relation,
it's a descriptor generated on-the-fly from the target list of the
plan tree (by ExecTypeFromTL).  And the target list includes some
constants, and our nodetree representation for constants fails to
preserve typmod knowledge, and so ExecTypeFromTL produces atttypmod
-1 for this column, and the security check didn't like that because
the Var it was checking had a nondefault typmod.

When the first reports came in, I thought seriously about fixing it
by forcing the target relation's real tupdesc (from its relcache entry)
to be used in this context instead of a generated tupdesc.  I concluded
that it was too likely that there were other cases where we were
evaluating expressions against generated tuples, and we had to back off
the strength of the security check instead.  I do not actually have any
specific examples, but I think it's fairly pointless to add a regression
test that covers this one narrow scenario when there are probably lots
of others.

I'm not especially a fan of the testing philosophy that says you
memorialize each individual past mistake as a permanent regression test
--- I think that just bloats the tests, and test bloat is a bad thing
because it discourages people from running the tests.  (MySQL's
regression tests currently require about an hour on a fast machine.
Somehow this has not helped them to achieve a low bug rate...)  I do
agree with adding a test when you think it is likely to be able to catch
a whole class of errors, or even a specific error if it seems especially
likely to recur, but right now I'm not seeing how we do that here.

BTW, I think a good case could be made that the core of the problem
is exactly that struct Const doesn't carry typmod, and thus that we
lose information about constructs like 'foo'::char(7).  We should fix
that, and also anywhere else in the expression tree structure where
we are discarding knowledge about the typmod of a result.  This has
got some urgency because of Teodor's recent work on allowing user
defined types to have typmods --- we can expect massive growth in the
number of scenarios where it matters.

			regards, tom lane

Re: Ooops ... seems we need a re-release pronto

Martijn van Oosterhout <kleptog@svana.org> — 2007-02-11T10:59:55Z

On Sat, Feb 10, 2007 at 12:20:35PM -0500, Tom Lane wrote:
>  I do
> agree with adding a test when you think it is likely to be able to catch
> a whole class of errors, or even a specific error if it seems especially
> likely to recur, but right now I'm not seeing how we do that here.

Well, currently the regression tests only make a handful of functional
indexes, and never insert any data into any of them. So arguably
there's a benefit to just adding a handful of inserts and updates
somewhere to test these. That a whole area of code not currently
tested.

In my memory I remember a site that displayed the code coverage of the
regression tests, but I can't find it now. Does anybody know?

Have a nice day,
-- 
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Re: Ooops ... seems we need a re-release pronto

D'Arcy Cain <darcy@druid.net> — 2007-02-11T13:40:20Z

On Sat, 10 Feb 2007 10:36:56 +0100
Peter Eisentraut <peter_e@gmx.net> wrote:
> Tom Lane wrote:
> > I'm not concerned so much about the runtime as the development and
> > maintenance effort...
> 
> Shouldn't we at least add the one or two exemplary statements that 
> failed so we have some sort of coverage of the problem?

How about a rule that says no new ode without a test?  That's one of
the ways that extreme programming is applied to new projects.
Basically what that means is that bugs continue to be found but we
never see the same bug twice because the regression test catches those.

Of course, while you are creating one test you can always add a few
related ones.

-- 
D'Arcy J.M. Cain <darcy@druid.net>         |  Democracy is three wolves
http://www.druid.net/darcy/                |  and a sheep voting on
+1 416 425 1212     (DoD#0082)    (eNTP)   |  what's for dinner.

Re: Ooops ... seems we need a re-release pronto

Tom Lane <tgl@sss.pgh.pa.us> — 2007-02-11T17:30:45Z

"D'Arcy J.M. Cain" <darcy@druid.net> writes:
> How about a rule that says no new ode without a test?

We've got way too many tests like that already, ie, a bunch of
mostly-redundant functional tests of isolated new features.
Most of the code I worry about there isn't any simple way to
test from the SQL level --- the fact that a query gives the
right answer doesn't prove it went through a particular part
of the planner, for example.

I think we need some intelligent test design, not tests thrown in
to meet a rule.

			regards, tom lane

Re: Ooops ... seems we need a re-release pronto

D'Arcy Cain <darcy@druid.net> — 2007-02-11T23:37:00Z

On Sun, 11 Feb 2007 12:30:45 -0500
Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "D'Arcy J.M. Cain" <darcy@druid.net> writes:
> > How about a rule that says no new ode without a test?
> 
> We've got way too many tests like that already, ie, a bunch of
> mostly-redundant functional tests of isolated new features.
> Most of the code I worry about there isn't any simple way to
> test from the SQL level --- the fact that a query gives the
> right answer doesn't prove it went through a particular part
> of the planner, for example.

Well, that is covered in the system that I took that from.  The full
description is;

 1. Identify a bug or missing feature.
 2. Write the test that proves the bug or missing feature.
 3. Run the test to prove that it fails.
 4. Code until the test passes and then stop.
 5. Run the regression test to make sure you didn't break something.

Step 3. is the critical one from the point of view of your concern.
Having a test that can't fail is worse than no test.

This is taken from the principles of extreme programming.

-- 
D'Arcy J.M. Cain <darcy@druid.net>         |  Democracy is three wolves
http://www.druid.net/darcy/                |  and a sheep voting on
+1 416 425 1212     (DoD#0082)    (eNTP)   |  what's for dinner.

Re: Ooops ... seems we need a re-release pronto

Tom Lane <tgl@sss.pgh.pa.us> — 2007-02-12T05:08:09Z

"D'Arcy J.M. Cain" <darcy@druid.net> writes:
> Well, that is covered in the system that I took that from.  The full
> description is;

>  1. Identify a bug or missing feature.
>  2. Write the test that proves the bug or missing feature.
>  3. Run the test to prove that it fails.
>  4. Code until the test passes and then stop.
>  5. Run the regression test to make sure you didn't break something.

> This is taken from the principles of extreme programming.

The above is all fine as a development methodology.  The question is
whether such tests are strictly a short-term development aid, or need to
be memorialized in a fashion that will cause every other developer to
re-execute them every time that developer needs to test his own work,
for the indefinite future.  I tend to think there are not that many
tests that really deserve that status.

			regards, tom lane

Re: Ooops ... seems we need a re-release pronto

Robert Treat <xzilla@users.sourceforge.net> — 2007-02-13T01:08:31Z

On Sunday 11 February 2007 05:59, Martijn van Oosterhout wrote:
> On Sat, Feb 10, 2007 at 12:20:35PM -0500, Tom Lane wrote:
> >  I do
> > agree with adding a test when you think it is likely to be able to catch
> > a whole class of errors, or even a specific error if it seems especially
> > likely to recur, but right now I'm not seeing how we do that here.
>
> Well, currently the regression tests only make a handful of functional
> indexes, and never insert any data into any of them. So arguably
> there's a benefit to just adding a handful of inserts and updates
> somewhere to test these. That a whole area of code not currently
> tested.
>
> In my memory I remember a site that displayed the code coverage of the
> regression tests, but I can't find it now. Does anybody know?
>

Are you thinking of spikesource? According to thier numbers, we currently 
cover about 40% of the code base. 

http://developer.spikesource.com/info/search.php?c=POSTGRESQL&view=details

-- 
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL

Re: Ooops ... seems we need a re-release pronto

Martijn van Oosterhout <kleptog@svana.org> — 2007-02-13T13:52:28Z

On Mon, Feb 12, 2007 at 08:08:31PM -0500, Robert Treat wrote:
> > In my memory I remember a site that displayed the code coverage of the
> > regression tests, but I can't find it now. Does anybody know?
> >
> 
> Are you thinking of spikesource? According to thier numbers, we currently 
> cover about 40% of the code base. 
> 
> http://developer.spikesource.com/info/search.php?c=POSTGRESQL&view=details

Yes, that was it, except I can't access the details, the redirection is
broken. However, with that info it would be nice to see which areas
could be better covered.

Have a nice day,
-- 
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Adding a typmod field to Const et al

Tom Lane <tgl@sss.pgh.pa.us> — 2007-03-16T23:07:33Z

A month or so back I wrote:
> BTW, I think a good case could be made that the core of the problem
> is exactly that struct Const doesn't carry typmod, and thus that we
> lose information about constructs like 'foo'::char(7).  We should fix
> that, and also anywhere else in the expression tree structure where
> we are discarding knowledge about the typmod of a result.  This has
> got some urgency because of Teodor's recent work on allowing user
> defined types to have typmods --- we can expect massive growth in the
> number of scenarios where it matters.

I looked into this and determined that the interesting cases seem to be

	Const:		needs a struct field added

	ArrayRef:	ditto; but we could drop refrestype which is
			redundant

	SubLink:	EXPR and ARRAY cases should recurse to
			subplan target item, as exprType() does

	ArrayExpr:	should adopt the same behavior as Coalesce and
			similar nodes, ie, if all the elements show the
			same type/typmod then return that typmod
			instead of -1

With these changes, exprTypmod covers all the same cases as exprType,
except for cases that demonstrably don't have a typmod, such as the
result of a non-length-coercion function, or nodes that have a hardwired
result type such as BOOL that doesn't take a typmod.

Comments, objections?

			regards, tom lane

Re: Adding a typmod field to Const et al

Greg Stark <stark@enterprisedb.com> — 2007-03-17T00:39:24Z

"Tom Lane" <tgl@sss.pgh.pa.us> writes:

> 	ArrayExpr:	should adopt the same behavior as Coalesce and
> 			similar nodes, ie, if all the elements show the
> 			same type/typmod then return that typmod
> 			instead of -1
...
> Comments, objections?

I'm not entirely convinced by this one. Does that mean expressions like this
would throw an error if col1 was declared as a numeric(1)?

 ARRAY[col1] || 10



-- 
  Gregory Stark
  EnterpriseDB          http://www.enterprisedb.com

Re: Adding a typmod field to Const et al

Tom Lane <tgl@sss.pgh.pa.us> — 2007-03-17T00:46:12Z

Gregory Stark <stark@enterprisedb.com> writes:
> I'm not entirely convinced by this one. Does that mean expressions like this
> would throw an error if col1 was declared as a numeric(1)?
>  ARRAY[col1] || 10

No, because the result of the || operator won't have a specific typmod.

			regards, tom lane