Internal state in sequence.c is a bad idea - Mailing list pgsql-hackers

From Tom Lane
Subject Internal state in sequence.c is a bad idea
Date
Msg-id 6356.1022084360@sss.pgh.pa.us
Whole thread Raw
List pgsql-hackers
From a recent bug report:

test72=# create sequence s;
CREATE
test72=# begin;
BEGIN
test72=# select nextval('s');nextval
---------      1
(1 row)

test72=# drop sequence s;
DROP
test72=# end;
NOTICE:  LockRelease: no such lock
COMMIT

The NOTICE is a symptom of bad problems underneath (memory corruption
eventually leading to coredump, in the bug report's example).

The problem is that after nextval(), sequence.c is holding on to a
pointer to the open relcache entry for the sequence, which it intends
to close at end of transaction.  After the sequence is dropped, the
pointer is just a dangling pointer, and so the eventual close is working
on freed memory, with usually-unpleasant consequences.

I think the best solution for this is to get rid of sequence.c's private
state list.  We could allow the state info for a sequence to be kept
right in the relcache entry (or more likely, in a struct owned and
pointed to by the relcache entry --- that way, the overhead added for
non-sequence relcache entries would only be one more pointer field).
Relcache deletion would know to free the sequence state object along
with the rest of the relcache entry.  As for the action of closing the
relcache entry, I don't see any good reason for sequence.c to hold
relcache entries open between calls anyway.  It is good to hold the
AccessShareLock till commit, but the lock manager can release that lock
at commit by itself; there's no reason at all for sequence.c to do it.

Comments?
        regards, tom lane


pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: Killing dead index tuples before they get vacuumed
Next
From: Jan Wieck
Date:
Subject: Re: Killing dead index tuples before they get vacuumed