Horton

Thread

  1. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-06T02:13:21Z

    "Kevin Grittner" wrote:

> Maybe I should submit a patch without added complexity of the
> scheduled cleanup and we can discuss that as a separate patch?

Here's a patch which adds the missing support for DDL. Cleanup of
predicate locks at commit time for transactions which ran DROP TABLE
or TRUNCATE TABLE can be added as a separate patch. I consider those
to be optimizations which are of dubious benefit, especially compared
to the complexity of the extra code required.

Also, Heikki pointed out that explicitly releasing the predicate
locks after a DROP DATABASE is an optimization, which on reflection
also seems to be of dubious value compared to the code needed.

Unless someone can find a way in which any of these early cleanups
are needed for correctness, I suggest they are better left as
enhancements in some future release, where there can be some
demonstration that they matter enough for performance to justify the
extra code, and there can be more testing to ensure the new code
doesn't break anything.

I stashed the partial work on the more aggressive cleanup, so if
there's a consensus that we want it, I can post a patch pretty
quickly.
 
In making sure that the new code for this patch was in pgindent
format, I noticed that the ASCII art and bullet lists recently added
to OnConflict_CheckForSerializationFailure() were mangled badly by
pgindent, so I added the dashes to protect those and included a
pgindent form of that function.  That should save someone some
trouble sorting things out after the next global pgindent run.

-Kevin

  2. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-06-07T07:55:05Z

    On 06.06.2011 05:13, Kevin Grittner wrote:
> "Kevin Grittner" wrote:
>
>> Maybe I should submit a patch without added complexity of the
>> scheduled cleanup and we can discuss that as a separate patch?
>
> Here's a patch which adds the missing support for DDL.

It makes me a bit uncomfortable to do catalog cache lookups while 
holding all the lwlocks. We've also already removed the reserved entry 
for scratch space while we do that - if a cache lookup errors out, we'll 
leave behind quite a mess. I guess it shouldn't fail, but it seems a bit 
fragile.

When TransferPredicateLocksToHeapRelation() is called for a heap, do we 
really need to transfer all the locks on its indexes too? When a heap is 
dropped or rewritten or truncated, surely all its indexes are also 
dropped or reindexed or truncated, so you'll get separate Transfer calls 
for each index anyway. I think the logic is actually wrong at the 
moment: When you reindex a single index, 
DropAllPredicateLocksFromTableImpl() will transfer all locks belonging 
to any index on the same table, and any finer-granularity locks on the 
heap. It would be enough to transfer only locks on the index being 
reindexed, so you risk getting some unnecessary false positives. As a 
bonus, if you dumb down DropAllPredicateLocksFromTableImpl() to only 
transfer locks on the given heap or index, and not any other indexes on 
the same table, you won't need IfIndexGetRelation() anymore, making the 
issue of catalog cache lookups moot.

Seems weird to call SkipSplitTracking() for heaps. I guess it's doing 
the right thing, but all the comments and the name of that refer to indexes.

> Cleanup of
> predicate locks at commit time for transactions which ran DROP TABLE
> or TRUNCATE TABLE can be added as a separate patch. I consider those
> to be optimizations which are of dubious benefit, especially compared
> to the complexity of the extra code required.

Ok.

> In making sure that the new code for this patch was in pgindent
> format, I noticed that the ASCII art and bullet lists recently added
> to OnConflict_CheckForSerializationFailure() were mangled badly by
> pgindent, so I added the dashes to protect those and included a
> pgindent form of that function.  That should save someone some
> trouble sorting things out after the next global pgindent run.

Thanks, committed that part.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  3. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-07T14:20:13Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> It makes me a bit uncomfortable to do catalog cache lookups while 
> holding all the lwlocks. We've also already removed the reserved entry 
> for scratch space while we do that - if a cache lookup errors out, we'll 
> leave behind quite a mess. I guess it shouldn't fail, but it seems a bit 
> fragile.

The above scares the heck out of me.  If you don't believe that a
catcache lookup will ever fail, I will contract to break the patch.
You need to rearrange the code so that this is less fragile.

			regards, tom lane

  4. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-07T14:56:42Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
 
> If you don't believe that a catcache lookup will ever fail, I will
> contract to break the patch.
 
