WIP: pl/pgsql cleanup

Neil Conway <neilc@samurai.com> writes:

This patch makes a number of cleanups to PL/PgSQL:

- replaced all uses of malloc/strdup with palloc/pstrdup.
...
(This was surprisingly easy, btw, so I am suspect that I've missed
something fundamental -- hence the patch is marked WIP. Guidance would
be welcome.)

I think that the existing parsing code feels free to palloc junk data
that needn't be kept around as part of the finished function definition.
It might be better to keep CurrentMemoryContext pointing at a temp
context, and translate malloc() to MemoryContextAlloc(function_context)
rather than just palloc(). (Of course you could hide this in a macro,
maybe falloc()?)

Other than that consideration, I never thought it would be complex to do
this, just tedious. As long as you're sure you caught everyplace that
needs to change ...

I don't have time to study the diff now, but your summary sounds good
on the other points.

regards, tom lane

On Tue, 2005-01-18 at 01:02 -0500, Tom Lane wrote:

I think that the existing parsing code feels free to palloc junk data
that needn't be kept around as part of the finished function definition.
It might be better to keep CurrentMemoryContext pointing at a temp
context, and translate malloc() to MemoryContextAlloc(function_context)
rather than just palloc(). (Of course you could hide this in a macro,
maybe falloc()?)

Are there really enough short-lived pallocs that this is worth the
trouble? One potential issue is that there are plenty of places where
we'd want to falloc(), but don't have the function easily available
(e.g. in the parser).

Attached is a revised patch (no major changes, just grammar cleanup).
While rewriting some cruft in PL/PgSQL's gram.y, I noticed that we will
overflow a heap-allocated array if the user specifies more than 1024
parameters to a refcursor (a demonstration .sql file is also attached).
I think this is probably worth backpatching to 8.0 (and earlier?) --
I've attached a quick hack of a patch that fixes the issue, although of
course the fix for 8.1 is nicer.

-Neil

Neil Conway <neilc@samurai.com> writes:

On Tue, 2005-01-18 at 01:02 -0500, Tom Lane wrote:

It might be better to keep CurrentMemoryContext pointing at a temp
context, and translate malloc() to MemoryContextAlloc(function_context)
rather than just palloc(). (Of course you could hide this in a macro,
maybe falloc()?)

Are there really enough short-lived pallocs that this is worth the
trouble?

Not sure, but it seems like at least as straightforward a translation
as the other way. More to the point, it makes clear the difference
between what is meant to be a long-lived data structure and what isn't.

One potential issue is that there are plenty of places where
we'd want to falloc(), but don't have the function easily available
(e.g. in the parser).

Why not? You'd need to keep the context-to-use in a static variable,
but that's no great difficulty considering that plpgsql function
parsing needn't be re-entrant.

regards, tom lane

On Thu, 2005-01-20 at 15:48 +1100, Neil Conway wrote:

Attached is a revised patch (no major changes, just grammar cleanup).
While rewriting some cruft in PL/PgSQL's gram.y, I noticed that we will
overflow a heap-allocated array if the user specifies more than 1024
parameters to a refcursor (a demonstration .sql file is also attached).
I think this is probably worth backpatching to 8.0 (and earlier?) --
I've attached a quick hack of a patch that fixes the issue, although of
course the fix for 8.1 is nicer.

I've applied the minimal fix for the buffer overrun to REL7_4_STABLE and
REL8_0_STABLE (let me know if you think it's worth backpatching
further).

I'll post a revised patch for HEAD that does the falloc() stuff soon.

-Neil

On Thu, 2005-01-20 at 07:57 -0500, Tom Lane wrote:

Not sure, but it seems like at least as straightforward a translation
as the other way. More to the point, it makes clear the difference
between what is meant to be a long-lived data structure and what isn't.

The latter point is sound, I think -- making that distinction clear
would be nice.

One problem is that this prevents easily using List in pl_comp.c and
gram.y, which is a shame. One solution would be to switch the
CurrentMemoryContext to the function's memory context, and provide a
macro to allocate short-lived memory that can be thrown away at the end
of do_compile(). Alternatively, we could provide some means to allow the
caller of the List API functions to specify the context in which list.c
allocations should be performed.

Why not? You'd need to keep the context-to-use in a static variable,
but that's no great difficulty considering that plpgsql function
parsing needn't be re-entrant.

Yeah, that's fair -- there's already a plpgsql_curr_compile variable, so
we needn't even add another.

-Neil

Attached is a revised patch. Changes:

