Fixing bug #8228 ("set-valued function called in context that cannot accept a set")

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Cc: david(dot)g(dot)johnston(at)gmail(dot)com
Subject: Fixing bug #8228 ("set-valued function called in context that cannot accept a set")
Date: 2014-01-07 02:12:02
Message-ID: 28100.1389060722@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I kinda forgot about this bug when I went off on vacation:
http://www.postgresql.org/message-id/E1UnCv4-0007oF-Bo@wrigleys.postgresql.org

The way to fix it seems to be to do static prechecking of whether a
function's input arguments can return sets, rather than relying on
their behavior during the first execution. We already have the
infrastructure needed, namely expression_returns_set(), so a fix can
be pretty short, as illustrated in the attached proposed patch.

Now one might object that this will add an unwanted amount of overhead
to query startup. expression_returns_set() is fairly cheap, since it
only requires a parsetree walk and not any catalog lookups, but it's not
zero cost. On the other hand, some of that cost is bought back in
normal non-set cases by the fact that we needn't go through
ExecMakeFunctionResult() even once. I tried to measure whether there
was a slowdown using this test case:
$ pgbench -c 4 -T 60 -S -n bench
in a non-assert build using a scale-factor-10 pgbench database, on an
8-core machine. The best reported transaction rate over five trials was
35556.680918 in current HEAD, and 35466.438468 with the patch, for an
apparent slowdown of 0.3%. This is below what I'd normally consider
significant, since the run-to-run variability is considerably more than
that, but there may be some actual slowdown there.

We could consider a more invasive fix that would try to push the static
checking back into the planner or even parser, but that would not make
things any faster when queries are only executed once after planning,
as is true in this test scenario. In any case, adding fields to
FuncExpr/OpExpr would not be a back-patchable fix.

Thoughts? Unless someone has a better idea, I'm inclined to push
this patch and call it good.

regards, tom lane

Attachment Content-Type Size
case-with-or-without-srf.patch text/x-diff 6.1 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Etsuro Fujita 2014-01-07 02:17:26 Re: Get more from indices.
Previous Message Robert Haas 2014-01-07 01:53:24 Re: ERROR: missing chunk number 0 for toast value