Issue NOTICE for attempt to raise lock level?

I am working on eliminating the "relation NNN modified while in use"
misfeature by instead grabbing a lock on each relation at first use
in a statement, and holding that lock till end of transaction.

As anticipated, I object :-)
If you do this you will most likely add the code to the wrong places,
since what we really need is a lock for the duration of one statement only.

Otherwise you will only fix this situation for those cases where the application
is actually inside a transaction. And this is (and hopefully stays) not mandatory.

Additionally we would have the discussed lockout of admin tasks ...

Andreas

A NOTICE might be useful to users since it would complain about
deadlock-prone user-level coding practices too, such as

begin;
select * from foo; -- grabs read lock
lock table foo; -- grabs exclusive lock

Although this is deadlock prone praxis, there is no mention in any standard that
this is not allowed or depricated. Thus we are imho not allowed to issue a notice.

Of course my opinion is that you should release the lock after select
if isolation is not RR or serializable. Thus not leading to a deadlock situation.
(I do have strong feelings about this issue)

Andreas

Zeugswetter Andreas SB <ZeugswetterA@wien.spardat.at> writes:

I am working on eliminating the "relation NNN modified while in use"
misfeature by instead grabbing a lock on each relation at first use
in a statement, and holding that lock till end of transaction.

As anticipated, I object :-)

Your objection is founded on two misunderstandings. In the first place,
we are *always* inside a transaction when executing a query. It may be
an implicit one-statement transaction, but it's still a transaction.
In the second place, we already grab locks that we do not release till
end of xact for all user-level queries. The problem is that we grab
them too late, ie, in the executor. I'm just planning to move up the
grab till first use.

regards, tom lane

I am working on eliminating the "relation NNN modified while in use"
misfeature by instead grabbing a lock on each relation at first use
in a statement, and holding that lock till end of transaction.

As anticipated, I object :-)

Your objection is founded on two misunderstandings. In the first place,
we are *always* inside a transaction when executing a query. It may be
an implicit one-statement transaction, but it's still a transaction.

Ok, but I thought there was some optimization for readonly statements.

In the second place, we already grab locks that we do not release till
end of xact for all user-level queries. The problem is that we grab
them too late, ie, in the executor. I'm just planning to move up the
grab till first use.

For a "select colname from tablename" we do not currently hold any lock
until end of tx. This is the situation you described, and I am worried about.

Andreas

Tom Lane wrote:

I am not nearly as worried about long-running transactions that delay
admin actions as I am about admin actions that crash other transactions.
I do not believe it is safe to drop read locks intra-transaction, and
I am unwilling to take a chance on it being safe so close to 7.1 beta.

Will we still have readers-dont-block-writers behaviour?
i.e. will the read lock lock only rows visible to current transaction ?
Or am i talking about something completely different ?

-------------
Hannu

Zeugswetter Andreas SB <ZeugswetterA@wien.spardat.at> writes:

I am working on eliminating the "relation NNN modified

while in use"

misfeature by instead grabbing a lock on each relation at first use
in a statement, and holding that lock till end of transaction.

As anticipated, I object :-)

Your objection is founded on two misunderstandings. In the
first place, we are *always* inside a transaction when executing
a query. It may be an implicit one-statement transaction, but it's
still a transaction.
In the second place, we already grab locks that we do not release till
end of xact for all user-level queries. The problem is that we grab

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Even for select?

them too late, ie, in the executor. I'm just planning to move up the
grab till first use.

BTW, what about indices?

Vadim

Zeugswetter Andreas SB <ZeugswetterA@wien.spardat.at> writes:

Ok, but I thought there was some optimization for readonly statements.

Doesn't have anything to do with locking, only with avoiding disk
writes.

In the second place, we already grab locks that we do not release till
end of xact for all user-level queries. The problem is that we grab
them too late, ie, in the executor. I'm just planning to move up the
grab till first use.

For a "select colname from tablename" we do not currently hold any lock
until end of tx. This is the situation you described, and I am worried about.

That's a bug in itself, because the executor's read lock is grabbed by
heap_beginscan and released by heap_endscan, which means it may be
grabbed and released multiple times during a single query (think
nested-loop join). There is nothing to stop someone from, say, dropping
the entire table between scans. I intend to fix that.

I am not nearly as worried about long-running transactions that delay
admin actions as I am about admin actions that crash other transactions.
I do not believe it is safe to drop read locks intra-transaction, and
I am unwilling to take a chance on it being safe so close to 7.1 beta.
We can argue about it when 7.2 development cycle starts, if you like.

regards, tom lane

In the second place, we already grab locks that we do not release till
end of xact for all user-level queries. The problem is that we grab

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Even for select?

No. Tom plans to change that, and I still think this is a bad idea.

them too late, ie, in the executor. I'm just planning to move up the
grab till first use.

This would be good.

BTW, what about indices?

Same here, would need to be released after each readonly statement.

Andreas

Tom Lane writes:

To help debug this, I'm planning to add a little bit of code to the
lock manager that detects a request for a lock on an object on which
we already hold a lock of a lower level. What I'm wondering about is
whether to make the report be elog(DEBUG) --- ie, send to postmaster
log only --- or elog(NOTICE), so that users would see it by default.

To me this seems to be a little like the much-disputed notice for adding
implicit range-table entries: Either it's an error, then you abort, or
it's legal, then you leave the user alone and perhaps explain failure
scenarios in the documentation. At least until we have something like a
user-configurable warning level.

elog(DEBUG) might be okay, but only with a positive DebugLvl, IMHO.

--
Peter Eisentraut peter_e@gmx.net http://yi.org/peter-e/

Hannu Krosing <hannu@tm.ee> writes:

Tom Lane wrote:

I do not believe it is safe to drop read locks intra-transaction, and
I am unwilling to take a chance on it being safe so close to 7.1 beta.

Will we still have readers-dont-block-writers behaviour?

Sure. The only thing this really affects is VACUUM and schema-altering
commands, which will now have to wait until reader transactions commit.
In other words

Session 1 Session 2

BEGIN;
SELECT * FROM foo;

ALTER TABLE foo ...

...

COMMIT;

Session 2 will have to wait for session 1 to commit; before it didn't.
An example of why this is a good idea is

Session 1 Session 2

BEGIN;
DECLARE c CURSOR FOR
SELECT * FROM foo;

ALTER TABLE foo ...

FETCH FROM c;

COMMIT;

Without a held read lock on foo, session 1 is in deep trouble,
because its cursor is no longer correctly planned.

regards, tom lane

"Mikheev, Vadim" <vmikheev@SECTORBASE.COM> writes:

BTW, what about indices?

CREATE/DROP INDEX grab exclusive lock on the parent table, so there's no
problem with indexes as long as transactions that use the parent table
hold some kind of lock on that table. I figure it's OK for the executor
to continue to grab and release locks on indexes on a per-scan basis.
It's just the parent table that we need a continuous lock on.

regards, tom lane

I said:

CREATE/DROP INDEX grab exclusive lock on the parent table,

Correction: DROP INDEX grabs exclusive lock, CREATE only grabs a
ShareLock on the parent. But that's OK since addition of an index
doesn't hurt existing readers.

regards, tom lane

-----Original Message-----
From: Tom Lane
Sent: Wednesday, November 08, 2000 1:26 AM
To: pgsql-hackers@postgreSQL.org
Subject: [HACKERS] Issue NOTICE for attempt to raise lock level?

I am working on eliminating the "relation NNN modified while in use"
misfeature by instead grabbing a lock on each relation at first use
in a statement, and holding that lock till end of transaction.

Isn't "relation NNN modified while in use" itself coming from heap_
open(r) 's LockRelation_after_allocate sequence ?
Or from a rd_refcnt leak,of cource.
I'm thinking that RelationCacheInvalidate() should ignore relations
which are while in use. IMHO allocate_after_lock sequence is
needed for heap_open(r).

The
main trick here is to make sure that the first lock grabbed is adequate
--- for example, it won't do to grab AccessShareLock and then have to
raise that to AccessExclusiveLock, because there will be a deadlock if
two backends do this concurrently.

I object to you if it also includes parse_rewrite_plan stage.
If there's a long transation it would also hold a AccessShareLock
on system tables for a long time. Then vacuum for system tables
would be blocked. Other transactions would be blocked......

Regards.
Hiroshi Inoue

"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:

I am working on eliminating the "relation NNN modified while in use"
misfeature by instead grabbing a lock on each relation at first use
in a statement, and holding that lock till end of transaction.

Isn't "relation NNN modified while in use" itself coming from heap_
open(r) 's LockRelation_after_allocate sequence ?

Right. I intend to eliminate that test entirely, and simply let the
relcache update happen. With appropriate start-to-end locking, it
shouldn't be possible for a schema update to sneak in at an unsafe
point.