- abandonded the falloc() idea. There really aren't that many
short-lived allocations in the PL/PgSQL compiler, and using falloc()
made it difficult to use List. Instead, make the CurrentMemoryContext
the long-lived function context, and explicitly pfree short-term
allocations. Not _all_ short-lived allocations are explicitly released;
if this turns out to be a problem, it can be cleaned up later.

- Rewrite various bits of parsing code to be less brain-damaged. Fix
four buffer overruns.

- Raise an error if more RAISE parameters were specified than were
required by the RAISE format string; also raise an error if there were
more parameters required by the format string than were specified.

Barring any objections, I'll apply this to HEAD tomorrow.

-Neil

Neil Conway <neilc@samurai.com> writes:

- abandonded the falloc() idea. There really aren't that many
short-lived allocations in the PL/PgSQL compiler, and using falloc()
made it difficult to use List. Instead, make the CurrentMemoryContext
the long-lived function context, and explicitly pfree short-term
allocations. Not _all_ short-lived allocations are explicitly released;
if this turns out to be a problem, it can be cleaned up later.

My recollection is that I was not nearly as worried about short-term
pallocs in the plpgsql code itself, as about leakage in various main-
backend routines that get called incidentally during parsing.
backend/parser/ is quite cavalier about this as a whole, because it
expects to be called in contexts that are not long-lived. Have you
done any checking to ensure that the per-function context doesn't get
unreasonably bloated this way?

regards, tom lane

On Mon, 2005-02-07 at 10:41 -0500, Tom Lane wrote:

My recollection is that I was not nearly as worried about short-term
pallocs in the plpgsql code itself, as about leakage in various main-
backend routines that get called incidentally during parsing.
backend/parser/ is quite cavalier about this as a whole, because it
expects to be called in contexts that are not long-lived.

Hmmm. What about switching the CurrentMemoryContext to the function's
long-lived context when we invoke plpgsql_yyparse(), but keeping it as
the short-lived context during the rest of the compilation process? This
unfortunately complicates the memory management model still further, but
it should significantly reduce the chance of any memory leaks.

-Neil

Neil Conway <neilc@samurai.com> writes:

On Mon, 2005-02-07 at 10:41 -0500, Tom Lane wrote:

My recollection is that I was not nearly as worried about short-term
pallocs in the plpgsql code itself, as about leakage in various main-
backend routines that get called incidentally during parsing.

Hmmm. What about switching the CurrentMemoryContext to the function's
long-lived context when we invoke plpgsql_yyparse(), but keeping it as
the short-lived context during the rest of the compilation process?

Doesn't plpgsql_yyparse cover pretty much all of it? IIRC, a lot of
code in pl_comp.c is called from either the lexing or parsing code.
It's not nearly as well compartmentalized as in the main SQL parser :-(