As you probably know by now by reaching the end of the thread, this
code is going away based on Heikki's arguments; but for my
understanding, so that I don't make a bad assumption in this area
again, what could cause the following function to throw an exception
if the current process is holding an exclusive lock on the relation
passed in to it?  (I could be a heap or an index relation.)  It
seemed safe to me, and I can't spot the risk on a scan of the called
functions.  What am I missing?
 
static Oid
IfIndexGetRelation(Oid indexId)
{
    HeapTuple tuple;
    Form_pg_index index;
    Oid result;

    tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexId));
    if (!HeapTupleIsValid(tuple))
        return InvalidOid;

    index = (Form_pg_index) GETSTRUCT(tuple);
    Assert(index->indexrelid == indexId);

    result = index->indrelid;
    ReleaseSysCache(tuple);
    return result;
}
 
Thanks for any clues,
 
-Kevin

  5. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-07T15:11:16Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If you don't believe that a catcache lookup will ever fail, I will
>> contract to break the patch.
 
> As you probably know by now by reaching the end of the thread, this
> code is going away based on Heikki's arguments; but for my
> understanding, so that I don't make a bad assumption in this area
> again, what could cause the following function to throw an exception
> if the current process is holding an exclusive lock on the relation
> passed in to it?  (I could be a heap or an index relation.)  It
> seemed safe to me, and I can't spot the risk on a scan of the called
> functions.  What am I missing?

Out-of-memory.  Query cancel.  The attempted catalog access failing
because it results in a detected deadlock.  I could probably think of
several more if I spent ten minutes on it; and that's not even
considering genuine problem conditions such as a corrupted catalog
index, which robustness demands that we not fall over completely for.

You should never, ever assume that an operation as complicated as a
catalog lookup can't fail.

			regards, tom lane

  6. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-07T15:29:10Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
 
>> What am I missing?
> 
> Out-of-memory.  Query cancel.  The attempted catalog access
> failing because it results in a detected deadlock.  I could
> probably think of several more if I spent ten minutes on it; and
> that's not even considering genuine problem conditions such as a
> corrupted catalog index, which robustness demands that we not fall
> over completely for.
> 
> You should never, ever assume that an operation as complicated as
> a catalog lookup can't fail.
 
Got it.  Thanks.
 
-Kevin

  7. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-07T17:03:29Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
 
> We've also already removed the reserved entry for scratch space
 
This and Tom's concerns have me wondering if we should bracket the
two sections of code where we use the reserved lock target entry
with HOLD_INTERRUPTS() and RESUME_INTERRUPTS().  In an assert-enable
build we wouldn't really recover from a transaction canceled while
it was checked out (although if that were the only problem, that
could be fixed), but besides that a cancellation while it's checked
out could cause these otherwise-safe functions to throw exceptions
due to a full heap table.
 
-Kevin

  8. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-06-07T17:10:12Z

    On 07.06.2011 20:03, Kevin Grittner wrote:
> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  wrote:
>
>> We've also already removed the reserved entry for scratch space
>
> This and Tom's concerns have me wondering if we should bracket the
> two sections of code where we use the reserved lock target entry
> with HOLD_INTERRUPTS() and RESUME_INTERRUPTS().

That's not necessary. You're holding a lwlock, which implies that 
interrupts are held off already. There's a HOLD_INTERRUPTS() call in 
LWLockAcquire and RESUME_INTERRUPTS() in LWLockRelease.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  9. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-07T17:42:02Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
 
> It makes me a bit uncomfortable to do catalog cache lookups while 
> holding all the lwlocks.
 
I think I've caught up with the rest of the class on why this isn't
sane in DropAllPredicateLocksFromTableImpl, but I wonder about
CheckTableForSerializableConflictIn.  We *do* expect to be throwing
errors in here, and we need some way to tell whether an index is
associated with a particular heap relation.  Is the catalog cache
the right way to check that here, or is something else more
appropriate?
 
-Kevin

  10. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-06-07T18:00:10Z

    On 07.06.2011 20:42, Kevin Grittner wrote:
> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  wrote:
>
>> It makes me a bit uncomfortable to do catalog cache lookups while
>> holding all the lwlocks.
>
> I think I've caught up with the rest of the class on why this isn't
> sane in DropAllPredicateLocksFromTableImpl, but I wonder about
> CheckTableForSerializableConflictIn.  We *do* expect to be throwing
> errors in here, and we need some way to tell whether an index is
> associated with a particular heap relation.  Is the catalog cache
> the right way to check that here, or is something else more
> appropriate?

