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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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:
Regards,
Jeff Davis
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
assert-transaction-state-in-searchcatcache.patchtext/x-patch; charset=us-asciiDownload
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 7fc205a..7863105 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -21,6 +21,7 @@
#include "access/sysattr.h"
#include "access/tuptoaster.h"
#include "access/valid.h"
+#include "access/xact.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_type.h"
#include "miscadmin.h"
@@ -1069,6 +1070,9 @@ SearchCatCache(CatCache *cache,
SysScanDesc scandesc;
HeapTuple ntp;
+ /* Make sure we're in a xact, even if this ends up being a cache hit */
+ Assert(IsTransactionState());
+
/*
* one-time startup overhead for each cache
*/
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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=<value optimized 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>) at inval.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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=<value optimized 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>) at inval.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:196When 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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=<value optimized 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>) at inval.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:196When 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
weaken-searchcatcache-assert.patchapplication/octet-stream; name=weaken-searchcatcache-assert.patchDownload
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index c467f11..7d63c97 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -1105,8 +1105,16 @@ SearchCatCache(CatCache *cache,
SysScanDesc scandesc;
HeapTuple ntp;
- /* Make sure we're in a xact, even if this ends up being a cache hit */
- Assert(IsTransactionState());
+ /*
+ * Make sure we're in a xact, even if this ends up being a cache hit
+ *
+ * This should really assert IsTransactionState() rather than merely
+ * IsTransactionOrTransactionBlock(), but it's currently possible for this
+ * function to be called from an aborted transaction. That's probably bad,
+ * but fixing that would require extensive reworking of our invalidation
+ * processing.
+ */
+ Assert(IsTransactionOrTransactionBlock());
/*
* one-time startup overhead for each cache
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index 5c84ec5..f0382de 100644
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -599,6 +599,15 @@ fetch from foo;
(1 row)
abort;
+create table pqr (a int);
+begin;
+create index on pqr (a);
+savepoint q;
+insert into pqr values (1);
+select 1/0;
+ERROR: division by zero
+rollback;
+drop table pqr;
-- Test for successful cleanup of an aborted transaction at session exit.
-- THIS MUST BE THE LAST TEST IN THIS FILE.
begin;
diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql
index faf6a9b..ed01b74 100644
--- a/src/test/regress/sql/transactions.sql
+++ b/src/test/regress/sql/transactions.sql
@@ -368,6 +368,15 @@ fetch from foo;
abort;
+create table pqr (a int);
+begin;
+create index on pqr (a);
+savepoint q;
+insert into pqr values (1);
+select 1/0;
+rollback;
+drop table pqr;
+
-- Test for successful cleanup of an aborted transaction at session exit.
-- THIS MUST BE THE LAST TEST IN THIS FILE.
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 majorrestructuring
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's in 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 have aborted out of the transaction. Released locks, the xid and everything. We just haven't changed the state yet - and affair we 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 code is 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
avoid-unsafe-cache-reloads.patchtext/x-diff; charset=us-ascii; name=avoid-unsafe-cache-reloads.patchDownload
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 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.
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 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.
*/
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers