Re: [HACKERS] SSI patch version 12

2011-01-18 Thread Kevin Grittner
Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote:
 
 There's a few remaining TODO comments in the code, which obviously
 need to be resolved one way or another
 
Not all of these are must haves for 9.1.  Here's how they break
down:
 
The two in predicate_internals.h mark places which would need to be
touched if we further modify the btree AM to do index-tuple level
predicate locking.  I know that Dan was eager to tackle that because
his group at MIT has predicate locking working at that granularity
for their transaction-aware caching which works with PostgreSQL.  At
this point, though, I'm very skeptical about starting that effort
for 9.1; it seems like something for 9.2.
 
There's one TODO related to whether we can drop the volatile
modifier from MySerializableXact.  I've started reviewing code
around this, but am not yet done.  It wouldn't be terrible to leave
it there and worry about this for 9.2, but it will be even better if
we can clear it up one way or the other now.
 
Three are marking the spots we want to touch if a patch becomes
available which lets us cancel the transaction on one connection
from another.  I'd love to take care of these, but we can't without
a separate patch that I know *I'm* not in a position to write for
9.1.  We may need to leave these for 9.2, as well.
 
One is about the desirability of taking some action (probably
reporting a warning) in the SLRU summarization code if we've burned
through over a billion transaction IDs while one read write
transaction has remained active.  That puts us close to where we'd
need to start canceling attempts to start a new transaction due to
resource issues.  Seems like we should issue that warning for 9.1. 
Not big or dangerous.  I'll do it.
 
One is related to the heuristics for promoting the granularity of
predicate locks.  We picked numbers out of the air which seemed
reasonable at the time.  I'm sure we can make this more
sophisticated, but what we have seems to be working OK.  I'm tempted
to suggest that we leave this alone until 9.2 unless someone has a
suggestion for a particular improvement.
 
One is related to the number of structures to allocate for detailed
conflict checking.  We've never seen a problem with this limit, at
least that has reached me.  On the other hand, it's certainly at
least theoretically possible to hit the limit and cause confusing
errors.  Perhaps we can put this one on a GUC and make sure the
error message is clear with a hint to think about adjusting the GUC?
The GUC would be named something like
max_conflicts_per_serializable_transaction (or something to that
general effect).  We should probably do that or bump up the limit.
If my back-of-the-envelope math is right, a carefully constructed
pessimal load could need up to (max_connections / 2)^2 -- so 100
connections could conceivably require 2500 structures, although such
a scenario would be hard to create.  Current picked from thin air
numbers would have space for 500.  The structure is six times the
pointer size, so 24 bytes each at 32 bit and 48 bytes each at 64
bit.
 
Three TODOs have popped up since Heikki's post because I pushed
Dan's 2PC WIP to the repo -- it's at a point where behavior should
be right for cases where the server keeps running.  He's working on
the persistence aspect now, and there are TODOs in the new stubs
he's filling out.  Definitely 9.1.
 
Then there's one still lurking outside the new predicate* files, in
heapam.c.  It's about 475 lines into the heap_update function (25
lines from the bottom).  In reviewing where we needed to acquire
predicate locks, this struck me as a place where we might need to
duplicate the predicate lock from one tuple to another, but I wasn't
entirely sure.  I tried for a few hours one day to construct a
scenario which would show a serialization anomaly if I didn't do
this, and failed create one.  If someone who understands both the
patch and heapam.c wants to look at this and offer an opinion, I'd
be most grateful.  I'll take another look and see if I can get my
head around it better, but failing that, I'm disinclined to either
add locking I'm not sure we need or to remove the comment that says
we *might* need it there.
 
That's all of them.
 
-Kevin

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SSI patch version 12

2011-01-18 Thread Robert Haas
On Tue, Jan 18, 2011 at 3:18 PM, Kevin Grittner
kevin.gritt...@wicourts.gov wrote:
 That's all of them.

Our existing code has plenty of TODOs in it already, so I see no
problem with continuing to comment places where future enhancements
are possible, as long as they don't reflect deficiencies that are
crippling in the present.

-- 
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


Re: [HACKERS] SSI patch version 12

