Re: foreign key locks

Alvaro Herrera wrote:

> here's fklocks v14, which also merges to new master as there were
> several other conflicts. It passes make installcheck-world now.

Recent commits broke it again, so here's a rebased v15. (No changes
other than attempting to merge your work with the pg_controldata and
pg_resetxlog changes and wrapping that FIXME in a comment.)

Using that patch, pg_upgrade completes, but pg_dumpall fails:

Upgrade Complete
----------------
Optimizer statistics are not transferred by pg_upgrade so,
once you start the new server, consider running:
analyze_new_cluster.sh

Running this script will delete the old cluster's data files:
delete_old_cluster.sh
+ pg_ctl start -l
/home/kevin/pg/master/contrib/pg_upgrade/log/postmaster2.log -w
waiting for server to start.... done
server started
+ pg_dumpall
pg_dump: Dumping the contents of table "hslot" failed: PQgetResult()
failed.
pg_dump: Error message from server: ERROR: MultiXactId 115 does no
longer exist -- apparent wraparound
pg_dump: The command was: COPY public.hslot (slotname, hubname,
slotno, slotlink) TO stdout;
pg_dumpall: pg_dump failed on database "regression", exiting
+ pg_dumpall2_status=1
+ pg_ctl -m fast stop
waiting for server to shut down.... done
server stopped
+ [ -n 1 ]
+ echo pg_dumpall of post-upgrade database cluster failed
pg_dumpall of post-upgrade database cluster failed
+ exit 1
make[2]: *** [check] Error 1
make[2]: Leaving directory `/home/kevin/pg/master/contrib/pg_upgrade'
make[1]: *** [check-pg_upgrade-recurse] Error 2
make[1]: Leaving directory `/home/kevin/pg/master/contrib'
make: *** [check-world-contrib-recurse] Error 2

Did I merge it wrong?

-Kevin

Excerpts from Kevin Grittner's message of mié jun 27 08:40:58 -0400 2012:
> Alvaro Herrera wrote:
>
> > here's fklocks v14, which also merges to new master as there were
> > several other conflicts. It passes make installcheck-world now.
>
> Recent commits broke it again, so here's a rebased v15. (No changes
> other than attempting to merge your work with the pg_controldata and
> pg_resetxlog changes and wrapping that FIXME in a comment.)

Oooh, so sorry -- I was supposed to have git-stashed that before
producing the patch. This change cannot have caused the pg_dumpall
problem below though, because it only touched the multixact cache code.

> Using that patch, pg_upgrade completes, but pg_dumpall fails:

Hmm, something changed enough that requires more than just a code merge.
I'll look into it.

Thanks for the merge.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Kevin Grittner's message of mié jun 27 08:40:58 -0400 2012:
> Alvaro Herrera wrote:
>
> > here's fklocks v14, which also merges to new master as there were
> > several other conflicts. It passes make installcheck-world now.
>
> Recent commits broke it again, so here's a rebased v15. (No changes
> other than attempting to merge your work with the pg_controldata and
> pg_resetxlog changes and wrapping that FIXME in a comment.)

Here's v16, merged to latest stuff, before attempting to fix this:

> Using that patch, pg_upgrade completes, but pg_dumpall fails:

> + pg_ctl start -l
> /home/kevin/pg/master/contrib/pg_upgrade/log/postmaster2.log -w
> waiting for server to start.... done
> server started
> + pg_dumpall
> pg_dump: Dumping the contents of table "hslot" failed: PQgetResult()
> failed.
> pg_dump: Error message from server: ERROR: MultiXactId 115 does no
> longer exist -- apparent wraparound

I was only testing migrating from an old version into patched, not
same-version upgrades.

I think I see what's happening here.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of vie jun 29 19:17:02 -0400 2012:
> Excerpts from Kevin Grittner's message of mié jun 27 08:40:58 -0400 2012:
> > Alvaro Herrera wrote:
> >
> > > here's fklocks v14, which also merges to new master as there were
> > > several other conflicts. It passes make installcheck-world now.
> >
> > Recent commits broke it again, so here's a rebased v15. (No changes
> > other than attempting to merge your work with the pg_controldata and
> > pg_resetxlog changes and wrapping that FIXME in a comment.)
>
> Here's v16, merged to latest stuff,

Really attached now.

BTW you can get this on github:
https://github.com/alvherre/postgres/tree/fklocks

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of vie jun 29 19:17:02 -0400 2012:

> I was only testing migrating from an old version into patched, not
> same-version upgrades.
>
> I think I see what's happening here.

Okay, I have pushed the fix to github -- as I suspected, code-wise the
fix was simple. I will post an updated consolidated patch later today.

make check of pg_upgrade now passes for me.

It would be nice that "make installcheck" would notify me that some
modules' tests are being skipped ...

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Updated patch attached.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of jue jul 05 18:44:11 -0400 2012:
>
> Updated patch attached.
>

Patch rebased to latest sources. This is also on Github:
https://github.com/alvherre/postgres/tree/fklocks

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Patch v19 contains some tweaks. Most notably,

1. if an Xid requests a lock A, and then a lock B which is stronger than
A, then record only lock B and forget lock A. This is important for
performance, because EvalPlanQual obtains ForUpdate locks on the tuples
that it chases on an update chain. If EPQ was called by an update or
delete, previously a MultiXact was being created.

In a stock pgbench run this was causing lots of multis to be created,
even when there are no FKs.

This was most likely involved in the 9% performance decrease that was
previously reported.

2. Save a few TransactionIdIsInProgress calls in MultiXactIdWait. We
were calling it to determine how many transactions were still running,
but not all callers were interested in that info. XidIsInProgress is
not particularly cheap, so it seems good to skip it if possible. I
didn't try to measure the difference, however.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Aug 22, 2012 at 5:31 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Patch v19 contains some tweaks. Most notably,
>
> 1. if an Xid requests a lock A, and then a lock B which is stronger than
> A, then record only lock B and forget lock A. This is important for
> performance, because EvalPlanQual obtains ForUpdate locks on the tuples
> that it chases on an update chain. If EPQ was called by an update or
> delete, previously a MultiXact was being created.
>
> In a stock pgbench run this was causing lots of multis to be created,
> even when there are no FKs.
>
> This was most likely involved in the 9% performance decrease that was
> previously reported.

Ah-ha! Neat. I'll try to find some time to re-benchmark this during
the next CF, unless you did that already.

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

Excerpts from Alvaro Herrera's message of mié ago 22 17:31:28 -0400 2012:
>
> Patch v19 contains some tweaks. Most notably,

v20 is merged to latest master; the only change other than automatic
merge is to update for pg_upgrade API fixes.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

v21 is merged to latest master.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Friday, August 31, 2012 06:59:51 AM Alvaro Herrera wrote:
> v21 is merged to latest master.
Ok, I am starting to look at this.

(working with a git merge of alvherre/fklocks into todays master)

In a very first pass as somebody who hasn't followed the discussions in the
past I took notice of the following things:

* compiles and survives make check
* README.tuplock jumps in quite fast without any introduction

* the reasons for the redesign aren't really included in the patch but just in
the - very nice - pgcon paper.

* "This is a bit of a performance regression, but since we expect FOR SHARE
locks to be seldom used, we don’t feel this is a serious problem." makes me a
bit uneasy, will look at performance separately