Hmm, it's not as dangerous there, as you're not in the middle of 
modifying stuff, but it doesn't feel right there either.

Predicate locks on indexes are only needed to lock key ranges, to notice 
later insertions into the range, right? For locks on tuples that do 
exist, we have locks on the heap. If we're just about to delete every 
tuple in the heap, that doesn't need to conflict with any locks on 
indexes, because we're deleting, not inserting. So I don't think we need 
to care about index locks here at all, only locks on the heap. Am I 
missing something?

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  11. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-07T18:10:05Z

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> I think I've caught up with the rest of the class on why this isn't
> sane in DropAllPredicateLocksFromTableImpl, but I wonder about
> CheckTableForSerializableConflictIn.  We *do* expect to be throwing
> errors in here, and we need some way to tell whether an index is
> associated with a particular heap relation.  Is the catalog cache
> the right way to check that here, or is something else more
> appropriate?

Just to answer the question (independently of Heikki's concern about
whether this is needed at all): it depends on the information you have.
If all you have is the index OID, then yeah a catcache lookup in
pg_index is probably the best thing.  If you have an open Relation for
the index, you could instead look into its cached copy of its pg_index
row.  If you have an open Relation for the table, I'd think that looking
for a match in RelationGetIndexList() would be the cheapest, since more
than likely that information is already cached.

			regards, tom lane

  12. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-07T18:10:06Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
 
> Predicate locks on indexes are only needed to lock key ranges, to
> notice later insertions into the range, right? For locks on tuples
> that do exist, we have locks on the heap. If we're just about to
> delete every tuple in the heap, that doesn't need to conflict with
> any locks on indexes, because we're deleting, not inserting. So I
> don't think we need to care about index locks here at all, only
> locks on the heap. Am I missing something?
 
You're right again.  My brain must be turning to mush.  This
function can also become simpler, and there is now no reason at all
to add catalog cache lookups to predicate.c.  I think that leaves me
with all the answers I need to get a new patch out this evening
(U.S. Central Time).
 
Thanks,
 
-Kevin

  13. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-07T18:21:10Z

    Tom Lane <tgl@sss.pgh.pa.us> wrote:
 
> Just to answer the question (independently of Heikki's concern
> about whether this is needed at all): it depends on the
> information you have. If all you have is the index OID, then yeah
> a catcache lookup in pg_index is probably the best thing.  If you
> have an open Relation for the index, you could instead look into
> its cached copy of its pg_index row.  If you have an open Relation
> for the table, I'd think that looking for a match in
> RelationGetIndexList() would be the cheapest, since more than
> likely that information is already cached.
 
Thanks, I wasn't aware of RelationGetIndexList().  That's a good one
to remember.
 
The issue here was: given a particular heap relation, going through
a list of locks looking for matches, where the lock targets use
OIDs.  So if I really *did* need to check whether an index was
related to that heap relation, it sounds like the catcache approach
would have been right.  But as Heikki points out, the predicate
locks on the indexes only need to guard scanned *gaps* against
*insert* -- so a DROP TABLE or TRUNCATE TABLE can just skip looking
at any locks which aren't against the heap relation.
 
I think maybe I need a vacation -- you know, one where I'm not using
my vacation time to attend a PostgreSQL conference.
 
Thanks for the overview of what to use when; it takes a while, but I
think I'm gradually coming up to speed on the million lines of code
which comprise PostgreSQL.  Tips like that do help.
 
-Kevin

  14. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-06-07T18:38:56Z

    On 07.06.2011 21:10, Kevin Grittner wrote:
> I think that leaves me
> with all the answers I need to get a new patch out this evening
> (U.S. Central Time).

Great, I'll review it in my morning (in about 12h)

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  15. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-08T00:16:01Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
> On 07.06.2011 21:10, Kevin Grittner wrote:
 
>> I think that leaves me with all the answers I need to get a new
>> patch out this evening (U.S. Central Time).
> 
> Great, I'll review it in my morning (in about 12h)
 
Attached.  Passes all the usual regression tests I run, plus light
ad hoc testing.  All is working fine as far as this patch itself
goes, although more testing is needed to really call it sound.
 
