Thread: additional patch for contrib/tablefunc - added to regression test
The attached adds a bit to the contrib/tablefunc regression test for behavior
of connectby() in the presence of infinite recursion. Please apply this one in
addition to the one sent earlier.
Thanks,
Joe
Index: contrib/tablefunc/sql/tablefunc.sql
===================================================================
RCS file: /opt/src/cvs/pgsql-server/contrib/tablefunc/sql/tablefunc.sql,v
retrieving revision 1.2
diff -c -r1.2 tablefunc.sql
*** contrib/tablefunc/sql/tablefunc.sql 14 Sep 2002 19:53:59 -0000 1.2
--- contrib/tablefunc/sql/tablefunc.sql 26 Sep 2002 23:53:29 -0000
***************
*** 58,60 ****
--- 58,81 ----
-- without branch
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level
int);
+ -- recursion detection
+ INSERT INTO connectby_int VALUES(10,9);
+ INSERT INTO connectby_int VALUES(11,10);
+ INSERT INTO connectby_int VALUES(9,11);
+
+ -- should fail due to infinite recursion
+ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid int, parent_keyid int,
levelint, branch text);
+
+ -- infinite recursion failure avoided by depth limit
+ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 4, '~') AS t(keyid int, parent_keyid int,
levelint, branch text);
+
+ -- test for falsely detected recursion
+ DROP TABLE connectby_int;
+ CREATE TABLE connectby_int(keyid int, parent_keyid int);
+ INSERT INTO connectby_int VALUES(11,NULL);
+ INSERT INTO connectby_int VALUES(10,11);
+ INSERT INTO connectby_int VALUES(111,11);
+ INSERT INTO connectby_int VALUES(1,111);
+ -- this should not fail due to recursion detection
+ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '11', 0, '-') AS t(keyid int, parent_keyid int,
levelint, branch text);
+
Index: contrib/tablefunc/expected/tablefunc.out
===================================================================
RCS file: /opt/src/cvs/pgsql-server/contrib/tablefunc/expected/tablefunc.out,v
retrieving revision 1.2
diff -c -r1.2 tablefunc.out
*** contrib/tablefunc/expected/tablefunc.out 14 Sep 2002 19:53:59 -0000 1.2
--- contrib/tablefunc/expected/tablefunc.out 26 Sep 2002 23:53:33 -0000
***************
*** 177,179 ****
--- 177,217 ----
9 | 5 | 2
(6 rows)
+ -- recursion detection
+ INSERT INTO connectby_int VALUES(10,9);
+ INSERT INTO connectby_int VALUES(11,10);
+ INSERT INTO connectby_int VALUES(9,11);
+ -- should fail due to infinite recursion
+ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid int, parent_keyid int,
levelint, branch text);
+ ERROR: infinite recursion detected
+ -- infinite recursion failure avoided by depth limit
+ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 4, '~') AS t(keyid int, parent_keyid int,
levelint, branch text);
+ keyid | parent_keyid | level | branch
+ -------+--------------+-------+-------------
+ 2 | | 0 | 2
+ 4 | 2 | 1 | 2~4
+ 6 | 4 | 2 | 2~4~6
+ 8 | 6 | 3 | 2~4~6~8
+ 5 | 2 | 1 | 2~5
+ 9 | 5 | 2 | 2~5~9
+ 10 | 9 | 3 | 2~5~9~10
+ 11 | 10 | 4 | 2~5~9~10~11
+ (8 rows)
+
+ -- test for falsely detected recursion
+ DROP TABLE connectby_int;
+ CREATE TABLE connectby_int(keyid int, parent_keyid int);
+ INSERT INTO connectby_int VALUES(11,NULL);
+ INSERT INTO connectby_int VALUES(10,11);
+ INSERT INTO connectby_int VALUES(111,11);
+ INSERT INTO connectby_int VALUES(1,111);
+ -- this should not fail due to recursion detection
+ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '11', 0, '-') AS t(keyid int, parent_keyid int,
levelint, branch text);
+ keyid | parent_keyid | level | branch
+ -------+--------------+-------+----------
+ 11 | | 0 | 11
+ 10 | 11 | 1 | 11-10
+ 111 | 11 | 1 | 11-111
+ 1 | 111 | 2 | 11-111-1
+ (4 rows)
+
Joe, This test presumes that it will succeed. That is, if there is an infinite recursion here, the test will just hang -- as far as I can tell -- and will not *actually* report the failure. Gavin On Thu, 26 Sep 2002, Joe Conway wrote: > The attached adds a bit to the contrib/tablefunc regression test for behavior > of connectby() in the presence of infinite recursion. Please apply this one in > addition to the one sent earlier. > > Thanks, > > Joe >
Gavin Sherry wrote:
> Joe,
>
> This test presumes that it will succeed. That is, if there is an infinite
> recursion here, the test will just hang -- as far as I can tell -- and
> will not *actually* report the failure.
No, look at expected/tablefunc.out:
-- recursion detection
INSERT INTO connectby_int VALUES(10,9);
INSERT INTO connectby_int VALUES(11,10);
INSERT INTO connectby_int VALUES(9,11);
-- should fail due to infinite recursion
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~')
AS t(keyid int, parent_keyid int, level int, branch text);
ERROR: infinite recursion detected
^^^^^^^^^^^^^^^^^^^^^^^^^^^
Unless I'm miss-understanding you?
Joe
On Thu, 26 Sep 2002, Joe Conway wrote:
> Gavin Sherry wrote:
> > Joe,
> >
> > This test presumes that it will succeed. That is, if there is an infinite
> > recursion here, the test will just hang -- as far as I can tell -- and
> > will not *actually* report the failure.
>
> No, look at expected/tablefunc.out:
>
> -- recursion detection
> INSERT INTO connectby_int VALUES(10,9);
> INSERT INTO connectby_int VALUES(11,10);
> INSERT INTO connectby_int VALUES(9,11);
> -- should fail due to infinite recursion
> SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~')
> AS t(keyid int, parent_keyid int, level int, branch text);
> ERROR: infinite recursion detected
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Unless I'm miss-understanding you?
Correct me if i'm wrong: there was a but in connectby which meant that it
did not detect infinite recursion correct. You want to add a test to make
sure it continues to do so. However, if you bust the infinite recursion
detection it will, by definition, recurse infinitely and not elog() -- at
least in a reasonably time frame.
To handle this situation, you may need to use the psql timeout
functionality -- but I know nothing about it.
Gavin
Gavin Sherry wrote: > Correct me if i'm wrong: there was a but in connectby which meant that it > did not detect infinite recursion correct. You want to add a test to make > sure it continues to do so. However, if you bust the infinite recursion > detection it will, by definition, recurse infinitely and not elog() -- at > least in a reasonably time frame. > > To handle this situation, you may need to use the psql timeout > functionality -- but I know nothing about it. > I think you finally got through my thick skull. Now I see the issue. OK, I'll rework the patch using a timeout. Thanks for pointing that out! New patch will be sent in shortly -- gotta fight traffic now. Joe
Joe Conway <mail@joeconway.com> writes:
> Gavin Sherry wrote:
>> ... However, if you bust the infinite recursion
>> detection it will, by definition, recurse infinitely and not elog() -- at
>> least in a reasonably time frame.
>>
>> To handle this situation, you may need to use the psql timeout
>> functionality -- but I know nothing about it.
> I think you finally got through my thick skull. Now I see the
> issue. OK, I'll rework the patch using a timeout.
I think this is unnecessary. Bugs in any functionality tested by a
regression test could cause the system to go into an infinite loop;
but we don't put timeouts on all of them. In practice we rely on
the user to exercise some judgment about whether the tests are
proceeding normally.
I think a timeout is more likely to create problems than fix them;
how will you pick a platform-independent, system-load-independent
timeout value, for example?
Also, in my experience an infinite-recursion bug generally results
in a pretty quick stack overflow and core dump, so a timeout per se
wouldn't be needed anyway.
regards, tom lane
Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >>I think you finally got through my thick skull. Now I see the >>issue. OK, I'll rework the patch using a timeout. > > I think this is unnecessary. Bugs in any functionality tested by a > regression test could cause the system to go into an infinite loop; > but we don't put timeouts on all of them. In practice we rely on > the user to exercise some judgment about whether the tests are > proceeding normally. Sounds good to me. I guess the patch stands as originally sent, unless other objections arise. Thanks, Joe
Joe Conway wrote: > Tom Lane wrote: > > Joe Conway <mail@joeconway.com> writes: > >>I think you finally got through my thick skull. Now I see the > >>issue. OK, I'll rework the patch using a timeout. > > > > I think this is unnecessary. Bugs in any functionality tested by a > > regression test could cause the system to go into an infinite loop; > > but we don't put timeouts on all of them. In practice we rely on > > the user to exercise some judgment about whether the tests are > > proceeding normally. > > Sounds good to me. I guess the patch stands as originally sent, unless other > objections arise. Uh, I already deleted it. Can you shoot it to me again? :-) -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073