Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Try to acquire relation locks in RangeVarGetRelid.

  1. a modest improvement to get_object_address()

    Robert Haas <robertmhaas@gmail.com> — 2011-11-09T13:15:23Z

    I'd like to propose the attached patch, which changes
get_object_address() in a manner similar to what we did in
RangeVarGetRelid() in commit 4240e429d0c2d889d0cda23c618f94e12c13ade7.
 The basic idea is that, if we look up an object name, acquire the
corresponding lock, and then find that the object was dropped during
the lock wait, we retry the whole operation instead of emitting a
baffling error message.  Example:

rhaas=# create schema x;
CREATE SCHEMA
rhaas=# begin;
BEGIN
rhaas=# drop schema x;
DROP SCHEMA

Then, in another session:

rhaas=# comment on schema x is 'doodle';

Then, in the first session:

rhaas=# commit;
COMMIT

At this point, the first session must error out.  The current code
produces this:

ERROR:  cache lookup failed for class 2615 object 16386 subobj 0

With the attached patch, you instead get:

ERROR:  schema "x" does not exist

...which is obviously quite a bit nicer.

Also, if the concurrent transaction drops and creates the schema
instead of just dropping it, the new code will allow the operation to
succeed (with the expected results) rather than failing.

Objections?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  2. Re: a modest improvement to get_object_address()

    Cédric Villemain <cedric.villemain.debian@gmail.com> — 2011-11-09T13:37:53Z

    2011/11/9 Robert Haas <robertmhaas@gmail.com>:
> I'd like to propose the attached patch, which changes
> get_object_address() in a manner similar to what we did in
> RangeVarGetRelid() in commit 4240e429d0c2d889d0cda23c618f94e12c13ade7.
>  The basic idea is that, if we look up an object name, acquire the
> corresponding lock, and then find that the object was dropped during
> the lock wait, we retry the whole operation instead of emitting a
> baffling error message.  Example:
>
> rhaas=# create schema x;
> CREATE SCHEMA
> rhaas=# begin;
> BEGIN
> rhaas=# drop schema x;
> DROP SCHEMA
>
> Then, in another session:
>
> rhaas=# comment on schema x is 'doodle';
>
> Then, in the first session:
>
> rhaas=# commit;
> COMMIT
>
> At this point, the first session must error out.  The current code
> produces this:
>
> ERROR:  cache lookup failed for class 2615 object 16386 subobj 0
>
> With the attached patch, you instead get:
>
> ERROR:  schema "x" does not exist
>
> ...which is obviously quite a bit nicer.
>
> Also, if the concurrent transaction drops and creates the schema
> instead of just dropping it, the new code will allow the operation to
> succeed (with the expected results) rather than failing.
>
> Objections?

Maybe I miss something but:
The ERROR message is misleading:  the schema 'x' does exist. And also
why a drop schema would fail and a drop+create would success ?!


>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>



-- 
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

  3. Re: a modest improvement to get_object_address()

    Robert Haas <robertmhaas@gmail.com> — 2011-11-09T14:21:22Z

    On Wed, Nov 9, 2011 at 8:37 AM, Cédric Villemain
<cedric.villemain.debian@gmail.com> wrote:
> Maybe I miss something but:
> The ERROR message is misleading:  the schema 'x' does exist.

No, it doesn't.  The concurrent transaction has dropped it.

> And also
> why a drop schema would fail and a drop+create would success ?!

Because you can't comment on a schema that doesn't exist any more, but
you can comment on one that does.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  4. Re: a modest improvement to get_object_address()

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-11-09T14:54:47Z

    Robert Haas <robertmhaas@gmail.com> writes:
> I'd like to propose the attached patch, which changes
> get_object_address() in a manner similar to what we did in
> RangeVarGetRelid() in commit 4240e429d0c2d889d0cda23c618f94e12c13ade7.

I would think you need to drop the now-useless lock, and I sure hope
that RangeVarGetRelid does likewise.

			regards, tom lane

  5. Re: a modest improvement to get_object_address()

    Robert Haas <robertmhaas@gmail.com> — 2011-11-09T15:03:55Z

    On Wed, Nov 9, 2011 at 9:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I'd like to propose the attached patch, which changes
>> get_object_address() in a manner similar to what we did in
>> RangeVarGetRelid() in commit 4240e429d0c2d889d0cda23c618f94e12c13ade7.
>
> I would think you need to drop the now-useless lock, and I sure hope
> that RangeVarGetRelid does likewise.

It doesn't currently.  The now-useless lock doesn't really hurt
anything, aside from taking up space in the lock table.  But we can
certainly make it (and this) do that, if you think it's worth the
extra lines of code.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  6. Re: a modest improvement to get_object_address()

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-11-09T15:33:10Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Nov 9, 2011 at 9:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I would think you need to drop the now-useless lock, and I sure hope
>> that RangeVarGetRelid does likewise.

> It doesn't currently.  The now-useless lock doesn't really hurt
> anything, aside from taking up space in the lock table.