* Should RelationGetIndexattrBitmap check whether a unique index is referenced
from a pg_constraint row? Currently we pay part of the overhead independent
from the existance of foreign keys.

* mode == LockTupleKeyShare, == LockTupleShare, == LockTupleUpdate in
heap_lock_tuple's BeingUpdated branch look like they should be an if/else if

* I don't really like HEAP_UPDATE_KEY_REVOKED as a name, what about
HEAP_KEYS_UPDATED (akin to HEAP_HOT_UPDATED)

* how can we get infomask2 & HEAP_UPDATE_KEY_REVOKED && infomask &
HEAP_XMAX_LOCK_ONLY?

* heap_lock_tuple with mode == LockTupleKeyShare && nowait looks like it would
wait anyway in heap_lock_updated_tuple_rec

* rename HeapSatisfiesHOTUpdate, adjust comments?

* I wonder whether HeapSatisfiesHOTUpdate could be made to compute the result
for key_attrs and hot_attrs at the same time, often enough they will do the
same thing on the same column

* you didn't start it but I find that all those l$i jump labels make the code
harder to follow

* shouldn't XmaxGetUpdateXid be rather called MultiXactIdGetUpdateXid or such?

*
/*
* If the existing lock mode is identical to or weaker than the new
* one, we can act as though there is no existing lock and have the
* upper block handle this.
*/
"block"?

* I am not yet sure whether the (xmax == add_to_xmax) optimization in
compute_new_xmax_infomask is actually safe

* ConditionalMultiXactIdWait and MultiXactIdWait should probably rely on a
common implementation

* I am not that convinced that moving the waiting functions away from
multixact.c is a good idea, but I guess the required knowledge about lockmodes
makes it hard not to do so

* Haven't looked yet, but I have a slightly uneasy feeling about Hot Standby
interactions. Did you look at that?

* Hm?
HeapTupleSatisfiesVacuum
#if 0
ResetMultiHintBit(tuple, buffer, xmax, true);
#endif

This obviously is not a real review, but just learning what the problem & patch
actually try to do. This is quite a bit to take in ;). I will let it sink in
ant try to have a look at the architecture and performance next.

Greetings,

Andres

.oO(and people call catalog timetravel complicated)

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund wrote:
> On Friday, August 31, 2012 06:59:51 AM Alvaro Herrera wrote:
> > v21 is merged to latest master.
> Ok, I am starting to look at this.
>
> (working with a git merge of alvherre/fklocks into todays master)
>
> In a very first pass as somebody who hasn't followed the discussions in the
> past I took notice of the following things:
>
> * compiles and survives make check

Please verify src/test/isolation's make installcheck as well.

> * README.tuplock jumps in quite fast without any introduction

Hmm .. I'll give that a second look. Maybe some material from the pgcon
paper should be added as introduction.

> * "This is a bit of a performance regression, but since we expect FOR SHARE
> locks to be seldom used, we don’t feel this is a serious problem." makes me a
> bit uneasy, will look at performance separately

Thanks. Keep in mind though that the bits we really want good
performance for is FOR KEY SHARE, which is going to be used in foreign
keys. FOR SHARE is staying just for hypothetical backwards
compatibility. I just found that people is using it, see for example
http://stackoverflow.com/a/3243083

> * Should RelationGetIndexattrBitmap check whether a unique index is referenced
> from a pg_constraint row? Currently we pay part of the overhead independent
> from the existance of foreign keys.

Noah also suggested something more or less in the along the same lines.
My answer is that I don't want to do it currently; maybe we can improve
on what indexes we use later, but since this can be seen as a modularity
violation, I prefer to keep it as simple as possible.

> * mode == LockTupleKeyShare, == LockTupleShare, == LockTupleUpdate in
> heap_lock_tuple's BeingUpdated branch look like they should be an if/else if

Hm, will look at that.

> * I don't really like HEAP_UPDATE_KEY_REVOKED as a name, what about
> HEAP_KEYS_UPDATED (akin to HEAP_HOT_UPDATED)

Thanks.

> * how can we get infomask2 & HEAP_UPDATE_KEY_REVOKED && infomask &
> HEAP_XMAX_LOCK_ONLY?

SELECT FOR KEY UPDATE? This didn't exist initially, but previous
reviewers (Noah) felt that it made sense to have it for consistency. It
makes the lock conflict table make more sense, anyway.

> * heap_lock_tuple with mode == LockTupleKeyShare && nowait looks like it would
> wait anyway in heap_lock_updated_tuple_rec

Oops.

> * rename HeapSatisfiesHOTUpdate, adjust comments?

Yeah.

> * I wonder whether HeapSatisfiesHOTUpdate could be made to compute the result
> for key_attrs and hot_attrs at the same time, often enough they will do the
> same thing on the same column

Hmm, I will look at that.

> * you didn't start it but I find that all those l$i jump labels make the code
> harder to follow

You mean you suggest to have them renamed? That can be arranged.

> * shouldn't XmaxGetUpdateXid be rather called MultiXactIdGetUpdateXid or such?

Yes.

> /*
> * If the existing lock mode is identical to or weaker than the new
> * one, we can act as though there is no existing lock and have the
> * upper block handle this.
> */
> "block"?

Yeah, as in "the if {} block above". Since this is not clear maybe it
needs rewording.

> * I am not yet sure whether the (xmax == add_to_xmax) optimization in
> compute_new_xmax_infomask is actually safe

Will think about it and add a/some comment(s).

> * ConditionalMultiXactIdWait and MultiXactIdWait should probably rely on a
> common implementation

Will look at that.

> * I am not that convinced that moving the waiting functions away from
> multixact.c is a good idea, but I guess the required knowledge about lockmodes
> makes it hard not to do so

Yeah. The line between multixact.c and heapam.c is a zigzag currently,
I think. Maybe it needs to be more clear; I think the multixact modes
really belong into heapam.c, and they are just uint32 to multixact.c. I
wasn't brave enough to attempt this; maybe I should.

> * Haven't looked yet, but I have a slightly uneasy feeling about Hot Standby
> interactions. Did you look at that?
>
> * Hm?
> HeapTupleSatisfiesVacuum
> #if 0
> ResetMultiHintBit(tuple, buffer, xmax, true);
> #endif

Yeah. I had a couple of ResetMultiHintBit calls sprinkled in some of
the tqual routines, with the idea that if they saw multis that were no
longer live they could be removed, and replaced by just the remaining
updater. However, this doesn't really work because it's not safe to
change the Xmax when not holding exclusive lock, and the tqual routines
don't know that. So we're stuck with keeping the multi in there, even
when we know they are no longer interesting. This is a bit sad, because
it would be a performance benefit to remove them; but it's not strictly
necessary for correctness, so we can leave the optimization for a later
phase. I let those #ifdefed out calls in place so that it's easy to see
where the reset needs to happen.

> This obviously is not a real review, but just learning what the problem & patch
> actually try to do. This is quite a bit to take in ;). I will let it sink in
> ant try to have a look at the architecture and performance next.

Well, the patch is large and complex so any review is obviously going to
take a long time.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera wrote:
> Andres Freund wrote:

> > * heap_lock_tuple with mode == LockTupleKeyShare && nowait looks like it would
> > wait anyway in heap_lock_updated_tuple_rec
>
> Oops.

I took a stab at fixing this. However, it is not easy. First you need
a way to reproduce the problem, which involves setting breakpoints in
GDB. (Since a REPEATABLE READ transaction will fail to follow an update
chain due to "tuple concurrently updated", you need to use a READ
COMMITTED transaction; but obviously the timing to insert the bunch of
updates in the middle is really short. Hence I inserted a breakpoint at
the end of GetSnapshotData, had a SELECT FOR KEY SHARE NOWAIT get stuck
in it, and then launched a couple of updates in another session). I was
able to reproduce the undesirable wait.

I quickly patched heap_lock_updated_tuple to pass down the nowait flag,
but this is actually not reached, because the update chain is first
followed by EvalPlanQual in ExecLockRows, and not by
heap_lock_updated_tuple directly. And EPQ does not have the nowait
behavior. So it still blocks.

Maybe what we need to do is prevent ExecLockRows from following the
update chain altogether -- after all, if heap_lock_tuple is going to do
it by itself maybe it's wholly redundant.

Not really sure what's the best way to approach this. At this stage I'm
inclined to ignore the problem, unless some EPQ expert shows up and
tells me that (1) it's okay to patch EPQ in that way, or (2) we should
hack ExecLockRows (and remove EPQ?).

I pushed (to github) patches for a couple of other issues you raised.
Some others still need a bit more of my attention.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Here is version 22 of this patch. This version contains fixes to issues
reported by Andres, as well as a rebase to latest master.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Oct 18, 2012 at 04:58:20PM -0300, Alvaro Herrera wrote:
> Here is version 22 of this patch. This version contains fixes to issues
> reported by Andres, as well as a rebase to latest master.

I scanned this for obvious signs of work left to do. It contains numerous XXX
and FIXME comments. Many highlight micro-optimization opportunities and the
like; those can stay. Others preclude commit, either highlighting an unsolved
problem or wrongly highlighting a non-problem:

> + /*
> + * XXX we do not lock this tuple here; the theory is that it's sufficient
> + * with the buffer lock we're about to grab. Any other code must be able
> + * to cope with tuple lock specifics changing while they don't hold buffer
> + * lock anyway.
> + */

> * so they will be uninteresting by the time our next transaction starts.
> * (XXX not clear that this is correct --- other members of the MultiXact
> * could hang around longer than we did. However, it's not clear what a
> ! * better policy for flushing old cache entries would be.) FIXME actually
> ! * this is plain wrong now that multixact's may contain update Xids.

> ! nmembers = GetMultiXactIdMembers(multi, &members, true);
> ! /*
> ! * XXX we don't have the infomask to run the required consistency check
> ! * here; the required notational overhead seems excessive.
> ! */

> /* We assume the cache entries are sorted */
> ! /* XXX we assume the unused bits in "status" are zeroed */
> ! if (memcmp(members, entry->members, nmembers * sizeof(MultiXactMember)) == 0)

> ! * XXX do we have any issues with needing to checkpoint here?
> */
> ! static void
> ! TruncateMultiXact(void)
> {

> + /* FIXME what should we initialize this to? */
> + newFrozenMulti = ReadNextMultiXactId();

> + * FIXME -- the XMAX_IS_MULTI test is a bit wrong .. it's possible to
> + * have tuples with that bit set that are dead. However, if that's
> + * changed, the RawXmax() call below should probably be researched as well.
> */
> if (tuple->t_infomask &
> ! (HEAP_XMAX_INVALID | HEAP_XMAX_LOCK_ONLY | HEAP_XMAX_IS_MULTI))
> return false;

On Thursday, October 18, 2012 09:58:20 PM Alvaro Herrera wrote:
> Here is version 22 of this patch. This version contains fixes to issues
> reported by Andres, as well as a rebase to latest master.

Ok, I now that pgconf.eu has ended I am starting to do a real review:

* Is it ok to make FOR UPDATE somewhat weaker than before? In 9.2 and earlier
you could be sure that if you FOR UPDATE'ed a row you could delete it. Unless
I miss something now this will not block somebody else acquiring a FOR KEY
SHARE lock, so this guarantee is gone.
This seems likely to introduce subtle problems in user applications.

I propose renaming FOR UPDATE => FOR NO KEY UPDATE, FOR KEY UPDATE => FOR
UPDATE or similar (or PREVENT KEY UPDATE?). That keeps the old meaning of FOR
UPDATE.

You write "SELECT FOR UPDATE is a standards-compliant exclusive lock". I
didn't really find all that much about the semantics of FOR UPDATE on cursors
in the standard other than "The operations of update and delete are permitted
for updatable cursors, subject to constraining Access Rules.".

* I would welcome adding some explanatory comments about the point of
TupleLockExtraInfo and MultiXactStatusLock at the respective definition.

* Why do we have the HEAP_XMAX_IS_MULTI && HEAP_XMAX_LOCK_ONLY case?

* I think some of the longer comments could use the attention of a native
speaker, unfortunately thats not me.

* I am still uncomfortable with the FOR SHARE deoptimization. I have seen
people lock larger parts of their table to make some READ COMMITTED things
actually safe.
Is there any problem retaining the non XMAX_IS_MULTI behaviour except space in
infomask? That seems solveable by something like

#define HEAP_XMAX_SHR_LOCK 0x0010
#define HEAP_XMAX_EXCL_LOCK 0x0040
#define HEAP_XMAX_KEYSHR_LOCK (HEAP_XMAX_SHR_LOCK | HEAP_XMAX_EXCL_LOCK)

and somewhat more complex expressions when testing the locks ((infomask &
HEAP_XMAX_KEYSHR_LOCK ) == HEAP_XMAX_KEYSHR_LOCK, etc).

* In heap_lock_tuple's XMAX_IS_MULTI case

for (i = 0; i < nmembers; i++)
{
if (TransactionIdIsCurrentTransactionId(members[i].xid))
{
LockTupleMode membermode;

membermode =
TUPLOCK_from_mxstatus(members[i].status);

if (membermode > mode)
{
if (have_tuple_lock)
UnlockTupleTuplock(relation, tid, mode);

pfree(members);
return HeapTupleMayBeUpdated;
}
}
}

why is it membermode > mode and not membermode >= mode?

* Is the case of a a non-key-update followed by a key-update actually handled
when doing a heap_lock_tuple with mode = LockTupleKeyShare and follow_updates
= false? I don't really see how, so it seems to deserve at least a comment.

But then I don't yet understand why follow_update makes sense.

* In heap_lock_tuple with mode == LockTupleUpdate && infomask &
HEAP_XMAX_IS_MULTI, were leaking members when doing goto l3. Probably not
relevant, but given the code tries to be careful everywhere else...
We might also leak in the members == 0 case there, not sure yet.

Ok, this is at about 35% of the diff in my second pass, but I just arrived back
in Berlin, and this seems useful enough on its own...

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 27 October 2012 00:06, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On Thursday, October 18, 2012 09:58:20 PM Alvaro Herrera wrote:
>> Here is version 22 of this patch. This version contains fixes to issues
>> reported by Andres, as well as a rebase to latest master.
>
> Ok, I now that pgconf.eu has ended I am starting to do a real review:
>
> * Is it ok to make FOR UPDATE somewhat weaker than before? In 9.2 and earlier
> you could be sure that if you FOR UPDATE'ed a row you could delete it. Unless
> I miss something now this will not block somebody else acquiring a FOR KEY
> SHARE lock, so this guarantee is gone.

Yes, that is exactly the point of this.

> This seems likely to introduce subtle problems in user applications.

Yes, it could. So we need some good docs on explaining this.

Which applications use FOR UPDATE?
Any analysis of particular situations would be very helpful in doing
the correct thing here.

I think introducing FOR DELETE would be much clearer as an addition/
synonym for FOR KEY UPDATE.

> I propose renaming FOR UPDATE => FOR NO KEY UPDATE, FOR KEY UPDATE => FOR
> UPDATE or similar (or PREVENT KEY UPDATE?). That keeps the old meaning of FOR
> UPDATE.

Which is essentially unwinding the feature, to some extent.

Does FOR UPDATE throw an error if the user later issues an UPDATE of
the PK or a DELETE? That sequence of actions would cause lock
escalation in the application, which could also lead to
deadlock/contention.

This sounds like we need a GUC or table-level default to control
whether FOR UPDATE means FOR UPDATE or FOR DELETE

More thought/input required on this point, it seems important.

> You write "SELECT FOR UPDATE is a standards-compliant exclusive lock". I
> didn't really find all that much about the semantics of FOR UPDATE on cursors
> in the standard other than "The operations of update and delete are permitted
> for updatable cursors, subject to constraining Access Rules.".

> * I think some of the longer comments could use the attention of a native
> speaker, unfortunately thats not me.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sunday, October 28, 2012 11:47:16 PM Simon Riggs wrote:
> On 27 October 2012 00:06, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On Thursday, October 18, 2012 09:58:20 PM Alvaro Herrera wrote:
> >> Here is version 22 of this patch. This version contains fixes to issues
> >> reported by Andres, as well as a rebase to latest master.
> >
> > Ok, I now that pgconf.eu has ended I am starting to do a real review:
> >
> > * Is it ok to make FOR UPDATE somewhat weaker than before? In 9.2 and
> > earlier you could be sure that if you FOR UPDATE'ed a row you could
> > delete it. Unless I miss something now this will not block somebody else
> > acquiring a FOR KEY SHARE lock, so this guarantee is gone.
>
> Yes, that is exactly the point of this.

Yes, sure. The point is the introduction of a weaker lock level which can be
used by the ri triggers. I don't see any imperative that the semantics of the
old lock level need to be redefined. That just seems dangerous to me.

We need to take care to reduce the complications of upgrades not introduce
changes that require complex code reviews.

> > This seems likely to introduce subtle problems in user applications.
>
> Yes, it could. So we need some good docs on explaining this.
>
> Which applications use FOR UPDATE?

Any that want to protect themselves against deadlocks and/or visibility
problems due to READ COMMITTED. Thats quite a bunch.

> Any analysis of particular situations would be very helpful in doing
> the correct thing here.

Usual things include

* avoiding problems due to lock upgrades in combination with foreign keys (as
far as I can see some of those still persist).
* prevent rows being deleted by other transactions
* prepare for updating if computation of the new values take some time
* guarantee order of locking to make sure rows are DELETE/UPDATEed in the same
order (still no ORDER BY in UPDATE/DELETE)
...

> I think introducing FOR DELETE would be much clearer as an addition/
> synonym for FOR KEY UPDATE.

Hm. Not really convinced. For me that seems to just make the overall situation
even more complex.

> > I propose renaming FOR UPDATE => FOR NO KEY UPDATE, FOR KEY UPDATE => FOR
> > UPDATE or similar (or PREVENT KEY UPDATE?). That keeps the old meaning of
> > FOR UPDATE.
>
> Which is essentially unwinding the feature, to some extent.

Why? The overall features available are just the same? The only thing is that
existing semantics aren't changed.

> Does FOR UPDATE throw an error if the user later issues an UPDATE of
> the PK or a DELETE? That sequence of actions would cause lock
> escalation in the application, which could also lead to
> deadlock/contention.

Unless I miss something it precisely does *not* result in lock escalation with
the 9.2 semanticsbut it *would* with fklocks applied. Thats exactly my point.

> This sounds like we need a GUC or table-level default to control
> whether FOR UPDATE means FOR UPDATE or FOR DELETE

I don't like adding a new guc for something that should be solveable with some
creative naming. If a new user doesn't get a bit more concurrency due to
manually issued 9.2 FOR UPDATE implicitly being converted into a FOR NO KEY
UPDATE its not too bad. The code needs to be checked whether thats valid
anyway. The reverse is not true...

> More thought/input required on this point, it seems important.

Yep, more input welcome.

Greetings,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 29 October 2012 12:27, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

>> This sounds like we need a GUC or table-level default to control
>> whether FOR UPDATE means FOR UPDATE or FOR DELETE
>
> I don't like adding a new guc for something that should be solveable with some
> creative naming. If a new user doesn't get a bit more concurrency due to
> manually issued 9.2 FOR UPDATE implicitly being converted into a FOR NO KEY
> UPDATE its not too bad. The code needs to be checked whether thats valid
> anyway. The reverse is not true...

The main point here is that introducing new non-optional behaviour is
a compatibility problem and we shouldn't rush that.

If we decide to change FOR UPDATE to mean FOR NO KEY UPDATE that needs
separate consideration and shouldn't happen until sometime after the
feature goes in (months, or perhaps releases).

We're introducing a new feature that will allow us to avoid lock
problems, by taking the current FOR UPDATE behaviour and splitting it
into two options: one weaker and one stronger. We need explicit names
for both of those options. The most obvious naming is
FOR [[NO] KEY] UPDATE

Don't much like that, but its pretty unambiguous and fairly neat.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Here's version 23 of this patch, with fixes for the below comments and
a merge to master.

Andres Freund wrote:
> On Thursday, October 18, 2012 09:58:20 PM Alvaro Herrera wrote:
> > Here is version 22 of this patch. This version contains fixes to issues
> > reported by Andres, as well as a rebase to latest master.
>
> Ok, I now that pgconf.eu has ended I am starting to do a real review:
>
> * Is it ok to make FOR UPDATE somewhat weaker than before? In 9.2 and earlier
> you could be sure that if you FOR UPDATE'ed a row you could delete it. Unless
> I miss something now this will not block somebody else acquiring a FOR KEY
> SHARE lock, so this guarantee is gone.
> This seems likely to introduce subtle problems in user applications.
>
> I propose renaming FOR UPDATE => FOR NO KEY UPDATE, FOR KEY UPDATE => FOR
> UPDATE or similar (or PREVENT KEY UPDATE?). That keeps the old meaning of FOR
> UPDATE.

Okay, in subsequent discussion everyone agreed that this is necessary;
thanks for noticing the problem. Instead of your suggestion, however, I
used Noah's; luckily I just noticed that it was identical to yours. So
apparently there is also agreement on this point. I have updated the
code, comments and docs (and renamed some other stuff to match).

> You write "SELECT FOR UPDATE is a standards-compliant exclusive lock". I
> didn't really find all that much about the semantics of FOR UPDATE on cursors
> in the standard other than "The operations of update and delete are permitted
> for updatable cursors, subject to constraining Access Rules.".

I don't really know where that comes from, but it's a statement I
grabbed from the docs somewhere.

> * I would welcome adding some explanatory comments about the point of
> TupleLockExtraInfo and MultiXactStatusLock at the respective definition.

Done. If that's not enough, I would welcome suggestions or specific
question needing clarification.

> * Why do we have the HEAP_XMAX_IS_MULTI && HEAP_XMAX_LOCK_ONLY case?

For pg_upgrade. HEAP_XMAX_LOCK_ONLY is the value previously used by
HEAP_XMAX_SHARED_LOCK, so a database containing such values that is
upgraded will contain that bit pattern.

> * I think some of the longer comments could use the attention of a native
> speaker, unfortunately thats not me.

Sorry about that.

> * I am still uncomfortable with the FOR SHARE deoptimization. I have seen
> people lock larger parts of their table to make some READ COMMITTED things
> actually safe.

I haven't changed this yet. There are bits available in infomask2 that
we could use, if we agree that it is necessary; or, as you say, we could
use two bits to distinguish three different states, but we need to
ensure that pg_upgraded databases will work sanely.

> * In heap_lock_tuple's XMAX_IS_MULTI case
>
> [snip]
>
> why is it membermode > mode and not membermode >= mode?

Uh, that's a bug. Fixed. As noticed in the comment above that snippet,
there was a deadlock possible here. Maybe I should add a test to ensure
this doesn't happen.

> * Is the case of a a non-key-update followed by a key-update actually handled
> when doing a heap_lock_tuple with mode = LockTupleKeyShare and follow_updates
> = false? I don't really see how, so it seems to deserve at least a comment.

I wasn't able to figure out what you think is the problem.

> But then I don't yet understand why follow_update makes sense.

Basically heap_lock_tuple can be told by the caller to follow the update
chain to lock subsequent tuples, or not. Mainly, EvalPlanQual does not
want this to happen, because it does its own update-chain walking.

In all honesty, it is not clear to me that this is the proper solution
to the problem. Maybe some hacking around ExecLockRows is necessary.

> * In heap_lock_tuple with mode == LockTupleUpdate && infomask &
> HEAP_XMAX_IS_MULTI, were leaking members when doing goto l3. Probably not
> relevant, but given the code tries to be careful everywhere else...

Right, plugged.

> We might also leak in the members == 0 case there, not sure yet.

Hm, I don't think GetMultiXactIdMembers really considers the case when 0
members are to be returned. Maybe that needs some more tweaking.

> Ok, this is at about 35% of the diff in my second pass, but I just arrived back
> in Berlin, and this seems useful enough on its own...

Many thanks.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera wrote:

> > * In heap_lock_tuple's XMAX_IS_MULTI case
> >
> > [snip]
> >
> > why is it membermode > mode and not membermode >= mode?
>
> Uh, that's a bug. Fixed. As noticed in the comment above that snippet,
> there was a deadlock possible here. Maybe I should add a test to ensure
> this doesn't happen.

Done:
https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770

(I don't think this is worth a v24 submission).

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Nov 14, 2012 at 01:27:26PM -0300, Alvaro Herrera wrote:
> https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770
>
> (I don't think this is worth a v24 submission).

Are you aware of any defects in or unanswered questions of this version that
would stall your commit thereof?

On 2012-11-14 13:27:26 -0300, Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>
> > > * In heap_lock_tuple's XMAX_IS_MULTI case
> > >
> > > [snip]
> > >
> > > why is it membermode > mode and not membermode >= mode?
> >
> > Uh, that's a bug. Fixed. As noticed in the comment above that snippet,
> > there was a deadlock possible here. Maybe I should add a test to ensure
> > this doesn't happen.
>
> Done:
> https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770
>
> (I don't think this is worth a v24 submission).

I have started doing some performance testing and I fear I was right in
being suspicious about the performance difference for FOR SHARE locks:

Tested with
pgbench -p 5442 -U andres \
-c 30 -j 30 \
-M prepared -f ~/tmp/postgres-fklocks/select-for-share.sql \
-T 20 postgres

on a pgbench -i -s 10 database, where select-for-share.sql is:

BEGIN;
\set naccounts 1000000
\setrandom aid 1 :naccounts
SELECT abalance FROM pgbench_accounts WHERE aid = :aid FOR SHARE;
\setrandom aid 1 :naccounts
SELECT abalance FROM pgbench_accounts WHERE aid = :aid FOR SHARE;
\setrandom aid 1 :naccounts
SELECT abalance FROM pgbench_accounts WHERE aid = :aid FOR SHARE;
COMMIT;

which very roughly resembles workloads I have seen in reality (locking
some records your rely on while you do some work).

With
52b4729fcfc20f056f17531a6670d8c4b9696c39 (alvherre/fklocks)
vs
273986bf0d39e5166eb15ba42ebff4803e23a688 (latest merged master)

I get
tps = 8986.133496 (excluding connections establishing)
vs
tps = 25307.861193 (excluding connections establishing)

Thats nearly a factor of three which seems to be too big to be
acceptable to me.
So I really think we need to bring FOR SHARE locks back as a flag.

I have done some benchmarking of other cases (plain pgbench, pgbench
with foreign keys, large insertions, large amounts of FOR SHARE locks)
and haven't found anything really outstanding so far.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Noah Misch wrote:
> On Wed, Nov 14, 2012 at 01:27:26PM -0300, Alvaro Herrera wrote:
> > https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770
> >
> > (I don't think this is worth a v24 submission).
>
> Are you aware of any defects in or unanswered questions of this version that
> would stall your commit thereof?

Yeah, I am revisiting the list of XXX/FIXME comments you pointed out
previously.

And I would still like someone with EPQ understanding to review the
ExecLockRows / EvalPlanQual / heap_lock_tuple interactions.

Andres is on the verge of convincing me that we need to support
singleton FOR SHARE without multixacts due to performance concerns. It
would be useful for more people to chime in here: is FOR SHARE an
important case to cater for? I wonder if using FOR KEY SHARE (keep
performance characteristics, but would need to revise application code)
would satisfy Andres' users, for example.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-11-16 13:17:47 -0300, Alvaro Herrera wrote:
> Noah Misch wrote:
> > On Wed, Nov 14, 2012 at 01:27:26PM -0300, Alvaro Herrera wrote:
> > > https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770
> > >
> > > (I don't think this is worth a v24 submission).
> >
> > Are you aware of any defects in or unanswered questions of this version that
> > would stall your commit thereof?
>
> Yeah, I am revisiting the list of XXX/FIXME comments you pointed out
> previously.
>
> And I would still like someone with EPQ understanding to review the
> ExecLockRows / EvalPlanQual / heap_lock_tuple interactions.

I am in the process of looking at those atm, but we need somebody that
actually understands the intricacies here...

> Andres is on the verge of convincing me that we need to support
> singleton FOR SHARE without multixacts due to performance concerns.

I don't really see any arguments against doing so. We aren't in a that
big shortage of flags and if we need more than available I think we can
free some (e.g. XMAX/XMIN_INVALID).

> It
> would be useful for more people to chime in here: is FOR SHARE an
> important case to cater for? I wonder if using FOR KEY SHARE (keep
> performance characteristics, but would need to revise application code)
> would satisfy Andres' users, for example.

It definitely wouldn't help in the cases I have seen because the point
is to protect against actual content changes of the rows, not just the
keys.
Note that you actually need to use explicit FOR SHARE/UPDATE for
correctness purposes in many scenarios unless youre running in 9.1+
serializable mode. And that cannot be used in some cases (try queuing
for example) because the rollback rates would be excessive.

Even if applications could be fixed, requiring changes to applications
locking behaviour, which possibly is far from trivial, seems like a too
big upgrade barrier.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Nov 16, 2012 at 05:31:12PM +0100, Andres Freund wrote:
> On 2012-11-16 13:17:47 -0300, Alvaro Herrera wrote:
> > Andres is on the verge of convincing me that we need to support
> > singleton FOR SHARE without multixacts due to performance concerns.
>
> I don't really see any arguments against doing so. We aren't in a that
> big shortage of flags and if we need more than available I think we can
> free some (e.g. XMAX/XMIN_INVALID).

The patch currently leaves two unallocated bits. Reclaiming currently-used
bits means a binary compatibility break. Until we plan out such a thing,
reclaimable bits are not as handy as never-allocated bits. I don't think we
should lightly expend one of the final two.

> > It
> > would be useful for more people to chime in here: is FOR SHARE an
> > important case to cater for? I wonder if using FOR KEY SHARE (keep
> > performance characteristics, but would need to revise application code)
> > would satisfy Andres' users, for example.
>
> It definitely wouldn't help in the cases I have seen because the point
> is to protect against actual content changes of the rows, not just the
> keys.
> Note that you actually need to use explicit FOR SHARE/UPDATE for
> correctness purposes in many scenarios unless youre running in 9.1+
> serializable mode. And that cannot be used in some cases (try queuing
> for example) because the rollback rates would be excessive.

I agree that tripling FOR SHARE cost is risky. Where is the added cost
concentrated? Perchance that multiple belies optimization opportunities.

Thanks,
nm

On 2012-11-16 22:31:51 -0500, Noah Misch wrote:
> On Fri, Nov 16, 2012 at 05:31:12PM +0100, Andres Freund wrote:
> > On 2012-11-16 13:17:47 -0300, Alvaro Herrera wrote:
> > > Andres is on the verge of convincing me that we need to support
> > > singleton FOR SHARE without multixacts due to performance concerns.
> >
> > I don't really see any arguments against doing so. We aren't in a that
> > big shortage of flags and if we need more than available I think we can
> > free some (e.g. XMAX/XMIN_INVALID).
>
> The patch currently leaves two unallocated bits. Reclaiming currently-used
> bits means a binary compatibility break. Until we plan out such a thing,
> reclaimable bits are not as handy as never-allocated bits. I don't think we
> should lightly expend one of the final two.

Not sure what you mean with a binary compatibility break?
pg_upgrade'ability?

What I previously suggested somewhere was:

#define HEAP_XMAX_SHR_LOCK 0x0010
#define HEAP_XMAX_EXCL_LOCK 0x0040
#define HEAP_XMAX_KEYSHR_LOCK (HEAP_XMAX_SHR_LOCK|HEAP_XMAX_EXCL_LOCK)
/*
* Different from _LOCK_BITS because it doesn't include LOCK_ONLY
*/
#define HEAP_LOCK_MASK (HEAP_XMAX_SHR_LOCK|HEAP_XMAX_EXCL_LOCK)

#define HEAP_XMAX_IS_SHR_LOCKED(tup) \
(((tup)->t_infomask & HEAP_LOCK_BITS) == HEAP_XMAX_SHR_LOCK)
#define HEAP_XMAX_IS_EXCL_LOCKED(tup) \
(((tup)->t_infomask & HEAP_LOCK_BITS) == HEAP_XMAX_EXCL_LOCK)
#define HEAP_XMAX_IS_KEYSHR_LOCKED(tup) \
(((tup)->t_infomask & HEAP_LOCK_BITS) == HEAP_XMAX_KEYSHR_LOCK)

It makes checking for locks sightly more more complicated, but its not
too bad...

> > > It
> > > would be useful for more people to chime in here: is FOR SHARE an
> > > important case to cater for? I wonder if using FOR KEY SHARE (keep
> > > performance characteristics, but would need to revise application code)
> > > would satisfy Andres' users, for example.
> >
> > It definitely wouldn't help in the cases I have seen because the point
> > is to protect against actual content changes of the rows, not just the
> > keys.
> > Note that you actually need to use explicit FOR SHARE/UPDATE for
> > correctness purposes in many scenarios unless youre running in 9.1+
> > serializable mode. And that cannot be used in some cases (try queuing
> > for example) because the rollback rates would be excessive.
>
> I agree that tripling FOR SHARE cost is risky. Where is the added cost
> concentrated? Perchance that multiple belies optimization opportunities.

Good question, let me play a bit.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Nov 17, 2012 at 03:20:20PM +0100, Andres Freund wrote:
> On 2012-11-16 22:31:51 -0500, Noah Misch wrote:
> > On Fri, Nov 16, 2012 at 05:31:12PM +0100, Andres Freund wrote:
> > > On 2012-11-16 13:17:47 -0300, Alvaro Herrera wrote:
> > > > Andres is on the verge of convincing me that we need to support
> > > > singleton FOR SHARE without multixacts due to performance concerns.
> > >
> > > I don't really see any arguments against doing so. We aren't in a that
> > > big shortage of flags and if we need more than available I think we can
> > > free some (e.g. XMAX/XMIN_INVALID).
> >
> > The patch currently leaves two unallocated bits. Reclaiming currently-used
> > bits means a binary compatibility break. Until we plan out such a thing,
> > reclaimable bits are not as handy as never-allocated bits. I don't think we
> > should lightly expend one of the final two.
>
> Not sure what you mean with a binary compatibility break?
> pg_upgrade'ability?

Yes. If we decide HEAP_XMIN_INVALID isn't helping, we can stop adding it to
tuples anytime. Old tuples may continue to carry the bit, with no particular
deadline for their demise. To reuse that bit in the mean time, we'll need to
prove that no tuple writable by PostgreSQL 8.3+ will get an unacceptable
interpretation under the new meaning of the bit. Alternately, build the
mechanism to prove that all such old bits are gone before using the bit in the
new way. This keeps HEAP_MOVED_IN and HEAP_MOVED_OFF unavailable today.

> What I previously suggested somewhere was:
>
> #define HEAP_XMAX_SHR_LOCK 0x0010
> #define HEAP_XMAX_EXCL_LOCK 0x0040
> #define HEAP_XMAX_KEYSHR_LOCK (HEAP_XMAX_SHR_LOCK|HEAP_XMAX_EXCL_LOCK)
> /*
> * Different from _LOCK_BITS because it doesn't include LOCK_ONLY
> */
> #define HEAP_LOCK_MASK (HEAP_XMAX_SHR_LOCK|HEAP_XMAX_EXCL_LOCK)
>
> #define HEAP_XMAX_IS_SHR_LOCKED(tup) \
> (((tup)->t_infomask & HEAP_LOCK_BITS) == HEAP_XMAX_SHR_LOCK)
> #define HEAP_XMAX_IS_EXCL_LOCKED(tup) \
> (((tup)->t_infomask & HEAP_LOCK_BITS) == HEAP_XMAX_EXCL_LOCK)
> #define HEAP_XMAX_IS_KEYSHR_LOCKED(tup) \
> (((tup)->t_infomask & HEAP_LOCK_BITS) == HEAP_XMAX_KEYSHR_LOCK)
>
> It makes checking for locks sightly more more complicated, but its not
> too bad...

Agreed; that seems reasonable.

> > I agree that tripling FOR SHARE cost is risky. Where is the added cost
> > concentrated? Perchance that multiple belies optimization opportunities.
>
> Good question, let me play a bit.

Ok, I benchmarked around and from what I see there is no single easy
target.
The biggest culprits I could find are:
1. higher amount of XLogInsert calls per transaction (visible
in pgbench -t instead of -T mode while watching the WAL volume)
2. Memory allocations in GetMultiXactIdMembers
3. Memory allocations in mXactCachePut
a) cache entry itself
b) the cache context
4. More lwlocks acquisitions

We can possibly optimize a bit with 2) by using a static buffer for
common member sizes, but thats not going to buy us too much...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Nov 17, 2012 at 05:07:18PM +0100, Andres Freund wrote:
> > > I agree that tripling FOR SHARE cost is risky. Where is the added cost
> > > concentrated? Perchance that multiple belies optimization opportunities.
> >
> > Good question, let me play a bit.
>
> Ok, I benchmarked around and from what I see there is no single easy
> target.
> The biggest culprits I could find are:
> 1. higher amount of XLogInsert calls per transaction (visible
> in pgbench -t instead of -T mode while watching the WAL volume)
> 2. Memory allocations in GetMultiXactIdMembers
> 3. Memory allocations in mXactCachePut
> a) cache entry itself
> b) the cache context
> 4. More lwlocks acquisitions
>
> We can possibly optimize a bit with 2) by using a static buffer for
> common member sizes, but thats not going to buy us too much...

In that case, +1 for your proposal to prop up FOR SHARE.

On 2012-11-14 13:27:26 -0300, Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>
> > > * In heap_lock_tuple's XMAX_IS_MULTI case
> > >
> > > [snip]
> > >
> > > why is it membermode > mode and not membermode >= mode?
> >
> > Uh, that's a bug. Fixed. As noticed in the comment above that snippet,
> > there was a deadlock possible here. Maybe I should add a test to ensure
> > this doesn't happen.
>
> Done:
> https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770

Some more review bits, based on ffd6250d1d393f2ecb9bfc55c2c6f715dcece557

- if oldestMultiXactId + db is set and then that database is dropped we seem to
have a problem because MultiXactAdvanceOldest won't overwrite those
values. Should probably use SetMultiXactIdLimit directly.

- what stop multixacts only being filled out (i.e RecordNewMultiXact()) *after*
the XLogInsert() *and* after a MultiXactGetCheckptMulti()? Afaics
MultiXactGenLock is not hold in CreateMultiXactId(). If we crash in that
moment we loose the multixact data which now means potential data loss...

- multixact member group data crossing 512 sector boundaries makes me uneasy
(as its 5 bytes). I don't really see a scenario where its dangerous, but
... Does anybody see a problem here?

- there are quite some places that do
multiStopLimit = multiWrapLimit - 100;
if (multiStopLimit < FirstMultiXactId)
multiStopLimit -= FirstMultiXactId;

perhaps MultiXactIdAdvance and MultiXactIdRetreat macros are in order?

- I find using a default: clause in switches with enum types where everything
is expected to be handled like the following a bad idea, this way the
compiler won't warn you if youve missed case's which makes changing/extending code harder:
switch (rc->strength)
{
case LCS_FORNOKEYUPDATE:
newrc->markType = ROW_MARK_EXCLUSIVE;
break;
case LCS_FORSHARE:
newrc->markType = ROW_MARK_SHARE;
break;
case LCS_FORKEYSHARE:
newrc->markType = ROW_MARK_KEYSHARE;
break;
case LCS_FORUPDATE:
newrc->markType = ROW_MARK_KEYEXCLUSIVE;
break;
default:
elog(ERROR, "unsupported rowmark type %d", rc->strength);
}
-
#if 0
/*
* The idea here is to remove the IS_MULTI bit, and replace the
* xmax with the updater's Xid. However, we can't really do it:
* modifying the Xmax is not allowed under our buffer locking
* rules, unless we have an exclusive lock; but we don't know that
* we have it. So the multi needs to remain in place :-(
*/
ResetMultiHintBit(tuple, buffer, xmax, true);
#endif

Three things:
- HeapTupleSatisfiesUpdate is actually always called exclusively locked ;)
- Extending something like LWLockHeldByMe to also return the current
lockmode doesn't sound hard
- we seem to be using #ifdef NOT_YET for such cases

- Using a separate production for the lockmode seems to be nicer imo, not
really important though
for_locking_item:
FOR UPDATE locked_rels_list opt_nowait
...
| FOR NO KEY UPDATE locked_rels_list opt_nowait
...
| FOR SHARE locked_rels_list opt_nowait
...
| FOR KEY SHARE locked_rels_list opt_nowait
;

- not really padding, MultiXactStatus is 4bytes...
/*
* XXX Note: there's a lot of padding space in MultiXactMember. We could
* find a more compact representation of this Xlog record -- perhaps all the
* status flags in one XLogRecData, then all the xids in another one? Not
* clear that it's worth the trouble though.
*/
- why
#define SizeOfMultiXactCreate (offsetof(xl_multixact_create, nmembers) + sizeof(int32))
and not
#define SizeOfMultiXactCreate offsetof(xl_multixact_create, members)
- starting a critical section in GetNewMultiXactId but not ending it is ugly,
but not new

Greetings,

Andres

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-11-14 13:27:26 -0300, Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>
> > > * In heap_lock_tuple's XMAX_IS_MULTI case
> > >
> > > [snip]
> > >
> > > why is it membermode > mode and not membermode >= mode?
> >
> > Uh, that's a bug. Fixed. As noticed in the comment above that snippet,
> > there was a deadlock possible here. Maybe I should add a test to ensure
> > this doesn't happen.
>
> Done:
> https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770
>
> (I don't think this is worth a v24 submission).

One more observation:
/*
* Get and lock the updated version of the row; if fail, return
NULL.
*/
- copyTuple = EvalPlanQualFetch(estate, relation, LockTupleExclusive,
+ copyTuple = EvalPlanQualFetch(estate, relation, LockTupleNoKeyExclusive,

That doesn't seem to be correct to me. Why is it ok to acquire a
potentially too low locklevel here?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund wrote:

> - I find using a default: clause in switches with enum types where everything
> is expected to be handled like the following a bad idea, this way the
> compiler won't warn you if youve missed case's which makes changing/extending code harder:
> switch (rc->strength)
> {

I eliminated some of these default clauses, but the compiler is not
happy about not having some of them, so they remain.

> -
> #if 0
> /*
> * The idea here is to remove the IS_MULTI bit, and replace the
> * xmax with the updater's Xid. However, we can't really do it:
> * modifying the Xmax is not allowed under our buffer locking
> * rules, unless we have an exclusive lock; but we don't know that
> * we have it. So the multi needs to remain in place :-(
> */
> ResetMultiHintBit(tuple, buffer, xmax, true);
> #endif
>
> Three things:
> - HeapTupleSatisfiesUpdate is actually always called exclusively locked ;)
> - Extending something like LWLockHeldByMe to also return the current
> lockmode doesn't sound hard
> - we seem to be using #ifdef NOT_YET for such cases

After spending some time trying to make this work usefully, I observed
that it's pointless, at least if we apply it only in
HeapTupleSatisfiesUpdate, because the IS_MULTI bit is going to be
removed by compute_new_xmax_infomask if the multi is gone. Something
like this would be useful if we could do it in other places; say when
we're merely scanning page without the specific intention of modifying
that particular tuple. Maybe in heap_prune_page, for example. ISTM we
can see that as an optimization we can add later.

For the record, the implementation of ResetMultiHintBit looks like this:

/*
* When a tuple is found to be marked by a MultiXactId, all whose members are
* completed transactions, the HEAP_XMAX_IS_MULTI bit can be removed.
* Additionally, at the request of caller, we can set the Xmax to the given
* Xid, and set HEAP_XMAX_COMMITTED.
*
* Note that per buffer access rules, the caller to this function must hold
* exclusive content lock on the affected buffer.
*/
static inline void
ResetMultiHintBit(HeapTupleHeader tuple, Buffer buffer,
TransactionId xid, bool mark_committed)
{
Assert(LWLockModeHeld((&BufferDescriptors[buffer])->content_lock ==
LW_EXCLUSIVE));
Assert(tuple->t_infomask & HEAP_XMAX_IS_MULTI);
Assert(!(tuple->t_infomask & HEAP_XMAX_INVALID));
Assert(!MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)));

tuple->t_infomask &= ~HEAP_XMAX_BITS;
HeapTupleHeaderSetXmax(tuple, xid);
if (!TransactionIdIsValid(xid))
tuple->t_infomask |= HEAP_XMAX_INVALID;
/* note that HEAP_KEYS_UPDATED persists, if set */

if (mark_committed)
SetHintBits(tuple, buffer, HEAP_XMAX_COMMITTED, xid);
else
SetBufferCommitInfoNeedsSave(buffer);
}

(This is removed by commit 52f86f82fb3e01a6ed0b8106bac20f319bb3ad0a in
my github tree, but that commit might be lost in the future, hence this
paste.)

Some of your other observations have been fixed by commits that I have
pushed to my github tree.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-11-20 17:36:14 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
>
>
> > - I find using a default: clause in switches with enum types where everything
> > is expected to be handled like the following a bad idea, this way the
> > compiler won't warn you if youve missed case's which makes changing/extending code harder:
> > switch (rc->strength)
> > {
>
> I eliminated some of these default clauses, but the compiler is not
> happy about not having some of them, so they remain.

You have removed the ones that looked removable to me...

>
> > -
> > #if 0
> > /*
> > * The idea here is to remove the IS_MULTI bit, and replace the
> > * xmax with the updater's Xid. However, we can't really do it:
> > * modifying the Xmax is not allowed under our buffer locking
> > * rules, unless we have an exclusive lock; but we don't know that
> > * we have it. So the multi needs to remain in place :-(
> > */
> > ResetMultiHintBit(tuple, buffer, xmax, true);
> > #endif
> >
> > Three things:
> > - HeapTupleSatisfiesUpdate is actually always called exclusively locked ;)
> > - Extending something like LWLockHeldByMe to also return the current
> > lockmode doesn't sound hard
> > - we seem to be using #ifdef NOT_YET for such cases
>
> After spending some time trying to make this work usefully, I observed
> that it's pointless, at least if we apply it only in
> HeapTupleSatisfiesUpdate, because the IS_MULTI bit is going to be
> removed by compute_new_xmax_infomask if the multi is gone. Something
> like this would be useful if we could do it in other places; say when
> we're merely scanning page without the specific intention of modifying
> that particular tuple. Maybe in heap_prune_page, for example. ISTM we
> can see that as an optimization we can add later.

heap_prune_page sounds like a good fit, yes. One other place would be
when following hot chains, but the locking would be far more critical in
that path, so its less likely to be beneficial.
Pushing it off sounds good to me.

> Some of your other observations have been fixed by commits that I have
> pushed to my github tree.

A short repetition of the previous pgbench run of many SELECT FOR
SHARE's:

10s test:
master: 22673 24637 23874 25527
fklocks: 24835 24601 24606 24868

60s test:
master: 32609 33087
fklocks: 33350 33359

Very nice!

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Here's version 24.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera wrote:
> Here's version 24.

Old review emails still contains some items that didn't lead to any
changes. I tried to keep close track of those. To that list I add a
couple of things of my own. Here they are, for those following along.
I welcome suggestions.

- Fix the multixact cache in multixact.c -- see mXactCacheEnt.

- ResetHintBitMulti() was removed; need to remove old XMAX_IS_MULTI
somewhere; perhaps during HOT page pruning?

- EvalPlanQual and ExecLockRows maybe need a different overall locking
strategy. Right now follow_updates=false is used to avoid deadlocks.

- Ensure multixact.c behaves sanely on wraparound.

- Is the case of a a non-key-update followed by a key-update actually handled
when doing a heap_lock_tuple with mode = LockTupleKeyShare and follow_updates
= false? I don't really see how, so it seems to deserve at least a comment.

- if oldestMultiXactId + db is set and then that database is dropped we seem to
have a problem because MultiXactAdvanceOldest won't overwrite those
values. Should probably use SetMultiXactIdLimit directly.

- what stop multixacts only being filled out (i.e RecordNewMultiXact()) *after*
the XLogInsert() *and* after a MultiXactGetCheckptMulti()? Afaics
MultiXactGenLock is not hold in CreateMultiXactId(). If we crash in that
moment we loose the multixact data which now means potential data loss...

- multixact member group data crossing 512 sector boundaries makes me uneasy
(as its 5 bytes). I don't really see a scenario where its dangerous, but
... Does anybody see a problem here?

- there are quite some places that do
multiStopLimit = multiWrapLimit - 100;
if (multiStopLimit < FirstMultiXactId)
multiStopLimit -= FirstMultiXactId;

perhaps MultiXactIdAdvance and MultiXactIdRetreat macros are in order?

- not really padding, MultiXactStatus is 4bytes...
/*
* XXX Note: there's a lot of padding space in MultiXactMember. We could
* find a more compact representation of this Xlog record -- perhaps all the
* status flags in one XLogRecData, then all the xids in another one? Not
* clear that it's worth the trouble though.
*/

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-11-27 12:07:34 -0300, Alvaro Herrera wrote:
> Alvaro Herrera wrote:
> > Here's version 24.
>
> Old review emails still contains some items that didn't lead to any
> changes. I tried to keep close track of those. To that list I add a
> couple of things of my own. Here they are, for those following along.
> I welcome suggestions.
>
> - EvalPlanQual and ExecLockRows maybe need a different overall locking
> strategy. Right now follow_updates=false is used to avoid deadlocks.

I think this really might need some work. Afaics the EPQ code now needs
to actually determine what locklevel needs to be passed to
EvalPlanQualFetch via EvalPlanQual otherwise some of the locking issues
that triggered all this remain. That sucks a bit from a modularity
perspective, but I don't see another way.

Greetings,

Andres

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services