SPI and CommandCounterIncrement, redux

Back in August we had a discussion about whether SPI isn't broken in
its handling of CommandCounterIncrement:http://fts.postgresql.org/db/mw/msg.html?mid=1029236
That discussion tailed off without any agreement what to do, but I've
just been reminded of the problem, and I now understand that there's
a separate and (I think) easily-fixable bug besides the larger issue.
Moreover, this bug represents a nasty regression from 7.1, so I think
we *must* do something about it now.

The example I've just been looking at (courtesy of Patrick MacDonald)
is

create table table1 (name text);

create or replace function failing () returns integer as '
 declare   counter  integer;   thisRow  record;   tmpName1 text;   tmpName2 text;
 begin      select count(*) into counter from table1;
   raise notice ''First count is: %'', counter;
   tmpName1 = ''name'' || counter + 1;   tmpName2 = ''name'' || counter + 2;
   -- insert two values   insert into table1 values (tmpName1);   insert into table1 values (tmpName2);
   -- loop through the values and display name   for thisRow in select * from table1      loop       raise notice
''Name:: %'', thisRow.name;     end loop;
 
   select count(*) into counter from table1;
   raise notice ''New count is: %'', counter;      -- the last name displayed should be the last   -- name inserted
raisenotice ''Last name should be: %'', tmpName2;
 
   return counter; end; ' language 'plpgsql';


In current sources, this function behaves correctly the first time you
invoke it (in a given backend), and incorrectly on subsequent uses:

regression=# select failing();
NOTICE:  First count is: 0
NOTICE:  Name :: name1
NOTICE:  Name :: name2
NOTICE:  New count is: 2
NOTICE:  Last name should be: name2failing
---------      2
(1 row)

regression=# select failing();
NOTICE:  First count is: 2
NOTICE:  Name :: name1
NOTICE:  Name :: name2
NOTICE:  Name :: name3
NOTICE:  New count is: 4
NOTICE:  Last name should be: name4failing
---------      4
(1 row)

regression=#

Note the failure to display "Name :: name4".  The problem is that
no CommandCounterIncrement happens between the second INSERT and
the FOR ... SELECT, so the inserted row is considered not yet visible.

This worked correctly in 7.1.  It fails in current sources because
plpgsql's FOR is now based on SPI cursor support, and there is no
CommandCounterIncrement in SPI_cursor_open, whereas there is one
in SPI_exec and SPI_execp.  (But the first time through, a
CommandCounterIncrement occurs as a side effect of query planning
for the SELECT, so the problem is masked.)

I believe an appropriate fix is to add a CommandCounterIncrement
call near the start of SPI_cursor_open; this will make it behave
similarly to the other SPI query-execution calls.  I have verified
that this change fixes Patrick's example, as well as the first-call-
vs-later-calls discrepancies in the examples I posted in August.  And
it doesn't break the regression tests.

However, this quick fix does not address the larger issue of whether we
need to try to make the CommandCounterIncrement-ing behavior consistent
between the cases where plpgsql has to generate a query plan and the
cases where it's already got one cached.  I think we're going to have
to come back and revisit that in a future release cycle.

Since this isn't a complete fix, I thought I'd better run it by
pgsql-hackers to see if anyone has a problem with it or sees a
better quick fix.  Any comments out there?
        regards, tom lane