Re: WIP: pl/pgsql cleanup

Neil Conway <neilc(at)samurai(dot)com> writes:
> Attached is a revised patch that incorporates your suggestions. Sorry
> for the delay in posting this.

Still a few bricks shy of a load I fear:

regression=# create or replace function foo() returns int language plpgsql as $$
regression$# begin
regression$# return ;
regression$# end$$;
CREATE FUNCTION
regression=# select foo();
server closed the connection unexpectedly

If you're going to stick a NULL into the return's expression then you'd
better check for same when it's used.

regression=# create or replace function foo() returns int language plpgsql as $$
regression$# begin
regression$# for x in
regression$# select 33
regression$# loop
regression$# end loop;
regression$# end$$;
server closed the connection unexpectedly

This one is from the Assert at line 966 failing:

TRAP: FailedAssertion("!(strcmp(expr1->query, "SELECT ") == 0)", File: "gram.y", Line: 966)

What I was actually intending to check when I ran into that is why some
of the error paths in your FOR-loop rewrite use
plpgsql_error_lineno = $1;
while others have
plpgsql_error_lineno = plpgsql_scanner_lineno();
I suspect the former is more appropriate, at least for errors that
reference the FOR variable and not some other part of the statement.
Also why the inconsistent use of yyerror() vs ereport()?

One really minor quibble:

***************
*** 524,531 ****
char *name;

plpgsql_convert_ident(yytext, &name, 1);
! /* name should be malloc'd for use as varname */
! $$.name = strdup(name);
$$.lineno = plpgsql_scanner_lineno();
pfree(name);
}
--- 496,502 ----
char *name;

plpgsql_convert_ident(yytext, &name, 1);
! $$.name = pstrdup(name);
$$.lineno = plpgsql_scanner_lineno();
pfree(name);
}

The pstrdup and pfree could be canceled out. (This seems to be the only
call of plpgsql_convert_ident where you missed this.)

regards, tom lane