Thread: mvcc catalo gsnapshots and TopTransactionContext
Hi, since the mvcc catalog patch has gone in we require all users of systable_* to be in a valid transaction since the snapshot is copied via CopySnapshot() in RegisterSnapshot(). Which we call in systable_beginscan(). CopySnapshot() allocates the copied snapshot in TopTransactionContext. There doesn't seem be an explicitly stated rule that we cannot use the syscaches outside of a transaction - but effectively that's required atm. Not having investigated at all so far I am not sure how much in core code that breaks, but it certainly broke some out of tree code of mine (bidirectional replication stuff, bdr branch on git.pg.o). Is that acceptable? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Thu, 2013-07-11 at 10:28 +0200, Andres Freund wrote: > There doesn't seem be an explicitly stated rule that we cannot use the > syscaches outside of a transaction - but effectively that's required > atm. Aren't there other things that already required that before the MVCC catalog snapshot patch went in? For instance, if you get a syscache miss, you have to load from the catalogs, meaning you need to acquire a lock. I've seen problems related to that in the past: http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ac63dca607e8e22247defbc8fe03b6baa3628c42 Regards,Jeff Davis
Andres Freund <andres@2ndquadrant.com> writes:
> since the mvcc catalog patch has gone in we require all users of
> systable_* to be in a valid transaction since the snapshot is copied via
> CopySnapshot() in RegisterSnapshot().
It never has been, and never will be, allowed to call the catcache code
without being in a transaction. What do you think will happen if the
requested row isn't in cache? A table access, that's what, and that
absolutely requires being in a transaction.
regards, tom lane
Hi, On 2013-07-11 11:53:57 -0700, Jeff Davis wrote: > On Thu, 2013-07-11 at 10:28 +0200, Andres Freund wrote: > > There doesn't seem be an explicitly stated rule that we cannot use the > > syscaches outside of a transaction - but effectively that's required > > atm. > > Aren't there other things that already required that before the MVCC > catalog snapshot patch went in? For instance, if you get a syscache > miss, you have to load from the catalogs, meaning you need to acquire a > lock. There are. Several. I am blaming it on conference induced haze. Or such. On 2013-07-11 15:09:45 -0400, Tom Lane wrote: > It never has been, and never will be, allowed to call the catcache code > without being in a transaction. What do you think will happen if the > requested row isn't in cache? A table access, that's what, and that > absolutely requires being in a transaction. Makes sense. I was confused because I thought I saw a get_database_name() and other users outside of a transaction but that turned out not looking closely enough. I'd like to add an Assert like in the attached patch making sure we're in a transaction. Otherwise it's far too easy not to hit an error during development because everything is cached - and syscache usage isn't always obvious from the outside to the naive or the tired. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On Fri, Jul 12, 2013 at 5:42 AM, Andres Freund <andres@2ndquadrant.com> wrote: > I'd like to add an Assert like in the attached patch making sure we're > in a transaction. Otherwise it's far too easy not to hit an error during > development because everything is cached - and syscache usage isn't > always obvious from the outside to the naive or the tired. Seems like a good idea to me. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Jul 12, 2013 at 11:42:23AM +0200, Andres Freund wrote: > On 2013-07-11 15:09:45 -0400, Tom Lane wrote: > > It never has been, and never will be, allowed to call the catcache code > > without being in a transaction. What do you think will happen if the > > requested row isn't in cache? A table access, that's what, and that > > absolutely requires being in a transaction. > I'd like to add an Assert like in the attached patch making sure we're > in a transaction. Otherwise it's far too easy not to hit an error during > development because everything is cached - and syscache usage isn't > always obvious from the outside to the naive or the tired. The following test case reliably hits this new assertion: create table t (c int); begin; create index on t (c); savepoint q; insert into t values (1); select 1/0; Stack trace: #2 0x0000000000761159 in ExceptionalCondition (conditionName=<value optimized out>, errorType=<value optimized out>, fileName=<valueoptimized out>, lineNumber=<value optimized out>) at assert.c:54 #3 0x000000000074fc53 in SearchCatCache (cache=0xc53048, v1=139479, v2=0, v3=0, v4=0) at catcache.c:1072 #4 0x000000000075c011 in SearchSysCache (cacheId=0, key1=369539, key2=6, key3=18446744073709551615, key4=0) at syscache.c:909 #5 0x0000000000757e57 in RelationReloadIndexInfo (relation=0x7f2dd5fffea0) at relcache.c:1770 #6 0x000000000075817b in RelationClearRelation (relation=0x7f2dd5fffea0, rebuild=1 '\001') at relcache.c:1926 #7 0x00000000007588c6 in RelationFlushRelation (relationId=139479) at relcache.c:2076 #8 RelationCacheInvalidateEntry (relationId=139479) at relcache.c:2138 #9 0x000000000075185d in LocalExecuteInvalidationMessage (msg=0xcde778) at inval.c:546 #10 0x0000000000751185 in ProcessInvalidationMessages (hdr=0xc0d390, func=0x7517d0 <LocalExecuteInvalidationMessage>) atinval.c:422 #11 0x0000000000751a3f in AtEOSubXact_Inval (isCommit=0 '\000') at inval.c:973 #12 0x00000000004cb986 in AbortSubTransaction () at xact.c:4250 #13 0x00000000004cbf05 in AbortCurrentTransaction () at xact.c:2863 #14 0x00000000006968f6 in PostgresMain (argc=1, argv=0x7fff7cf71610, dbname=0xc13b60 "test", username=0xbedc88 "nm") at postgres.c:3848 #15 0x000000000064c2b5 in BackendRun () at postmaster.c:4044 #16 BackendStartup () at postmaster.c:3733 #17 ServerLoop () at postmaster.c:1571 #18 0x000000000064fa8d in PostmasterMain (argc=1, argv=0xbe84a0) at postmaster.c:1227 #19 0x00000000005e2dcd in main (argc=1, argv=0xbe84a0) at main.c:196 When we call AtEOSubXact_Inval() or AtEOXact_Inval() with a relation still open, we can potentially get a relcache rebuild and therefore a syscache lookup as shown above. CommitSubTransaction() is also potentially affected, though I don't have an SQL-level test case for that. It calls CommandCounterIncrement() after moving to TRANS_COMMIT. That CCI had better find no invalidations of open relations, or we'll make syscache lookups. (In simple tests, any necessary invalidations tend to happen at the CCI in CommitTransactionCommand(), so the later CCI does in fact find nothing to do. I have little confidence that should be counted upon, though.) How might we best rearrange things to avoid these hazards? Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com
On 2013-08-05 13:09:31 -0400, Noah Misch wrote: > On Fri, Jul 12, 2013 at 11:42:23AM +0200, Andres Freund wrote: > > On 2013-07-11 15:09:45 -0400, Tom Lane wrote: > > > It never has been, and never will be, allowed to call the catcache code > > > without being in a transaction. What do you think will happen if the > > > requested row isn't in cache? A table access, that's what, and that > > > absolutely requires being in a transaction. > > > I'd like to add an Assert like in the attached patch making sure we're > > in a transaction. Otherwise it's far too easy not to hit an error during > > development because everything is cached - and syscache usage isn't > > always obvious from the outside to the naive or the tired. > > The following test case reliably hits this new assertion: > > create table t (c int); > begin; > create index on t (c); > savepoint q; > insert into t values (1); > select 1/0; > > Stack trace: > > #2 0x0000000000761159 in ExceptionalCondition (conditionName=<value optimized out>, errorType=<value optimized out>, fileName=<valueoptimized out>, lineNumber=<value optimized out>) at assert.c:54 > #3 0x000000000074fc53 in SearchCatCache (cache=0xc53048, v1=139479, v2=0, v3=0, v4=0) at catcache.c:1072 > #4 0x000000000075c011 in SearchSysCache (cacheId=0, key1=369539, key2=6, key3=18446744073709551615, key4=0) at syscache.c:909 > #5 0x0000000000757e57 in RelationReloadIndexInfo (relation=0x7f2dd5fffea0) at relcache.c:1770 > #6 0x000000000075817b in RelationClearRelation (relation=0x7f2dd5fffea0, rebuild=1 '\001') at relcache.c:1926 > #7 0x00000000007588c6 in RelationFlushRelation (relationId=139479) at relcache.c:2076 > #8 RelationCacheInvalidateEntry (relationId=139479) at relcache.c:2138 > #9 0x000000000075185d in LocalExecuteInvalidationMessage (msg=0xcde778) at inval.c:546 > #10 0x0000000000751185 in ProcessInvalidationMessages (hdr=0xc0d390, func=0x7517d0 <LocalExecuteInvalidationMessage>) atinval.c:422 > #11 0x0000000000751a3f in AtEOSubXact_Inval (isCommit=0 '\000') at inval.c:973 > #12 0x00000000004cb986 in AbortSubTransaction () at xact.c:4250 > #13 0x00000000004cbf05 in AbortCurrentTransaction () at xact.c:2863 > #14 0x00000000006968f6 in PostgresMain (argc=1, argv=0x7fff7cf71610, dbname=0xc13b60 "test", username=0xbedc88 "nm") atpostgres.c:3848 > #15 0x000000000064c2b5 in BackendRun () at postmaster.c:4044 > #16 BackendStartup () at postmaster.c:3733 > #17 ServerLoop () at postmaster.c:1571 > #18 0x000000000064fa8d in PostmasterMain (argc=1, argv=0xbe84a0) at postmaster.c:1227 > #19 0x00000000005e2dcd in main (argc=1, argv=0xbe84a0) at main.c:196 > > When we call AtEOSubXact_Inval() or AtEOXact_Inval() with a relation still > open, we can potentially get a relcache rebuild and therefore a syscache > lookup as shown above. CommitSubTransaction() is also potentially affected, > though I don't have an SQL-level test case for that. It calls > CommandCounterIncrement() after moving to TRANS_COMMIT. That CCI had better > find no invalidations of open relations, or we'll make syscache lookups. (In > simple tests, any necessary invalidations tend to happen at the CCI in > CommitTransactionCommand(), so the later CCI does in fact find nothing to do. > I have little confidence that should be counted upon, though.) > > How might we best rearrange things to avoid these hazards? Man. This is a nasty one. And seems to have been there for a long time. I am rather surprised that we haven't seen this as a cause of problems. Deferring invalidation processing a good bit seems to be the only way to deal with this I can see but that seems to require an uncomfortable amount of shuffling around in xact.c. I'll think about it more, but so far I haven't seen anything close to nice. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2013-08-05 13:09:31 -0400, Noah Misch wrote: > On Fri, Jul 12, 2013 at 11:42:23AM +0200, Andres Freund wrote: > > On 2013-07-11 15:09:45 -0400, Tom Lane wrote: > > > It never has been, and never will be, allowed to call the catcache code > > > without being in a transaction. What do you think will happen if the > > > requested row isn't in cache? A table access, that's what, and that > > > absolutely requires being in a transaction. > > > I'd like to add an Assert like in the attached patch making sure we're > > in a transaction. Otherwise it's far too easy not to hit an error during > > development because everything is cached - and syscache usage isn't > > always obvious from the outside to the naive or the tired. > > The following test case reliably hits this new assertion: > > create table t (c int); > begin; > create index on t (c); > savepoint q; > insert into t values (1); > select 1/0; > > Stack trace: > > #2 0x0000000000761159 in ExceptionalCondition (conditionName=<value optimized out>, errorType=<value optimized out>, fileName=<valueoptimized out>, lineNumber=<value optimized out>) at assert.c:54 > #3 0x000000000074fc53 in SearchCatCache (cache=0xc53048, v1=139479, v2=0, v3=0, v4=0) at catcache.c:1072 > #4 0x000000000075c011 in SearchSysCache (cacheId=0, key1=369539, key2=6, key3=18446744073709551615, key4=0) at syscache.c:909 > #5 0x0000000000757e57 in RelationReloadIndexInfo (relation=0x7f2dd5fffea0) at relcache.c:1770 > #6 0x000000000075817b in RelationClearRelation (relation=0x7f2dd5fffea0, rebuild=1 '\001') at relcache.c:1926 > #7 0x00000000007588c6 in RelationFlushRelation (relationId=139479) at relcache.c:2076 > #8 RelationCacheInvalidateEntry (relationId=139479) at relcache.c:2138 > #9 0x000000000075185d in LocalExecuteInvalidationMessage (msg=0xcde778) at inval.c:546 > #10 0x0000000000751185 in ProcessInvalidationMessages (hdr=0xc0d390, func=0x7517d0 <LocalExecuteInvalidationMessage>) atinval.c:422 > #11 0x0000000000751a3f in AtEOSubXact_Inval (isCommit=0 '\000') at inval.c:973 > #12 0x00000000004cb986 in AbortSubTransaction () at xact.c:4250 > #13 0x00000000004cbf05 in AbortCurrentTransaction () at xact.c:2863 > #14 0x00000000006968f6 in PostgresMain (argc=1, argv=0x7fff7cf71610, dbname=0xc13b60 "test", username=0xbedc88 "nm") atpostgres.c:3848 > #15 0x000000000064c2b5 in BackendRun () at postmaster.c:4044 > #16 BackendStartup () at postmaster.c:3733 > #17 ServerLoop () at postmaster.c:1571 > #18 0x000000000064fa8d in PostmasterMain (argc=1, argv=0xbe84a0) at postmaster.c:1227 > #19 0x00000000005e2dcd in main (argc=1, argv=0xbe84a0) at main.c:196 > > When we call AtEOSubXact_Inval() or AtEOXact_Inval() with a relation still > open, we can potentially get a relcache rebuild and therefore a syscache > lookup as shown above. CommitSubTransaction() is also potentially affected, > though I don't have an SQL-level test case for that. It calls > CommandCounterIncrement() after moving to TRANS_COMMIT. That CCI had better > find no invalidations of open relations, or we'll make syscache lookups. (In > simple tests, any necessary invalidations tend to happen at the CCI in > CommitTransactionCommand(), so the later CCI does in fact find nothing to do. > I have little confidence that should be counted upon, though.) > How might we best rearrange things to avoid these hazards? Ok. After a good bit of code reading, I think this isn't an actual bug but an overzealous Assert(). I think it should be Assert(IsTransactionBlock()) not Assert(IsTransactionState()); The reason for that is that when we do the AtEO(Sub)?Xact_Inval(), we've already done a RecordTransactionAbort(true|false) and CurrentTransactionState->state = TRANS_ABORT. So the visibility routines have enough information to consider rows created by the aborted transaction as invisible. I am not really happy with the RelationReloadIndexInfo()s in RelationClearRelation() when we're in an aborted state, especially as the comment surrounding them are clearly out of date, but I don't see a bug there anymore. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Aug 6, 2013 at 3:06 AM, Andres Freund <andres@2ndquadrant.com> wrote: > The reason for that is that when we do the AtEO(Sub)?Xact_Inval(), we've > already done a RecordTransactionAbort(true|false) and > CurrentTransactionState->state = TRANS_ABORT. So the visibility routines > have enough information to consider rows created by the aborted > transaction as invisible. > > I am not really happy with the RelationReloadIndexInfo()s in > RelationClearRelation() when we're in an aborted state, especially as > the comment surrounding them are clearly out of date, but I don't see a > bug there anymore. How can it be safe to try to read catalogs if the transaction is aborted? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-08-08 09:27:24 -0400, Robert Haas wrote: > On Tue, Aug 6, 2013 at 3:06 AM, Andres Freund <andres@2ndquadrant.com> wrote: > > The reason for that is that when we do the AtEO(Sub)?Xact_Inval(), we've > > already done a RecordTransactionAbort(true|false) and > > CurrentTransactionState->state = TRANS_ABORT. So the visibility routines > > have enough information to consider rows created by the aborted > > transaction as invisible. > > > > I am not really happy with the RelationReloadIndexInfo()s in > > RelationClearRelation() when we're in an aborted state, especially as > > the comment surrounding them are clearly out of date, but I don't see a > > bug there anymore. > > How can it be safe to try to read catalogs if the transaction is aborted? Well. It isn't. At least not in general. The specific case triggered here though are cache invalidations being processed which can lead to the catalog being read (pretty crummy, but not easy to get rid of). That's actually safe since before we process the invalidations we have done: 1) CurrentTransactionState->state = TRANS_ABORT 2) RecordTransactionAbort(), marking the transaction as aborted in the clog 3) marked subxacts as aborted 3) ProcArrayEndTransaction() (for toplevel ones) Due to these any tqual stuff will treat the current (sub-)xact and it's children as aborted. So the catalog lookups will use the catalog in a sensible state. Now, one could argue that it's certainly not safe for anything but xact.c itself to play such games. And would be pretty damn right. We could add some flat to signal catcache.c to temporarily use Assert(IsTransactionBlock()) instead of IsTransactionStmt() but that seems overly complex. I think the danger of code doing stuff in an aborted transaction isn't that big. This certainly deserves a good comment... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Aug 8, 2013 at 1:28 PM, Andres Freund <andres@2ndquadrant.com> wrote: > Well. It isn't. At least not in general. The specific case triggered > here though are cache invalidations being processed which can lead to > the catalog being read (pretty crummy, but not easy to get rid > of). That's actually safe since before we process the invalidations we > have done: > 1) CurrentTransactionState->state = TRANS_ABORT > 2) RecordTransactionAbort(), marking the transaction as aborted in the > clog > 3) marked subxacts as aborted > 3) ProcArrayEndTransaction() (for toplevel ones) > > Due to these any tqual stuff will treat the current (sub-)xact and it's > children as aborted. So the catalog lookups will use the catalog in a > sensible state. > > Now, one could argue that it's certainly not safe for anything but > xact.c itself to play such games. And would be pretty damn right. We > could add some flat to signal catcache.c to temporarily use > Assert(IsTransactionBlock()) instead of IsTransactionStmt() but that > seems overly complex. I think the danger of code doing stuff in an > aborted transaction isn't that big. > > This certainly deserves a good comment... Do you want to propose something? I basically don't have a very good feeling about this. Processing invalidations should invalidate stuff, not try to reread it. You may be right that our MVCC snapshot is OK at the point invalidations are processed, but we don't know what caused the transaction to abort in the first place. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-08-09 09:05:51 -0400, Robert Haas wrote: > On Thu, Aug 8, 2013 at 1:28 PM, Andres Freund <andres@2ndquadrant.com> wrote: > > Well. It isn't. At least not in general. The specific case triggered > > here though are cache invalidations being processed which can lead to > > the catalog being read (pretty crummy, but not easy to get rid > > of). That's actually safe since before we process the invalidations we > > have done: > > 1) CurrentTransactionState->state = TRANS_ABORT > > 2) RecordTransactionAbort(), marking the transaction as aborted in the > > clog > > 3) marked subxacts as aborted > > 3) ProcArrayEndTransaction() (for toplevel ones) > > > > Due to these any tqual stuff will treat the current (sub-)xact and it's > > children as aborted. So the catalog lookups will use the catalog in a > > sensible state. > > > > Now, one could argue that it's certainly not safe for anything but > > xact.c itself to play such games. And would be pretty damn right. We > > could add some flat to signal catcache.c to temporarily use > > Assert(IsTransactionBlock()) instead of IsTransactionStmt() but that > > seems overly complex. I think the danger of code doing stuff in an > > aborted transaction isn't that big. > > > > This certainly deserves a good comment... > > Do you want to propose something? I can, but it will have to wait a couple of days. I am only still online because my holiday plans didn't 100% work out and got delayed by a day... > I basically don't have a very good feeling about this. Processing > invalidations should invalidate stuff, not try to reread it. I agree. But fixing that seems to require a good amount of surgery. The problem is that currently we cannot know for sure some index doesn't still use the index support infrastructure :(. > You may > be right that our MVCC snapshot is OK at the point invalidations are > processed, but we don't know what caused the transaction to abort in > the first place. Well, we know it has been an ERROR and nothing worse. And that it successfully longjmp'ed up the way to postgres.c or one of the PLs. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Aug 06, 2013 at 09:06:59AM +0200, Andres Freund wrote: > On 2013-08-05 13:09:31 -0400, Noah Misch wrote: > > When we call AtEOSubXact_Inval() or AtEOXact_Inval() with a relation still > > open, we can potentially get a relcache rebuild and therefore a syscache > > lookup as shown above. CommitSubTransaction() is also potentially affected, > > though I don't have an SQL-level test case for that. It calls > > CommandCounterIncrement() after moving to TRANS_COMMIT. That CCI had better > > find no invalidations of open relations, or we'll make syscache lookups. (In > > simple tests, any necessary invalidations tend to happen at the CCI in > > CommitTransactionCommand(), so the later CCI does in fact find nothing to do. > > I have little confidence that should be counted upon, though.) > > > How might we best rearrange things to avoid these hazards? > > Ok. After a good bit of code reading, I think this isn't an actual bug > but an overzealous Assert(). I think it should be > Assert(IsTransactionBlock()) not Assert(IsTransactionState()); IsTransactionBlock() is for higher-level things that care about actual use of BEGIN. It's false in the middle of executing a single-statement transaction, but that's of course a perfectly valid time for syscache lookups. > The reason for that is that when we do the AtEO(Sub)?Xact_Inval(), we've > already done a RecordTransactionAbort(true|false) and > CurrentTransactionState->state = TRANS_ABORT. So the visibility routines > have enough information to consider rows created by the aborted > transaction as invisible. > > I am not really happy with the RelationReloadIndexInfo()s in > RelationClearRelation() when we're in an aborted state, especially as > the comment surrounding them are clearly out of date, but I don't see a > bug there anymore. Interesting. -- Noah Misch EnterpriseDB http://www.enterprisedb.com
Andres Freund <andres@2ndquadrant.com> writes:
> On 2013-08-08 09:27:24 -0400, Robert Haas wrote:
>> How can it be safe to try to read catalogs if the transaction is aborted?
> Well. It isn't. At least not in general. The specific case triggered
> here though are cache invalidations being processed which can lead to
> the catalog being read (pretty crummy, but not easy to get rid
> of). That's actually safe since before we process the invalidations we
> have done:
> 1) CurrentTransactionState->state = TRANS_ABORT
> 2) RecordTransactionAbort(), marking the transaction as aborted in the
> clog
> 3) marked subxacts as aborted
> 3) ProcArrayEndTransaction() (for toplevel ones)
> Due to these any tqual stuff will treat the current (sub-)xact and it's
> children as aborted. So the catalog lookups will use the catalog in a
> sensible state.
I don't have any faith in this argument. You might be right that we'll
correctly see our own output rows as aborted, but that's barely the tip
of the iceberg of risk here. Is it safe to take new locks in an aborted
transaction? (What if we're already past the lock-release point in
the abort sequence?) For that matter, given that we don't know what
exactly caused the transaction abort, how safe is it to do anything at
all --- we might for instance be nearly out of memory. If the catalog
reading attempt itself fails, won't we be in an infinite loop of
transaction aborts? I could probably think of ten more risks if
I spent a few more minutes at it.
Cache invalidation during abort should *not* lead to any attempt to
immediately revalidate the cache. No amount of excuses will make that
okay. I have not looked to see just what the path of control is in this
particular case, but we need to fix it, not paper over it.
regards, tom lane
On Fri, Aug 09, 2013 at 02:11:46PM -0400, Tom Lane wrote: > Cache invalidation during abort should *not* lead to any attempt to > immediately revalidate the cache. No amount of excuses will make that > okay. I have not looked to see just what the path of control is in this > particular case, but we need to fix it, not paper over it. +1. What if (sub)transaction end only manipulated the local invalidation message queue for later processing? Actual processing would happen after CleanupSubTransaction() returns control to the owning xact, or at the start of the next transaction for a top-level ending. -- Noah Misch EnterpriseDB http://www.enterprisedb.com
On 2013-08-09 14:11:46 -0400, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > On 2013-08-08 09:27:24 -0400, Robert Haas wrote: > >> How can it be safe to try to read catalogs if the transaction is aborted? > > > Well. It isn't. At least not in general. The specific case triggered > > here though are cache invalidations being processed which can lead to > > the catalog being read (pretty crummy, but not easy to get rid > > of). That's actually safe since before we process the invalidations we > > have done: > > 1) CurrentTransactionState->state = TRANS_ABORT > > 2) RecordTransactionAbort(), marking the transaction as aborted in the > > clog > > 3) marked subxacts as aborted > > 3) ProcArrayEndTransaction() (for toplevel ones) > > > Due to these any tqual stuff will treat the current (sub-)xact and it's > > children as aborted. So the catalog lookups will use the catalog in a > > sensible state. > > I don't have any faith in this argument. You might be right that we'll > correctly see our own output rows as aborted, but that's barely the tip > of the iceberg of risk here. Is it safe to take new locks in an aborted > transaction? (What if we're already past the lock-release point in > the abort sequence?) Don't get me wrong. I find the idea of doing catalog lookup during abort horrid. But it's been that way for at least 10 years (I checked 7.4), so it has at least some resemblance of working. Today we do a good bit less than back then, for one we don't do a full cache reload during abort anymore, just for the index support infrastructure. Also, you've reduced the amount of lookups a bit with the relmapper introduction. > For that matter, given that we don't know what > exactly caused the transaction abort, how safe is it to do anything at > all --- we might for instance be nearly out of memory. If the catalog > reading attempt itself fails, won't we be in an infinite loop of > transaction aborts? Looks like that's possible, yes. There seem to be quite some other opportunities for this to happen if you look at the amount of work done in AbortSubTransaction(). I guess it rarely happens because we previously release some memory... > I could probably think of ten more risks if I spent a few more minutes > at it. No need to convince me here. I neither could believe we were doing this, nor figure out why it even "works" for the first hour of looking at it. > Cache invalidation during abort should *not* lead to any attempt to > immediately revalidate the cache. No amount of excuses will make that > okay. I have not looked to see just what the path of control is in this > particular case, but we need to fix it, not paper over it. I agree, although that's easier said than done in the case of subtransactions. The problem we have there is that it's perfectly valid to still have references to a relation from the outer, not aborted, transaction. Those need to be valid for anybody looking at the relcache entry after we've processed the ROLLBACK TO/... I guess the fix is something like we do in the commit case, where we transfer invalidations to the parent transaction. If we then process local invalidations *after* we've cleaned up the subtransaction completely we should be fine. We already do an implicity CommandCounterIncrement() in CommitSubTransaction()... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Aug 9, 2013 at 11:17 PM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-08-09 14:11:46 -0400, Tom Lane wrote: >> Andres Freund <andres@2ndquadrant.com> writes: >> > On 2013-08-08 09:27:24 -0400, Robert Haas wrote: >> >> How can it be safe to try to read catalogs if the transaction is aborted? >> >> > Well. It isn't. At least not in general. The specific case triggered >> > here though are cache invalidations being processed which can lead to >> > the catalog being read (pretty crummy, but not easy to get rid >> > of). That's actually safe since before we process the invalidations we >> > have done: >> > 1) CurrentTransactionState->state = TRANS_ABORT >> > 2) RecordTransactionAbort(), marking the transaction as aborted in the >> > clog >> > 3) marked subxacts as aborted >> > 3) ProcArrayEndTransaction() (for toplevel ones) >> >> > Due to these any tqual stuff will treat the current (sub-)xact and it's >> > children as aborted. So the catalog lookups will use the catalog in a >> > sensible state. >> >> I don't have any faith in this argument. You might be right that we'll >> correctly see our own output rows as aborted, but that's barely the tip >> of the iceberg of risk here. Is it safe to take new locks in an aborted >> transaction? (What if we're already past the lock-release point in >> the abort sequence?) > > Don't get me wrong. I find the idea of doing catalog lookup during abort > horrid. But it's been that way for at least 10 years (I checked 7.4), so > it has at least some resemblance of working. > Today we do a good bit less than back then, for one we don't do a full > cache reload during abort anymore, just for the index support > infrastructure. Also, you've reduced the amount of lookups a bit with the > relmapper introduction. > >> For that matter, given that we don't know what >> exactly caused the transaction abort, how safe is it to do anything at >> all --- we might for instance be nearly out of memory. If the catalog >> reading attempt itself fails, won't we be in an infinite loop of >> transaction aborts? > > Looks like that's possible, yes. There seem to be quite some other > opportunities for this to happen if you look at the amount of work done > in AbortSubTransaction(). I guess it rarely happens because we > previously release some memory... > >> I could probably think of ten more risks if I spent a few more minutes >> at it. > > No need to convince me here. I neither could believe we were doing this, > nor figure out why it even "works" for the first hour of looking at it. > >> Cache invalidation during abort should *not* lead to any attempt to >> immediately revalidate the cache. No amount of excuses will make that >> okay. I have not looked to see just what the path of control is in this >> particular case, but we need to fix it, not paper over it. > > I agree, although that's easier said than done in the case of > subtransactions. The problem we have there is that it's perfectly valid > to still have references to a relation from the outer, not aborted, > transaction. Those need to be valid for anybody looking at the relcache > entry after we've processed the ROLLBACK TO/... > > I guess the fix is something like we do in the commit case, where we > transfer invalidations to the parent transaction. If we then process > local invalidations *after* we've cleaned up the subtransaction > completely we should be fine. We already do an implicity > CommandCounterIncrement() in CommitSubTransaction()... Andres, are you (or is anyone) going to try to fix this assertion failure? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-10-04 15:15:36 -0400, Robert Haas wrote: > Andres, are you (or is anyone) going to try to fix this assertion failure? I think short term replacing it by IsTransactionOrTransactionBlock() is the way to go. Entirely restructuring how cache invalidation in the abort path works is not a small task. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Oct 4, 2013 at 3:20 PM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-10-04 15:15:36 -0400, Robert Haas wrote: >> Andres, are you (or is anyone) going to try to fix this assertion failure? > > I think short term replacing it by IsTransactionOrTransactionBlock() is > the way to go. Entirely restructuring how cache invalidation in the > abort path works is not a small task. Well, if we're going to go that route, how about something like the attached? I included the assert-change per se, an explanatory comment, and the test case that Noah devised to cause the current assertion to fail. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On 2013-10-07 15:02:36 -0400, Robert Haas wrote: > On Fri, Oct 4, 2013 at 3:20 PM, Andres Freund <andres@2ndquadrant.com> wrote: > > On 2013-10-04 15:15:36 -0400, Robert Haas wrote: > >> Andres, are you (or is anyone) going to try to fix this assertion failure? > > > > I think short term replacing it by IsTransactionOrTransactionBlock() is > > the way to go. Entirely restructuring how cache invalidation in the > > abort path works is not a small task. > > Well, if we're going to go that route, how about something like the > attached? I included the assert-change per se, an explanatory > comment, and the test case that Noah devised to cause the current > assertion to fail. Sounds good to me. Maybe add a comment to the added regression test explaining it tests invalidation processing at xact abort? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Oct 7, 2013 at 03:02:36PM -0400, Robert Haas wrote: > On Fri, Oct 4, 2013 at 3:20 PM, Andres Freund <andres@2ndquadrant.com> wrote: > > On 2013-10-04 15:15:36 -0400, Robert Haas wrote: > >> Andres, are you (or is anyone) going to try to fix this assertion failure? > > > > I think short term replacing it by IsTransactionOrTransactionBlock() is > > the way to go. Entirely restructuring how cache invalidation in the > > abort path works is not a small task. > > Well, if we're going to go that route, how about something like the > attached? I included the assert-change per se, an explanatory > comment, and the test case that Noah devised to cause the current > assertion to fail. Is there any plan to commit this? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 2014-01-31 16:41:33 -0500, Bruce Momjian wrote: > On Mon, Oct 7, 2013 at 03:02:36PM -0400, Robert Haas wrote: > > On Fri, Oct 4, 2013 at 3:20 PM, Andres Freund <andres@2ndquadrant.com> wrote: > > > On 2013-10-04 15:15:36 -0400, Robert Haas wrote: > > >> Andres, are you (or is anyone) going to try to fix this assertion failure? > > > > > > I think short term replacing it by IsTransactionOrTransactionBlock() is > > > the way to go. Entirely restructuring how cache invalidation in the > > > abort path works is not a small task. > > > > Well, if we're going to go that route, how about something like the > > attached? I included the assert-change per se, an explanatory > > comment, and the test case that Noah devised to cause the current > > assertion to fail. > > Is there any plan to commit this? IMO it has to be applied. Tom objected on the grounds that cache invalidation has to be fixed properly but that's a major restructuring of code that worked this way for a long time. So changing the Assert() to reflect that seems fair to me. I'd adapt the tests with a sentence explaining what they test, on a first look they are pretty obscure... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-01-31 16:41:33 -0500, Bruce Momjian wrote:
>> Is there any plan to commit this?
> IMO it has to be applied. Tom objected on the grounds that cache
> invalidation has to be fixed properly but that's a major restructuring
> of code that worked this way for a long time. So changing the Assert()
> to reflect that seems fair to me.
The replacement Assert is wrong no? At least that's what was said
upthread. More to the point, changing the Assert so it doesn't fire
doesn't do one damn thing to ameliorate the fact that cache reload
during transaction abort is wrong and unsafe.
We need to fix the real problem not paper over it. The fact that the
fix may be hard doesn't change that.
regards, tom lane
On February 2, 2014 5:52:22 PM CET, Tom Lane <tgl@sss.pgh.pa.us> wrote: >Andres Freund <andres@2ndquadrant.com> writes: >> On 2014-01-31 16:41:33 -0500, Bruce Momjian wrote: >>> Is there any plan to commit this? > >> IMO it has to be applied. Tom objected on the grounds that cache >> invalidation has to be fixed properly but that's a major >restructuring >> of code that worked this way for a long time. So changing the >Assert() >> to reflect that seems fair to me. > >The replacement Assert is wrong no? At least that's what was said >upthread. Well, no. Noah's version isn't as strict as desirable, but it doesn't trigger in wrong cases. Thus still better than what'sin 9.3 (nothing). > More to the point, changing the Assert so it doesn't fire >doesn't do one damn thing to ameliorate the fact that cache reload >during transaction abort is wrong and unsafe. And, as upthread, I still don't think that's correct. I don't have sources available right now, but IIRC we already haveaborted out of the transaction. Released locks, the xid and everything. We just haven't changed the state yet - and affairwe can't naively do so, otherwise we'd do incomplete cleanups if we got interrupted somehow. So yes, it's not pretty and it's really hard to verify. But that doesn't make it entirely wrong. I don't see the point of an assert that triggers for code (and coders) that can't do anything about it because their codeis correct. All the while the assertion actually triggers for ugly but correct code. Andres -- Please excuse brevity and formatting - I am writing this on my mobile phone. Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> On February 2, 2014 5:52:22 PM CET, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> More to the point, changing the Assert so it doesn't fire
>> doesn't do one damn thing to ameliorate the fact that cache reload
>> during transaction abort is wrong and unsafe.
> And, as upthread, I still don't think that's correct. I don't have
> sources available right now, but IIRC we already have aborted out of the
> transaction. Released locks, the xid and everything.
Nope ... the case exhibited in the example is dying in AtEOSubXact_Inval,
which is in the very midst of subxact abort.
I've been thinking about this for the past little while, and I believe
that it's probably okay to have RelationClearRelation leave the relcache
entry un-rebuilt, but with rd_isvalid = false so it will be rebuilt when
next opened. The rationale is explained in the comments in the attached
patch. I've checked that this fixes Noah's test case and still passes
the existing regression tests.
regards, tom lane
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 2a46cfc..791ddc6 100644
*** a/src/backend/utils/cache/relcache.c
--- b/src/backend/utils/cache/relcache.c
*************** RelationClearRelation(Relation relation,
*** 1890,1900 ****
* in case it is a mapped relation whose mapping changed.
*
* If it's a nailed index, then we need to re-read the pg_class row to see
! * if its relfilenode changed. We can't necessarily do that here, because
! * we might be in a failed transaction. We assume it's okay to do it if
! * there are open references to the relcache entry (cf notes for
! * AtEOXact_RelationCache). Otherwise just mark the entry as possibly
! * invalid, and it'll be fixed when next opened.
*/
if (relation->rd_isnailed)
{
--- 1890,1898 ----
* in case it is a mapped relation whose mapping changed.
*
* If it's a nailed index, then we need to re-read the pg_class row to see
! * if its relfilenode changed. We do that immediately if we're inside a
! * valid transaction. Otherwise just mark the entry as possibly invalid,
! * and it'll be fixed when next opened.
*/
if (relation->rd_isnailed)
{
*************** RelationClearRelation(Relation relation,
*** 1903,1909 ****
if (relation->rd_rel->relkind == RELKIND_INDEX)
{
relation->rd_isvalid = false; /* needs to be revalidated */
! if (relation->rd_refcnt > 1)
RelationReloadIndexInfo(relation);
}
return;
--- 1901,1907 ----
if (relation->rd_rel->relkind == RELKIND_INDEX)
{
relation->rd_isvalid = false; /* needs to be revalidated */
! if (IsTransactionState())
RelationReloadIndexInfo(relation);
}
return;
*************** RelationClearRelation(Relation relation,
*** 1921,1927 ****
relation->rd_indexcxt != NULL)
{
relation->rd_isvalid = false; /* needs to be revalidated */
! RelationReloadIndexInfo(relation);
return;
}
--- 1919,1926 ----
relation->rd_indexcxt != NULL)
{
relation->rd_isvalid = false; /* needs to be revalidated */
! if (IsTransactionState())
! RelationReloadIndexInfo(relation);
return;
}
*************** RelationClearRelation(Relation relation,
*** 1942,1947 ****
--- 1941,1969 ----
/* And release storage */
RelationDestroyRelation(relation);
}
+ else if (!IsTransactionState())
+ {
+ /*
+ * If we're not inside a valid transaction, we can't do any catalog
+ * access so it's not possible to rebuild yet. Just exit, leaving
+ * rd_isvalid = false so that the rebuild will occur when the entry is
+ * next opened.
+ *
+ * Note: it's possible that we come here during subtransaction abort,
+ * and the reason for wanting to rebuild is that the rel is open in
+ * the outer transaction. In that case it might seem unsafe to not
+ * rebuild immediately, since whatever code has the rel already open
+ * will keep on using the relcache entry as-is. However, in such a
+ * case the outer transaction should be holding a lock that's
+ * sufficient to prevent any significant change in the rel's schema,
+ * so the existing entry contents should be good enough for its
+ * purposes; at worst we might be behind on statistics updates or the
+ * like. (See also CheckTableNotInUse() and its callers.) These
+ * same remarks also apply to the cases above where we exit without
+ * having done RelationReloadIndexInfo() yet.
+ */
+ return;
+ }
else
{
/*
*************** RelationClearRelation(Relation relation,
*** 2054,2059 ****
--- 2076,2082 ----
* RelationFlushRelation
*
* Rebuild the relation if it is open (refcount > 0), else blow it away.
+ * This is used when we receive a cache invalidation event for the rel.
*/
static void
RelationFlushRelation(Relation relation)
On 2014-02-02 15:16:45 -0500, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > On February 2, 2014 5:52:22 PM CET, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> More to the point, changing the Assert so it doesn't fire > >> doesn't do one damn thing to ameliorate the fact that cache reload > >> during transaction abort is wrong and unsafe. > > > And, as upthread, I still don't think that's correct. I don't have > > sources available right now, but IIRC we already have aborted out of the > > transaction. Released locks, the xid and everything. > > Nope ... the case exhibited in the example is dying in AtEOSubXact_Inval, > which is in the very midst of subxact abort. True. But we've done LWLockReleaseAll(), TransactionIdAbortTree(), XidCacheRemoveRunningXids() and ResourceOwnerRelease(RESOURCE_RELEASE_BEFORE_LOCKS), which is why we are currently able to build correct entries, even though we are in an aborted transaction. > I've been thinking about this for the past little while, and I believe > that it's probably okay to have RelationClearRelation leave the relcache > entry un-rebuilt, but with rd_isvalid = false so it will be rebuilt when > next opened. The rationale is explained in the comments in the attached > patch. I've checked that this fixes Noah's test case and still passes > the existing regression tests. Hm, a bit scary, but I don't see an immediate problem. The following comment now is violated for nailed relations* We assume that at the time we are called, we have at leastAccessShareLock* on the target index. (Note: in the calls from RelationClearRelation,* this is legitimate becausewe know the rel has positive refcount.) but that should be easy to fix. I wonder though, if we couldn't just stop doing the RelationReloadIndexInfo() for nailed indexes. The corresponding comment says: * If it's a nailed index, then we need to re-read the pg_class row to see * if its relfilenode changed. We do thatimmediately if we're inside a * valid transaction. Otherwise just mark the entry as possibly invalid, * and it'll befixed when next opened. */ but any relfilenode change should have already been handled by RelationInitPhysicalAddr()? Do you plan to backpatch this? If so, even to 8.4? Greetings, Andres Freund --Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Noah Misch <noah@leadboat.com> writes:
> The following test case reliably hits this new assertion:
> create table t (c int);
> begin;
> create index on t (c);
> savepoint q;
> insert into t values (1);
> select 1/0;
As of commit ac8bc3b6e4, this test case no longer triggers the assertion,
because it depends on the INSERT issuing a relcache invalidation request
against t's index. btree used to do that when changing the index
metapage, but no longer does. The underlying problem is certainly still
there, though, and can be triggered with this slightly more complex test:
create or replace function inverse(int) returns float8 as
$$
begin analyze t1; return 1::float8/$1;
exception when division_by_zero then return 0;
end$$ language plpgsql volatile;
drop table if exists t1;
create table t1 (c float8 unique);
insert into t1 values (1);
insert into t1 values (inverse(0));
Here, the ANALYZE triggers a relcache inval within the subtransaction
established by the function's BEGIN/EXCEPTION block, and then we abort
that subtransaction with a zero-divide, so you end up at the same place
as with the older example.
Of note is that we still have to have an index on t1; remove that,
no assert. This is a bit surprising since the ANALYZE certainly causes
a relcache flush on the table not just the index. The reason turns out
to be that for a simple table like this, relcache entry rebuild does not
involve consulting any syscache: we load the pg_class row with a systable
scan on pg_class, and build the tuple descriptor using a systable scan on
pg_attribute, and we're done. IIRC this was done this way intentionally
to avoid duplicative caching of the pg_class and pg_attribute rows.
However, RelationReloadIndexInfo uses the syscaches with enthusiasm, so
you will hit the Assert in question if you're trying to rebuild an index;
and you possibly could hit it for a table if you have a more complicated
table definition, such as one with rules.
Of course, a direct system catalog scan is certainly no safer in an
aborted transaction than the one that catcache.c is refusing to do.
Therefore, in my opinion, relcache.c ought also to be doing an
Assert(IsTransactionState()), at least in ScanPgRelation and perhaps
everywhere that it does catalog scans.
I stuck such an Assert in ScanPgRelation, and verified that it doesn't
break any existing regression tests --- although of course the above
test case still fails, and now even without declaring an index.
Barring objections I'll go commit that. It's a bit pointless to be
Asserting that catcache.c does nothing unsafe when relcache.c does
the same things without any such test.
(Alternatively, maybe we should centralize the asserting in
systable_beginscan or some such place?)
regards, tom lane
On Wed, Feb 05, 2014 at 02:07:29PM -0500, Tom Lane wrote: > Of course, a direct system catalog scan is certainly no safer in an > aborted transaction than the one that catcache.c is refusing to do. > Therefore, in my opinion, relcache.c ought also to be doing an > Assert(IsTransactionState()), at least in ScanPgRelation and perhaps > everywhere that it does catalog scans. > > I stuck such an Assert in ScanPgRelation, and verified that it doesn't > break any existing regression tests --- although of course the above > test case still fails, and now even without declaring an index. > > Barring objections I'll go commit that. It's a bit pointless to be > Asserting that catcache.c does nothing unsafe when relcache.c does > the same things without any such test. > > (Alternatively, maybe we should centralize the asserting in > systable_beginscan or some such place?) I'd put it in LockAcquireExtended() and perhaps also heap_beginscan_internal() and/or index_beginscan_internal(). ScanPgRelation() isn't special. -- Noah Misch EnterpriseDB http://www.enterprisedb.com
On 2014-02-05 14:07:29 -0500, Tom Lane wrote: > I stuck such an Assert in ScanPgRelation, and verified that it doesn't > break any existing regression tests --- although of course the above > test case still fails, and now even without declaring an index. > > Barring objections I'll go commit that. It's a bit pointless to be > Asserting that catcache.c does nothing unsafe when relcache.c does > the same things without any such test. > > (Alternatively, maybe we should centralize the asserting in > systable_beginscan or some such place?) I don't have a problem with sticking an additional assert elsewhere, but I think ScanPgRelation, systable_beginscan are a bit late, because they frequently won't be hit during testing because the lookups will be cached... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-02-05 14:07:29 -0500, Tom Lane wrote:
>> I stuck such an Assert in ScanPgRelation, and verified that it doesn't
>> break any existing regression tests --- although of course the above
>> test case still fails, and now even without declaring an index.
>>
>> Barring objections I'll go commit that. It's a bit pointless to be
>> Asserting that catcache.c does nothing unsafe when relcache.c does
>> the same things without any such test.
>>
>> (Alternatively, maybe we should centralize the asserting in
>> systable_beginscan or some such place?)
> I don't have a problem with sticking an additional assert elsewhere, but
> I think ScanPgRelation, systable_beginscan are a bit late, because they
> frequently won't be hit during testing because the lookups will be
> cached...
Oh, good point. By analogy to the placement of the existing Assert in
SearchCatCache, the one for relcache should be in RelationIdGetRelation.
[ experiments... ] OK, that passes regression tests too. Good...
regards, tom lane
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-02-02 15:16:45 -0500, Tom Lane wrote:
>> I've been thinking about this for the past little while, and I believe
>> that it's probably okay to have RelationClearRelation leave the relcache
>> entry un-rebuilt, but with rd_isvalid = false so it will be rebuilt when
>> next opened. The rationale is explained in the comments in the attached
>> patch. I've checked that this fixes Noah's test case and still passes
>> the existing regression tests.
> Hm, a bit scary, but I don't see an immediate problem.
> The following comment now is violated for nailed relations
> * We assume that at the time we are called, we have at least AccessShareLock
> * on the target index. (Note: in the calls from RelationClearRelation,
> * this is legitimate because we know the rel has positive refcount.)
> but that should be easy to fix.
Ah, good point. So we should keep the refcnt test in the nailed-rel case.
> I wonder though, if we couldn't just stop doing the
> RelationReloadIndexInfo() for nailed indexes.
No; you're confusing nailed indexes with mapped indexes. There are nailed
indexes that aren't on mapped catalogs, see the load_critical_index calls
in RelationCacheInitializePhase3.
> The corresponding comment says:
> * If it's a nailed index, then we need to re-read the pg_class row to see
> * if its relfilenode changed.
This comment could use some work I guess; needs to say "nailed but not
mapped index".
> Do you plan to backpatch this? If so, even to 8.4?
I'm of two minds about that. I think this is definitely a bug, but we
do not currently have any evidence that there's an observable problem
in practice. On the other hand, we certainly get reports of
irreproducible issues from time to time, so I don't care to rule out
the theory that some of them might be caused by faulty cache reloads.
That possibility has to be balanced against the risk of introducing
new issues with this patch.
Thoughts?
(BTW, I see that the CLOBBER_CACHE_ALWAYS members of the buildfarm are
unhappy with the IsTransactionState check in RelationIdGetRelation.
Will look at that ... but it seems to be in initdb which breaks a lot
of these rules anyway, so I think it's probably not significant.)
regards, tom lane
On 2014-02-06 16:35:07 -0500, Tom Lane wrote: > > I wonder though, if we couldn't just stop doing the > > RelationReloadIndexInfo() for nailed indexes. > > No; you're confusing nailed indexes with mapped indexes. There are nailed > indexes that aren't on mapped catalogs, see the load_critical_index calls > in RelationCacheInitializePhase3. Oh. I'd somehow remembered nailed catalogs would be a subset of mapped ones. But as you say, they are not. Not sure I like that, but it's certainly set for now. > > Do you plan to backpatch this? If so, even to 8.4? > I'm of two minds about that. I think this is definitely a bug, but we > do not currently have any evidence that there's an observable problem > in practice. On the other hand, we certainly get reports of > irreproducible issues from time to time, so I don't care to rule out > the theory that some of them might be caused by faulty cache reloads. > That possibility has to be balanced against the risk of introducing > new issues with this patch. > > Thoughts? Let's let it stew a while in master, there certainly are enough subtleties around this that I'd hesitate to add it to a point release in the not too far away future. Doesn't seem that urgent to me. But after that I'd say lets backpatch it. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
I wrote:
> (BTW, I see that the CLOBBER_CACHE_ALWAYS members of the buildfarm are
> unhappy with the IsTransactionState check in RelationIdGetRelation.
> Will look at that ... but it seems to be in initdb which breaks a lot
> of these rules anyway, so I think it's probably not significant.)
So what's happening is that in bootstrap mode, we run each BKI command
in a separate transaction. (Since these transactions all use the same
XID, it's not clear what's the point of doing that rather than running
the whole BKI script as one transaction. But it's been like that for
twenty years so I'm disinclined to mess with it.) Bootstrap mode is
peculiar in that it expects relcache entries to stay open across these
"transactions", which we implement by keeping their refcnts positive.
Throwing CLOBBER_CACHE_ALWAYS into the mix means we do a relcache flush
during transaction start, during which RelationClearRelation tries to
rebuild the cache entries right away because they have positive refcnt,
triggering the Assert because it has to access system relations such as
pg_class and we're not in TRANS_INPROGRESS state yet.
So in short, the patch I proposed upthread fixes this, since it postpones
the actual cache entry reload into the main part of the transaction.
So rather than lobotomize that Assert with an IsBootstrapMode exception,
I think I'll just go ahead and commit this patch, with the cleanups
we discussed earlier.
regards, tom lane
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-02-06 16:35:07 -0500, Tom Lane wrote:
>> Thoughts?
> Let's let it stew a while in master, there certainly are enough
> subtleties around this that I'd hesitate to add it to a point release in
> the not too far away future. Doesn't seem that urgent to me. But after
> that I'd say lets backpatch it.
Seems reasonable to me. The urgency would go up if we could positively
trace any field trouble reports to this.
regards, tom lane