Re: GSOC13 proposal - extend RETURNING syntax

From: Boszormenyi Zoltan <zb(at)cybertec(dot)at>
To: Karol Trzcionka <karlikt(at)gmail(dot)com>
Cc: David Fetter <david(at)fetter(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andres Freund <andres(at)2ndquadrant(dot)com>, Marko Tiikkaja <marko(at)joh(dot)to>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: GSOC13 proposal - extend RETURNING syntax
Date: 2013-08-20 13:37:17
Message-ID: 5213710D.5090506@cybertec.at
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

2013-08-19 21:52 keltezéssel, Boszormenyi Zoltan írta:
> 2013-08-19 21:21 keltezéssel, Karol Trzcionka írta:
>> W dniu 19.08.2013 19:56, Boszormenyi Zoltan pisze:
>>> * Does it apply cleanly to the current git master?
>>>
>>> No. There's a reject in src/backend/optimizer/plan/initsplan.c
>> Thank you, merged in attached version.
>>> * Does it include reasonable tests?
>>>
>>> Yes but the test fails after trying to fix the rejected chunk of the
>>> patch.
>> It fails because the "HINT" was changed, fixed.
>> That version merges some nested "ifs" left over from earlier work.
>
> I tried to compile your v5 patch and I got:
>
> initsplan.c: In function ‘add_vars_to_targetlist’:
> initsplan.c:216:26: warning: ‘rel’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> rel->reltargetlist = lappend(rel->reltargetlist,
> ^
>
> You shouldn't change the assignment at declaration:
>
> - RelOptInfo *rel = find_base_rel(root, var->varno);
> + RelOptInfo *rel;
> ...
> + if (root->parse->commandType == CMD_UPDATE)
> + {
> ... (code using rel)
> + }
> + rel = find_base_rel(root, varno);

Let me say it again: the new code in initsplan.c::add_vars_to_targetlist() is fishy.
The compiler says that "rel" used on line 216 may be uninitialized.

Keeping it that way passes "make check", perhaps "rel" was initialized
in a previous iteration of "foreach(temp, vars)", possibly in the
else if (IsA(node, PlaceHolderVar))
branch, which means that "PlaceHolderInfo *phinfo" may be interpreted
as RelOptInfo *, stomping on memory.

Moving the assignment back to the declaration makes "make check"
fail with the attached regression.diffs file.

Initializing it as "RelOptInfo *rel = NULL;" makes the regression check
die with a segfault, obviously.

Change the code to avoid the warning and still produce the wanted effect.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

Attachment Content-Type Size
regression.diffs text/plain 6.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Stehule 2013-08-20 13:38:17 Re: PL/pgSQL PERFORM with CTE
Previous Message David E. Wheeler 2013-08-20 13:32:11 Re: PL/pgSQL PERFORM with CTE