2011-01-18 Thread Kevin Grittner
Kevin Grittner kevin.gritt...@wicourts.gov wrote:
 
 If my back-of-the-envelope math is right, a carefully constructed
 pessimal load could need up to (max_connections / 2)^2 -- so 100
 connections could conceivably require 2500 structures, although
 such a scenario would be hard to create.  Current picked from
 thin air numbers would have space for 500.
 
Er, actually, we would have space for 5000, because it's five times
the number of SERIALIZABLEXACT structures which is ten times
max_connections. I guess that would explain why I've never seen a
report of a problem.
 
Still, someone who creates a very large number of connections and
pounds them heavily with SERIALIZABLE transactions might conceivably
run into it.  Since that's something the docs explicitly warn you
*not* to do with serializable transactions, I'm not sure we need to
do more than make sure the error message and hint are good. 
Thoughts?
 
-Kevin

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SSI patch version 12

2011-01-17 Thread David Fetter
On Mon, Jan 17, 2011 at 09:58:35AM +0200, Anssi Kääriäinen wrote:
 One thing I have been thinking about is how does predicate locking
 indexes work when using functional indexes and functions marked as
 immutable but which really aren't.  I don't know how predicate
 locking indexes works, so it might be that this is a non-issue.  I
 haven't been able to break anything in this way, and even if I
 could, this is probably something that doesn't need anything else
 than a warning that if you label your index functions immutable when
 they aren't, serializable transactions might not work.

When you tell the system an untruth about the state of the world, such
as alleging immutability when it's not actually there, it will get its
revenge in ways more drastic than this.

Cheers,
David.
-- 
David Fetter da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SSI patch version 12

2011-01-17 Thread Kevin Grittner
Anssi Kääriäinen wrote:
 
 I tried to break the version 11 of the patch (some of the work was
 against earlier versions). In total I have used a full work day
 just trying to break things, but haven't been able to find anything
 after version 8. I can verify that the partial index issue is
 fixed, and the count(*) performance is a lot better now.
 
Thanks for all your testing!
 
For the record, Dan found one more issue with the placement of our LW
locking statements which provided a very small window for one process
to see an uninitialized structure from another.  He found it by
uncommenting the '#define TEST_OLDSERXID' line (thereby forcing any
terminated transaction immediately into the SLRU logic) and letting
DBT-2 pound on it for a long time.  If anyone else is going to be
doing heavy stress testing, you might want to apply this fix:
 
http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=036eefe05ba58da6dff9e1cae766e565182f37be
 
The issue is sufficiently hard to hit that I didn't want to post a
whole new patch for it.  I've been considering it now that doc
changes are complete, but am waiting to see how the 2PC work is
going.
 
 One thing I have been thinking about is how does predicate locking
 indexes work when using functional indexes and functions marked as
 immutable but which really aren't. I don't know how predicate
 locking indexes works, so it might be that this is a non-issue.
 
As David pointed out, doing that is likely to cause you bigger
headaches than this; but yes, you can break serializability by
declaring a non-immutable function as immutable and then using it on
a permanent table.  Like David apparently is, I'm skeptical that this
merits specific mention in the docs; but I'm open to arguments to
the contrary -- particularly if they suggest what language to put
where.
 
-Kevin

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SSI patch version 12

2011-01-17 Thread Heikki Linnakangas

On 15.01.2011 01:54, Kevin Grittner wrote:

/*
 * for r/o transactions: list of concurrent r/w transactions that we 
could
 * potentially have conflicts with, and vice versa for r/w transactions
 */
TransactionId topXid;   /* top level xid for the transaction, 
if one
 * exists; else 
invalid */
TransactionId finishedBefore;   /* invalid means still running; 
else

 * the struct expires when no

 * serializable xids are before this. */
TransactionId xmin; /* the transaction's snapshot 
xmin */
uint32  flags;  /* OR'd combination of values 
defined below */
int pid;/* pid of associated 
process */
} SERIALIZABLEXACT;


What does that comment about list of concurrent r/w transactions refer 
to? I don't see any list there. Does it refer to 
possibleUnsafeConflicts, which is above that comment?


