Thread: Re: [PATCHES] Important 7.0.* fix to ensure buffers are released

[ Cc: to hackers list]

> I think the primary problems had to do with recursive calls to
> ExecutorRun, which'd invoke the badly broken buffer refcount save/
> restore mechanism that was present in 6.5 and earlier.  This would
> mainly be done by SQL and PL functions that do SELECTs.  A couple
> of examples:
>   * elog(ERROR) from inside an SQL function would mean that buffer
>     refcounts held by the outer scan wouldn't be released.  So, eg,
>     SELECT sqlfunction(column1) FROM foo;
>     was a buffer leak risk.

Following case doesn't produce notices from BlowawayRelationBuffers.

drop table t1;
create table t1(i int);
drop table t2;
create table t2(i int);
insert into t1 values(1);
drop function f1(int);
create function f1(int) returns int as '   select $1 +1;   select i from t2;
' language 'sql';
drop table t2;
select f1(i) from t1;
delete from t1;
vacuum t1;

Am I missing something?

>   * SQL functions returning sets could leak even without any elog(),
>     if the entire set result was not read for some reason.

However, following case produces:

NOTICE:  BlowawayRelationBuffers(t1, 0): block 0 is referenced...

as expected.

drop table t1;
create table t1(i int);
insert into t1 values(1);
insert into t1 select i from t1;
insert into t1 select i from t1;
drop function f1(int);
create function f1(int) returns setof int as '   select i from t1;
' language 'sql';
select f1(i) from t1 limit 1 offset 0;
delete from t1;
vacuum analyze t1;

Interesting thing is that the select in above case produces a
notice in 7.0.2 (with or without your patches):

NOTICE:  Buffer Leak: [059] (freeNext=-3, freePrev=-3, relname=t1, blockNum=0, flags=0x4, refcount=1 2)

while 6.5.3 does not. Maybe 6.5.3 failes to detect buffer leaks at
transaction commit time?
--
Tatsuo Ishii

t-ishii@sra.co.jp writes:
> Am I missing something?

I don't have 6.5.* running anymore to check, but it looked to me like
elog out of an SQL function would result in refcount leaks.  But the
elog would have to occur while inside the function's recursive call
to ExecutorRun, so your example (which will detect its error during
query plan setup) doesn't exhibit the problem.  Try something likeselect 1/0;
inside the function.

> Interesting thing is that the select in above case produces a
> notice in 7.0.2 (with or without your patches):

Yes, still there in current sources.  The leak comes from the fact
that the function's internal SELECT is never shut down because the
function isn't run to completion.  This is one of the things I think we
need to fix during querytree redesign.  However, 7.0 at least detects
and recovers from the leak, which is more than can be said for 6.5.

> while 6.5.3 does not. Maybe 6.5.3 failes to detect buffer leaks at
> transaction commit time?

In fact it does fail to detect them, in cases like this where the leak
is attributable to an uncompleted query inside a function call.  That's
one of the things that was broken about the refcount save/restore
mechanism...
        regards, tom lane

> I don't have 6.5.* running anymore to check, but it looked to me like
> elog out of an SQL function would result in refcount leaks.  But the
> elog would have to occur while inside the function's recursive call
> to ExecutorRun, so your example (which will detect its error during
> query plan setup) doesn't exhibit the problem.  Try something like
>     select 1/0;
> inside the function.

Oh, I see.

> > Interesting thing is that the select in above case produces a
> > notice in 7.0.2 (with or without your patches):
> 
> Yes, still there in current sources.  The leak comes from the fact
> that the function's internal SELECT is never shut down because the
> function isn't run to completion.  This is one of the things I think we
> need to fix during querytree redesign.  However, 7.0 at least detects
> and recovers from the leak, which is more than can be said for 6.5.

Agreed.

> > while 6.5.3 does not. Maybe 6.5.3 failes to detect buffer leaks at
> > transaction commit time?
> 
> In fact it does fail to detect them, in cases like this where the leak
> is attributable to an uncompleted query inside a function call.  That's
> one of the things that was broken about the refcount save/restore
> mechanism...

Understood.
--
Tatsuo Ishii