Re: Error check always bypassed in tablefunc.c

From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Error check always bypassed in tablefunc.c
Date: 2015-01-17 14:16:30
Message-ID: CAB7nPqRPjh5cW1yW6FQW=ib3OfEodn2koRF9EtADpbfvsMN=bA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Alvaro Herrera wrote:
> Michael Paquier wrote:
>
>> As mentioned in $subject, commit 08c33c4 of 2003 has made the
>> following block of code dead in tablefunc.c:1320 because level is
>> incremented to at least 1:
>> /* First time through, do a little more setup */
>> if (level == 0)
>> {
>
> Uh. This means the error case has no test coverage ...
And looking at that more closely things are a bit more tricky than it
seems. The problem is here related especially to connectby in the
checks done to determine if the input and return key fields are
compatible or not by comparing directly some type OIDs in
compatConnectbyTupleDescs.

Note that if we simply enable this check as-is, connectby would fail
for example with test cases like this one simply because text !=
varchar:
CREATE TABLE connectby_tree(keyid text, parent_keyid text, pos int);
INSERT INTO connectby_tree VALUES('row1',NULL, 0);
INSERT INTO connectby_tree VALUES('row2','row1', 0);
INSERT INTO connectby_tree VALUES('row3','row1', 0);
INSERT INTO connectby_tree VALUES('row4','row2', 1);
INSERT INTO connectby_tree VALUES('row5','row2', 0);
SELECT * FROM connectby('connectby_tree', 'keyid', 'parent_keyid',
'pos', 'row2', 0) AS
t(keyid varchar(256), parent_keyid varchar(256), level int, pos int);

Hence, what I am proposing as a fix is to replace the old check by
something checking if there is a possible cast between the input and
output types, and to error out if types are not compatible. This way,
compatibility is preserved in HEAD and back-branches, and IMO I think
that it would be good to check this compatibility in connectby. New
regression tests are added as well.

Patch is attached. Comments welcome.
--
Michael

Attachment Content-Type Size
20150117_tablefunc_deadcode_fix_v2.patch text/x-diff 6.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2015-01-17 14:55:37 Re: PrivateRefCount patch has got issues
Previous Message Robert Haas 2015-01-17 14:06:40 Re: Safe memory allocation functions