What might work is to run in the function context as the basic state,
but switch to a short-term context when calling any main-backend code
from pl_comp.c. I'm not sure how many such calls there are, but if it's
not more than a dozen or two then this wouldn't be horridly painful.
(he says, knowing *he* doesn't have to do it...)

regards, tom lane

On Mon, 2005-02-07 at 19:22 -0500, Tom Lane wrote:

What might work is to run in the function context as the basic state,
but switch to a short-term context when calling any main-backend code
from pl_comp.c. I'm not sure how many such calls there are, but if it's
not more than a dozen or two then this wouldn't be horridly painful.

WRT calls to backend/parser, I can see LookupTypeName (pl_comp.c:1060),
and parseTypeString (pl_comp.c:1627). Are these the only calls you had
in mind, or am I missing some?

If that's all, then it would probably be doable to manually switch
contexts.

-Neil

Neil Conway <neilc@samurai.com> writes:

WRT calls to backend/parser, I can see LookupTypeName (pl_comp.c:1060),
and parseTypeString (pl_comp.c:1627). Are these the only calls you had
in mind, or am I missing some?

I haven't looked lately, but my recollection is that there are just a
few calls into the main backend from pl_comp.c. I'm not sure they are
all to backend/parser though; check /catalog, etc as well.

regards, tom lane

On Mon, 2005-02-07 at 21:25 -0500, Tom Lane wrote:

I haven't looked lately, but my recollection is that there are just a
few calls into the main backend from pl_comp.c. I'm not sure they are
all to backend/parser though; check /catalog, etc as well.

Attached is a patch that implements this. I could only find a few places
that needed to switch into the temporary context; thankfully, doing that
wasn't _too_ ugly, since it allows us to dispense with retail pfrees in
the function as well.

The patch also has mostly-finished support for better PL/PgSQL syntax
checking (per the discussion on the subject from a few months ago). My
original implementation did the syntax checking after parsing was
complete; this version does the checking in gram.y itself, so it should
hopefully be more resilient against syntax errors that confuse statement
boundaries and the like. It would have been nice to check for errors in
plpgsql_read_expression() itself (rather than adding checks in most of
its call sites), but we sometimes use plpgsql_read_expression() to read
malformed SQL (e.g. gram.y:1069) -- it might be possible to fix that. I
need to add some more regression tests and clean up the error message we
emit on a syntax error, but the rest of the work is done.

-Neil

When you apply this, please put the remaining array-overflow checks
before the overflow occurs, not after. See patches I just applied
in the back branches.

regards, tom lane

On Tue, 2005-02-08 at 13:25 -0500, Tom Lane wrote:

When you apply this, please put the remaining array-overflow checks
before the overflow occurs, not after.

Actually, my original fix _does_ check for the overflow before it
occurs. ISTM both fixes are essentially identical, although yours may be
preferable as it is slightly less confusing.

BTW, both of our fixes suffer from the deficiency that they will
actually reject input one variable too early: we disallow a SQL
statement with 1023 variables that we strictly speaking could store. But
I don't see that as a major problem.

-Neil

Neil Conway <neilc@samurai.com> writes:

BTW, both of our fixes suffer from the deficiency that they will
actually reject input one variable too early: we disallow a SQL
statement with 1023 variables that we strictly speaking could store.

Right. I thought about putting the overflow checks inside the switches
so that they wouldn't trigger on the case where we don't need another
variable ... but it doesn't seem worth the extra code to me either.

regards, tom lane

Another version of the patch is attached. Changes:

- clean up grammar so that read_sql_construct() is always called to read
a well-formed SQL expression. That way we can check for errors in
embedded SQL by just adding a single function call to
read_sql_construct(), rather than adding calls at most of its call
sites.

- changed location of array overflow checks per Tom

- mostly fixed error message formatting for syntax errors in PL/PgSQL. I
found this part of the ereport() framework rather confusing. The patch
currently emits errors like:

create function bad_sql1() returns int as $$
declare a int;
begin
a := 5;
Johnny Yuma;
a := 10;
return a;
end$$ language 'plpgsql';
ERROR: syntax error at or near "Johnny"
CONTEXT: SQL statement embedded in PL/PgSQL function "bad_sql1" near
line 4

Any suggestions for improvement would be welcome.

Barring any objections, I'd like to apply this patch to HEAD tomorrow.

-Neil

Neil Conway <neilc@samurai.com> writes:

create function bad_sql1() returns int as $$
declare a int;
begin
a := 5;
Johnny Yuma;
a := 10;
return a;
end$$ language 'plpgsql';
ERROR: syntax error at or near "Johnny"
CONTEXT: SQL statement embedded in PL/PgSQL function "bad_sql1" near
line 4

That seems like a step backwards from the current behavior:

regression=# create function bad_sql1() returns int as $$
regression$# declare a int;
regression$# begin
regression$# a := 5;
regression$# Johnny Yuma;
regression$# a := 10;
regression$# return a;
regression$# end$$ language 'plpgsql';
CREATE FUNCTION
regression=# select bad_sql1();
ERROR: syntax error at or near "Johnny" at character 1
QUERY: Johnny Yuma
CONTEXT: PL/pgSQL function "bad_sql1" line 4 at SQL statement
LINE 1: Johnny Yuma
^
regression=#

While the difference in information content may not seem great, it is a
big deal when you are talking about a small syntax error in a large SQL
command spanning many lines.

regards, tom lane

On Wed, 2005-02-09 at 23:57 -0500, Tom Lane wrote:

That seems like a step backwards from the current behavior [...]

Hmm, fair enough. Is this better?

create function bad_sql1() returns int as $$
declare a int;
begin
a := 5;
Johnny Yuma;
a := 10;
return a;
end$$ language 'plpgsql';
ERROR: syntax error at or near "Johnny" at character 1
QUERY: Johnny Yuma
CONTEXT: SQL statement embedded in PL/PgSQL function "bad_sql1" near
line 4
LINE 1: Johnny Yuma
^

-Neil

Neil Conway <neilc@samurai.com> writes:

On Wed, 2005-02-09 at 23:57 -0500, Tom Lane wrote:

That seems like a step backwards from the current behavior [...]

Hmm, fair enough. Is this better?

Yeah, looks better, though I question the use of "embedded" in the
message --- seems a bit jargony. Are you going to post a revised patch?

regards, tom lane