If anyone is interested in the differential from version 3 of the
patch, it is the result of these two commits to my local repo:
 
http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=018b0fcbeba05317ba7066e552efe9a04e6890d9
http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=fc651e2721a601ea806cf6e5d53d0676dfd26dca
 
During testing I found two annoying things not caused by this patch
which should probably be addressed in 9.1 if feasible, although I
don't think they rise to the level of blockers.  More on those in
separate threads.
 
-Kevin

  16. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-06-08T11:18:33Z

    On 08.06.2011 03:16, Kevin Grittner wrote:
> + 			/*
> + 			 * It's OK to remove the old lock first because of the ACCESS
> + 			 * EXCLUSIVE lock on the heap relation when this is called.  It is
> + 			 * desirable to do so because it avoids any chance of running out
> + 			 * of lock structure entries for the table.
> + 			 */

That assumption is wrong, for REINDEX. REINDEX INDEX holds an 
AccessExclusive on the index, but only a ShareLock on the heap. The code 
is still OK, though. As long as you hold an AccessExclusiveLock on the 
index, no-one will care about predicate locks on the index, so we can 
remove them before acquiring the lock on the heap. And we hold lwlocks 
on all the predicate lock partitions, so all the operations will appear 
atomic to any outside observer anyway.

Committed after adjusting that comment. I did a lot of other cosmetic 
changes too, please double-check that I didn't screw up anything.

I also made rewriting ALTER TABLE to behave like CLUSTER, by adding a 
call to TransferPredicateLocksToHeapRelation() there. I just looked back 
at your old email where you listed the different DDL operations, and 
notice that we missed VACUUM FULL as well 
(http://archives.postgresql.org/message-id/4DBD7E91020000250003D0D6@gw.wicourts.gov). 
I'll look into that. ALTER INDEX was also still an open question in that 
email.

BTW, truncating the tail of a heap in vacuum probably also should drop 
any locks on the truncated part of the heap. Or is it not possible any 
such locks to exist, because none of the tuples are visible to anyone 
anymore? A comment in lazy_truncate_heap() might be in order.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  17. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-06-08T11:26:16Z

    On 08.06.2011 14:18, Heikki Linnakangas wrote:
> I just looked back
> at your old email where you listed the different DDL operations, and
> notice that we missed VACUUM FULL as well
> (http://archives.postgresql.org/message-id/4DBD7E91020000250003D0D6@gw.wicourts.gov).
> I'll look into that.

Never mind that, VACUUM FULL uses cluster_rel(), so that's covered already.

> ALTER INDEX was also still an open question in that email.

None of the variants of ALTER INDEX do anything that affects predicate 
locks, so that's OK too.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  18. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-06-08T11:30:20Z

    (sorry for repeatedly replying to self. I'll go for a coffee after this...)

On 08.06.2011 14:18, Heikki Linnakangas wrote:
> Committed after adjusting that comment. I did a lot of other cosmetic
> changes too, please double-check that I didn't screw up anything.

Also, it would be nice to have some regression tests for this. I don't 
think any of the existing tests exercise these new functions (except for 
the fast-exit, which isn't very interesting).

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  19. Re: SIREAD lock versus ACCESS EXCLUSIVE lock

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-06-08T14:46:08Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
 
> (sorry for repeatedly replying to self. I'll go for a coffee after
> this...)
 
That's so nice of you to try to make me feel better for the serious
brain fade I suffered yesterday.  ;-)
 
> On 08.06.2011 14:18, Heikki Linnakangas wrote:
>> Committed after adjusting that comment. I did a lot of other
>> cosmetic changes too, please double-check that I didn't screw up
>> anything.
 
On a first read-through, all your changes look like improvements to
me.  I'll do some testing and give it a closer read.
 
> Also, it would be nice to have some regression tests for this. I
> don't think any of the existing tests exercise these new functions
> (except for the fast-exit, which isn't very interesting).
 
I'll see about posting regression tests for this, as well as looking
into the questions in your earlier posts -- particularly about the
heap truncation in vacuum.  I'm pretty sure that vacuum can't clean
up anything where we're still holding predicate locks because of
visibility rules, but I'll take another look to be sure.  I hope to
do all that today, but I don't want to push to the point where I'm
making dumb mistakes again.
 
-Kevin