Above SERIALIZABLEXID struct:

 * A hash table of these objects is maintained in shared memory to provide a
 * quick way to find the top level transaction information for a serializable
 * transaction,  Because a serializable transaction can acquire a snapshot
 * and read information which requires a predicate lock before it has a
 * TransactionId, it must be keyed by VirtualTransactionId; this hashmap
 * allows a fast link from MVCC transaction IDs to the related serializable
 * transaction hash table entry.


I believe the comment is trying to say that there's some *other* hash 
that is keyed by VirtualTransactionId, so we need this other one keyed 
by TransactionId. It took me a while to understand that, it should be 
rephrased.


Setting the high bit in OldSetXidAdd() seems a bit weird. How about just 
using UINT64_MAX instead of 0 to mean no conflicts? Or 1, and start the 
sequence counter from 2.


ReleasePredicateLocks() reads ShmemVariableCache-nextXid without 
XidGenLock. Maybe it's safe, we assume that TransactionIds are atomic 
elsewhere, but at least there needs to be a comment explaining it. But 
it probably should use ReadNewTransactionId() instead.


Attached is a patch for some trivial changes, mostly typos.


Overall, this is very neat and well-commented code. All the data 
structures make my head spin, but I don't see anything unnecessary or 
have any suggestions for simplification. There's a few remaining TODO 
comments in the code, which obviously need to be resolved one way or 
another, but as soon as you're finished with any outstanding issues that 
you know of, I think this is ready for commit.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 2024b48..c8078fe 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -29,7 +29,7 @@
/indexterm
 
indexterm
-primarySerializable Snapshot Isloation/primary
+primarySerializable Snapshot Isolation/primary
/indexterm
 
indexterm
@@ -61,9 +61,9 @@
 do not conflict with locks acquired for writing data, and so
 reading never blocks writing and writing never blocks reading.
 productnamePostgreSQL/productname maintains this guarantee
-even when providing the the strictest level of transaction
+even when providing the strictest level of transaction
 isolation through the use of an innovative firsttermSerializable
-Snapshot Isloation/firstterm (acronymSSI/acronym) level.
+Snapshot Isolation/firstterm (acronymSSI/acronym) level.
/para
 
para
@@ -647,7 +647,7 @@ ERROR:  could not serialize access due to read/write dependencies among transact
  /listitem
  listitem
   para
-   Don't leave connections dangling idle in transction longer than
+   Don't leave connections dangling idle in transaction longer than
necessary.
   /para
  /listitem
diff --git a/src/backend/storage/lmgr/README-SSI b/src/backend/storage/lmgr/README-SSI
index 7eb82be..2c4de29 100644
--- a/src/backend/storage/lmgr/README-SSI
+++ b/src/backend/storage/lmgr/README-SSI
@@ -207,7 +207,7 @@ until one of the other transactions in the set of conflicts which
 could generate an anomaly has successfully committed.  This is
 conceptually similar to how write conflicts are handled.  To fully
 implement this guarantee there needs to be a way to roll back the
-active transaciton for another process with a serialization failure
+active transaction for another process with a serialization failure
 SQLSTATE, even if it is idle in transaction.
 
 
@@ -286,7 +286,7 @@ index which would have been included in the scan had they existed at
 the time.  Conceptually, we want to lock the gaps between and
 surrounding index entries 

Re: [HACKERS] SSI patch version 12

2011-01-17 Thread Dan Ports
On Mon, Jan 17, 2011 at 09:58:44PM +0200, Heikki Linnakangas wrote:
 What does that comment about list of concurrent r/w transactions refer 
 to? I don't see any list there. Does it refer to 
 possibleUnsafeConflicts, which is above that comment?

Yes, that comment was supposed to be attached to
possibleUnsafeConflicts. It appears to have wandered down a couple
lines, maybe during some combination of git merges and pgindent runs.

 Above SERIALIZABLEXID struct:
   * A hash table of these objects is maintained in shared memory to provide a
   * quick way to find the top level transaction information for a 
  serializable
   * transaction,  Because a serializable transaction can acquire a snapshot
   * and read information which requires a predicate lock before it has a
   * TransactionId, it must be keyed by VirtualTransactionId; this hashmap
   * allows a fast link from MVCC transaction IDs to the related serializable
   * transaction hash table entry.
 
 I believe the comment is trying to say that there's some *other* hash 
 that is keyed by VirtualTransactionId, so we need this other one keyed 
 by TransactionId. It took me a while to understand that, it should be 
 rephrased.