Well, there are corner cases where the object OID gets reused during
the lifetime of the transaction, and then the lock *does* do something
(and what it does would be bad).  But taking up extra space in the
finite-size lock table is sufficient reason IMO to drop the lock.
It's not like these are performance-critical code paths.

			regards, tom lane

  7. Re: a modest improvement to get_object_address()

    Cédric Villemain <cedric.villemain.debian@gmail.com> — 2011-11-09T15:50:22Z

    2011/11/9 Robert Haas <robertmhaas@gmail.com>:
> On Wed, Nov 9, 2011 at 8:37 AM, Cédric Villemain
> <cedric.villemain.debian@gmail.com> wrote:
>> Maybe I miss something but:

I read that the error was produced by first session and didn't check
carefuly (it fails silently in 9.0! and 'works' as expected in 9.1)

No objection, but I would like to still be able to diagnose the same
things as in the past, can you make it clear that the schema/object
just disappear ? (else we don't know if the relation just never exists
or was drop while we were waiting)

-- 
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

  8. Re: a modest improvement to get_object_address()

    Robert Haas <robertmhaas@gmail.com> — 2011-11-09T16:00:09Z

    On Wed, Nov 9, 2011 at 10:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, Nov 9, 2011 at 9:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I would think you need to drop the now-useless lock, and I sure hope
>>> that RangeVarGetRelid does likewise.
>
>> It doesn't currently.  The now-useless lock doesn't really hurt
>> anything, aside from taking up space in the lock table.
>
> Well, there are corner cases where the object OID gets reused during
> the lifetime of the transaction, and then the lock *does* do something
> (and what it does would be bad).  But taking up extra space in the
> finite-size lock table is sufficient reason IMO to drop the lock.
> It's not like these are performance-critical code paths.

OK.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: a modest improvement to get_object_address()

    Robert Haas <robertmhaas@gmail.com> — 2011-11-09T16:13:09Z

    On Wed, Nov 9, 2011 at 10:50 AM, Cédric Villemain
<cedric.villemain.debian@gmail.com> wrote:
> 2011/11/9 Robert Haas <robertmhaas@gmail.com>:
>> On Wed, Nov 9, 2011 at 8:37 AM, Cédric Villemain
>> <cedric.villemain.debian@gmail.com> wrote:
>>> Maybe I miss something but:
>
> I read that the error was produced by first session and didn't check
> carefuly (it fails silently in 9.0! and 'works' as expected in 9.1)
>
> No objection, but I would like to still be able to diagnose the same
> things as in the past, can you make it clear that the schema/object
> just disappear ? (else we don't know if the relation just never exists
> or was drop while we were waiting)

I don't see a clean way to do that, and I'm not convinced it's a good
idea anyway.  I think that if we start generating different error
messages based on whether or not a lock wait was involved at some
point in the operation, we're going to drive ourselves nuts.  There
are a lot of places where that can happen.

e.g. Suppose that you have a table with a unique index on column a.
Transaction A deletes the tuple where a = 1. Transaction B attempts to
insert a new tuple with a = 1, and blocks.  Now if A commits, B will
succeed, but if A rolls back, B will abort.  Had transaction A not
existed, B would simply abort at once.  But the error message will not
indicate which of the two it was, and I don't thinkit needs to.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  10. Re: a modest improvement to get_object_address()

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-11-09T20:40:51Z

    Robert Haas <robertmhaas@gmail.com> writes:
> e.g. Suppose that you have a table with a unique index on column a.
> Transaction A deletes the tuple where a = 1. Transaction B attempts to

That's DML, I agree with you there, no need.  In DML we have MVCC.

Back to the problem you raised, it's DDL and we're sitting in between
SnapshotNow and catalog cache entries.  Not so comfy.  I would guess
that the problem (I confess didn't read carefully enough) happens after
having done a cache lookup when trying to use its result?

Could we check the object still exists as part of the cache lookup, or
would that mean we don't have a cache anymore?  Or is the answer related
to consuming invalidation messages before returning a stale entry from
the cache?

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  11. Re: a modest improvement to get_object_address()

    Robert Haas <robertmhaas@gmail.com> — 2011-11-09T21:04:37Z

    On Wed, Nov 9, 2011 at 3:40 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
> Back to the problem you raised, it's DDL and we're sitting in between
> SnapshotNow and catalog cache entries.  Not so comfy.  I would guess
> that the problem (I confess didn't read carefully enough) happens after
> having done a cache lookup when trying to use its result?

There's a test case in the original post, but yes, the problem happens
when something changes between the time you do the catcache lookup and
the time you acquire the lock.  This is not a new problem; I'm just
trying to give a more intelligible error message - and avoid
unnecessary failures, as in the case where two concurrent DROP IF
EXISTS operations target the same object and one of them unnecessarily
rolls back.

> Could we check the object still exists as part of the cache lookup, or
> would that mean we don't have a cache anymore?  Or is the answer related
> to consuming invalidation messages before returning a stale entry from
> the cache?

All of that is way beyond the scope of what I'm doing here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company