The only reason that there is a "modified while in use" test at all
is that I put it in awhile back as a stopgap solution until we did
a better job with the end-to-end locking problem. The reports coming
back on 7.0.* make it clear that the stopgap answer isn't good enough,
so I want to fix it right for 7.1.

I'm thinking that RelationCacheInvalidate() should ignore relations
which are while in use.

Won't work unless you somehow store an "update needed" flag to make the
update happen later --- you can't just discard a shared-inval
notification. And if you did that, you'd have synchronization issues
to contend with. Since we use SnapshotNow for reading catalogs, catalog
fetches may see data that is inconsistent with the current state of the
relcache. Not good. Forcing the schema update to be held off in the
first place seems the right answer.

I object to you if it also includes parse_rewrite_plan stage.
If there's a long transation it would also hold a AccessShareLock
on system tables for a long time.

No, I'm going to leave locking of system catalogs as-is. This basically
means that we don't support concurrent alteration of schemas for system
tables. Seems like an OK tradeoff to me ...

regards, tom lane

Tom Lane wrote:

"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:

I am working on eliminating the "relation NNN modified while in use"
misfeature by instead grabbing a lock on each relation at first use
in a statement, and holding that lock till end of transaction.

Isn't "relation NNN modified while in use" itself coming from heap_
open(r) 's LockRelation_after_allocate sequence ?

Right. I intend to eliminate that test entirely, and simply let the
relcache update happen. With appropriate start-to-end locking, it
shouldn't be possible for a schema update to sneak in at an unsafe
point.

The only reason that there is a "modified while in use" test at all
is that I put it in awhile back as a stopgap solution until we did
a better job with the end-to-end locking problem. The reports coming
back on 7.0.* make it clear that the stopgap answer isn't good enough,
so I want to fix it right for 7.1.

I'm thinking that RelationCacheInvalidate() should ignore relations
which are while in use.

Won't work unless you somehow store an "update needed" flag to make the
update happen later --- you can't just discard a shared-inval
notification.

What I mean is to change heap_open(r) like

LockRelationId(Name) -> shared-inval-handling ->
allocate the relation descriptor and increment rd_refcnt

This would ensure that relations with rd_refcnt > 0
acquire some lock. Could any shared-inval-noti
fication arrive for such relations under the me-
chanism ? However 'reset system cache' message
could arrive at any time. I've examined the error
'recursive use of cache' for some time. It seems
very difficult to avoid the error if we reconstruct
relation descriptors whose rd_refcnt > 0 in
RelationCacheInvalidate().

Comments ?

Regards.
Hiroshi Inoue

relcache. Not good. Forcing the schema update to be held off in the
first place seems the right answer.

Agreed, the only question is, how long. My idea would be until statement end,
which is also how Informix does it btw. (If you wanted a prominent example)

Of course a "statement" spans open cursor and all subsequent fetches.

Andreas

Hiroshi Inoue <Inoue@tpf.co.jp> writes:

What I mean is to change heap_open(r) like
LockRelationId(Name) -> shared-inval-handling ->
allocate the relation descriptor and increment rd_refcnt
This would ensure that relations with rd_refcnt > 0
acquire some lock. Could any shared-inval-noti
fication arrive for such relations under the me-
chanism ?

Yes, because the system doesn't make any attempt to ensure that relcache
entries are held open throughout a statement or transaction. (If they
were, we largely wouldn't have a problem.) So we can't use relcache
refcount going from 0 to 1 as the sole criterion for when to acquire
a lock.

I did look at using the relcache to control holding locks throughout
statements, but it seems that it doesn't have enough information
to grab the right kind of lock. For example, I had to modify the
parser to ensure that the right kind of lock is grabbed on the
initial relcache access, depending on whether the table involved is
accessed for plain SELECT, SELECT FOR UPDATE, or INSERT/UPDATE/DELETE.
I still have to make a similar change in the rewriter for table
references that are added to a query by rewrite. The code that is
doing this stuff knows full well that it is making the first reference
to a table, and so the relcache doesn't really have anything to
contribute.

However 'reset system cache' message
could arrive at any time. I've examined the error
'recursive use of cache' for some time. It seems
very difficult to avoid the error if we reconstruct
relation descriptors whose rd_refcnt > 0 in
RelationCacheInvalidate().

I haven't had time to look at that yet, but one possible answer is just
to disable the 'recursive use of cache' test. It's only a debugging
sanity-check anyway, not essential functionality.

regards, tom lane