Actually, I think that other hash no longer exists, it got replaced
with a list because we weren't actually using vxid - sxact lookup. So
the comment appears to be both confusing and inaccurate and should be
removed entirely, other than to note somewhere that not every
SERIALIZABLEXACT will appear in SerializableXidHash because it might
not have a TransactionId. 

The comment above SERIALIZABLEXACT also needs to be updated since it
refers to said hash table. And if I'm not mistaken (Kevin?), we can
eliminate SERIALIZABLEXACTTAG altogether and not bother putting the
vxid in the sxact.

While we're at it, it probably wouldn't hurt to rename
SerializableXactHashLock to PredTranLock or something, since there's no
SerializableXactHash anymore (although the lock is still being used
correctly)

Dan

-- 
Dan R. K. Ports  MIT CSAILhttp://drkp.net/

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SSI patch version 12

2011-01-17 Thread Kevin Grittner
 Dan Ports  wrote:
 
 Yes, that comment was supposed to be attached to
 possibleUnsafeConflicts.
 
 Actually, I think that other hash no longer exists
 
 The comment above SERIALIZABLEXACT also needs to be updated since
 it refers to said hash table. And if I'm not mistaken (Kevin?), we
 can eliminate SERIALIZABLEXACTTAG altogether and not bother putting
 the vxid in the sxact.
 
I think we still need the vxid.  It shows in the pg_locks view, and
we might possibly need it to find the right process to cancel once we
have some way to do that.  But there's no point with having the tag
level anymore.
 
Otherwise, I agree with Dan on all counts.
 
Dan, if you're not already working on these, I'll take them, so you
can focus on 2PC.
 
-Kevin

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SSI patch version 12

2011-01-17 Thread Dan Ports
On Mon, Jan 17, 2011 at 06:52:09PM -0600, Kevin Grittner wrote:
 I think we still need the vxid.  It shows in the pg_locks view, and
 we might possibly need it to find the right process to cancel once we
 have some way to do that.  But there's no point with having the tag
 level anymore.

Oh, right, we do use it to generate pg_lock_status. I forgot about that
particular tendril of code that extends outside of predicate.c.

 Dan, if you're not already working on these, I'll take them, so you
 can focus on 2PC.

All yours! But if you feel like renaming SerializableXactHashLock,
please hold off to prevent conflicts with my 2PC changes.

Dan

-- 
Dan R. K. Ports  MIT CSAILhttp://drkp.net/

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SSI patch version 12

2011-01-17 Thread Kevin Grittner
 Heikki Linnakangas  wrote:
 
 Setting the high bit in OldSetXidAdd() seems a bit weird. How about
 just using UINT64_MAX instead of 0 to mean no conflicts? Or 1, and
 start the sequence counter from 2.
 
Sure.  I think I like reserving 1 and starting at 2 better.  Will do.
 
 ReleasePredicateLocks() reads ShmemVariableCache-nextXid without
 XidGenLock. Maybe it's safe, we assume that TransactionIds are
 atomic elsewhere, but at least there needs to be a comment
 explaining it. But it probably should use ReadNewTransactionId()
 instead.
 
Hmm...  We don't want to *assign* a new xid here -- we just want to
know what the next one *will be*.  I will review locking around this
area, but I think it's sound.  If that checks out OK on review, I'll
add comments.
 
 Attached is a patch for some trivial changes, mostly typos.
 
Wow.  I transpose letters a lot don't I?  Thanks for finding those.
Will fix.
 
 Overall, this is very neat and well-commented code. All the data
 structures make my head spin, but I don't see anything unnecessary
 or have any suggestions for simplification. There's a few remaining
 TODO comments in the code, which obviously need to be resolved one
 way or another, but as soon as you're finished with any outstanding
 issues that you know of, I think this is ready for commit.
 
OK.  I may need to bounce some questions off the list to resolve some
of them.  The biggest, in my mind, is whether MySerializableXact
needs to be declared volatile.  I don't have my head around the
issues on that as well as I might.  Any thoughts on it?
 
Thanks for the multiple reviews and all the suggestions.  The code is
clearly better for your attention.
 
-Kevin

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SSI patch version 12

2011-01-17 Thread Dan Ports
On Mon, Jan 17, 2011 at 07:20:20PM -0600, Kevin Grittner wrote:
 OK.  I may need to bounce some questions off the list to resolve some
 of them.  The biggest, in my mind, is whether MySerializableXact
 needs to be declared volatile.  I don't have my head around the
 issues on that as well as I might.  Any thoughts on it?

I'd been meaning to ask about that too. I couldn't think of any reason
it would need to be volatile. Why do you think it might need to be?

Dan

-- 
Dan R. K. Ports  MIT CSAILhttp://drkp.net/

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SSI patch version 12

2011-01-17 Thread Kevin Grittner
 Dan Ports  wrote:
 On Mon, Jan 17, 2011 at 07:20:20PM -0600, Kevin Grittner wrote:
 OK. I may need to bounce some questions off the list to resolve
 some of them. The biggest, in my mind, is whether
 MySerializableXact needs to be declared volatile. I don't have my
 head around the issues on that as well as I might. Any thoughts on
 it?
 
 I'd been meaning to ask about that too. I couldn't think of any
 reason it would need to be volatile. Why do you think it might need
 to be?
 
I honestly can't remember why I initially put that there -- possibly
because I had a notion that some modifications to the structure might
not need to be so closely synchronized with other processes as to
need have modifications covered by a LW lock, but not wanting them to
linger forever.  It may be completely bogus.
 
The problem is that I don't have a very clear sense of what it really
does, which is not helped much by having done a few years of Java
programming, where the same keyword seems to have a vaguely-similar-
but-not-really-the-same meaning.
 
-Kevin

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SSI patch version 12

2011-01-17 Thread Tom Lane
Kevin Grittner kevin.gritt...@wicourts.gov writes:
 Dan Ports  wrote:
  The biggest, in my mind, is whether
 MySerializableXact needs to be declared volatile.
 
 The problem is that I don't have a very clear sense of what it really
 does, which is not helped much by having done a few years of Java
 programming, where the same keyword seems to have a vaguely-similar-
 but-not-really-the-same meaning.

In C it means that the compiler must not try to optimize away loads or
stores of the variable, because the variable is subject to being read or
changed by outside forces (interrupt service routines, other processes
or threads, etc).

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


Re: [HACKERS] SSI patch version 12

2011-01-16 Thread Anssi Kääriäinen
While I haven't tried this patch, I tried to break the version 11 of the 
patch (some of the work was against earlier versions). In total I have 
used a full work day just trying to break things, but haven't been able 
to find anything after version 8. I can verify that the partial index 
issue is fixed, and the count(*) performance is a lot better now.


One thing I have been thinking about is how does predicate locking 
indexes work when using functional indexes and functions marked as 
immutable but which really aren't. I don't know how predicate locking 
indexes works, so it might be that this is a non-issue. I haven't been 
able to break anything in this way, and even if I could, this is 
probably something that doesn't need anything else than a warning that 
if you label your index functions immutable when they aren't, 
serializable transactions might not work.


On 01/15/2011 01:54 AM, Kevin Grittner wrote:

The index types other than btree don't have fine-grained support,
which I don't think is a fatal defect, but it would be nice to
implement.  I may be able to get GiST working again this weekend in
addition to the documentation work.  The others might not get done
for 9.1 unless someone who knows their way around the guts of those
AMs can give us some advice soon
I wonder if there are people using GiST and GIN indexes and serializable 
transactions. When upgrading to 9.1 and if there is no backwards 
compatibility GUC this could be a problem... The amount of users in this 
category is probably very low anyways, so maybe this is not an issue 
worth worrying about.


 - Anssi

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers