foreign key locks

On Thu, Jun 14, 2012 at 11:41 AM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:

Hi,

This is v12 of the foreign key locks patch.

Hi Álvaro,

Just noticed that this patch needs a rebase because of the refactoring
Tom did in ri_triggers.c

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

Jaime Casanova <jaime@2ndquadrant.com> writes:

On Thu, Jun 14, 2012 at 11:41 AM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:

This is v12 of the foreign key locks patch.

Just noticed that this patch needs a rebase because of the refactoring
Tom did in ri_triggers.c

Hold on a bit before you work on that code --- I've got one more bit of
hacking I want to try before walking away from it. I did some oprofile
work on Dean's example from
<CAEZATCWm8M00RA814o4DC2cD_aj44gQLb0tDdxMHA312qg7HCQ@mail.gmail.com>
and noticed that it looks like ri_FetchConstraintInfo is eating a
noticeable fraction of the runtime, which is happening because it is
called to deconstruct the relevant pg_constraint row for each tuple
we consider firing the trigger for (and then again, if we do fire the
trigger). I'm thinking it'd be worth maintaining a backend-local cache
for the deconstructed data, and am going to go try that.

regards, tom lane

Excerpts from Tom Lane's message of mié jun 20 12:54:24 -0400 2012:

Jaime Casanova <jaime@2ndquadrant.com> writes:

On Thu, Jun 14, 2012 at 11:41 AM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:

This is v12 of the foreign key locks patch.

Just noticed that this patch needs a rebase because of the refactoring
Tom did in ri_triggers.c

Hold on a bit before you work on that code --- I've got one more bit of
hacking I want to try before walking away from it. I did some oprofile
work on Dean's example from
<CAEZATCWm8M00RA814o4DC2cD_aj44gQLb0tDdxMHA312qg7HCQ@mail.gmail.com>
and noticed that it looks like ri_FetchConstraintInfo is eating a
noticeable fraction of the runtime, which is happening because it is
called to deconstruct the relevant pg_constraint row for each tuple
we consider firing the trigger for (and then again, if we do fire the
trigger). I'm thinking it'd be worth maintaining a backend-local cache
for the deconstructed data, and am going to go try that.

I had merged already when I got your email, but merging that additional
change did not cause any conflicts. So here's the rebased version.

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

Alvaro Herrera wrote:

So here's the rebased version.

I found a couple problems on `make check-world`. Attached is a patch
to fix one of them. The other is on pg_upgrade, pasted below.

+ pg_upgrade -d
/home/kevin/pg/master/contrib/pg_upgrade/tmp_check/data.old -D
/home/kevin/pg/master/contrib/pg_upgrade/tmp_check/data -b
/home/kevin/pg/master/contrib/pg_upgrade/tmp_check/install//home/
kevin/pg/master/Debug/bin -B
/home/kevin/pg/master/contrib/pg_upgrade/tmp_check/install//home/
kevin/pg/master/Debug/bin
Performing Consistency Checks
-----------------------------
Checking current, bin, and data directories ok
Checking cluster versions ok
Checking database user is a superuser ok
Checking for prepared transactions ok
Checking for reg* system OID user data types ok
Checking for contrib/isn with bigint-passing mismatch ok
Creating catalog dump ok
Checking for presence of required libraries ok
Checking database user is a superuser ok
Checking for prepared transactions ok

If pg_upgrade fails after this point, you must re-initdb the
new cluster before continuing.

Performing Upgrade
------------------
Analyzing all rows in the new cluster ok
Freezing all rows on the new cluster ok
Deleting files from new pg_clog ok
Copying old pg_clog to new server ok
Setting next transaction ID for new cluster ok
Deleting files from new pg_multixact/offsets ok
Copying old pg_multixact/offsets to new server ok
Deleting files from new pg_multixact/members ok
Copying old pg_multixact/members to new server ok
Setting next multixact ID and offset for new cluster sh:
/home/kevin/pg/master/contrib/pg_upgrade/tmp_check/install//home/
kevin/pg/master/Debug/bin: Permission denied
*failure*

Consult the last few lines of ""%s/pg_resetxlog" -O %u -m %u,%u "%s"

/dev/null" for the probable cause of the failure.

Failure, exiting
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

I'm marking it as "Waiting for Author" since this seems like a "must
fix", but I'll keep looking at it,

-Kevin

Excerpts from Kevin Grittner's message of sáb jun 23 18:38:10 -0400 2012:

Alvaro Herrera wrote:

So here's the rebased version.

I found a couple problems on `make check-world`. Attached is a patch
to fix one of them. The other is on pg_upgrade, pasted below.

Ah, I mismerged pg_upgrade. The fix is trivial, AFAICS -- a function
call is missing a log file name to be specified. Strange that the
compiler didn't warn me of a broken printf format specifier. However,
pg_upgrade itself has been broken by an unrelated commit and that fix is
not so trivial, so I'm stuck waiting for that to be fixed. (We really
need automated pg_upgrade testing.)

Thanks for the patch for the other problem, I'll push it out.

I'm marking it as "Waiting for Author" since this seems like a "must
fix", but I'll keep looking at it,

Great, thanks.

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

Excerpts from Alvaro Herrera's message of dom jun 24 23:24:47 -0400 2012:

Excerpts from Kevin Grittner's message of sáb jun 23 18:38:10 -0400 2012:

Alvaro Herrera wrote:

So here's the rebased version.

I found a couple problems on `make check-world`. Attached is a patch
to fix one of them. The other is on pg_upgrade, pasted below.

Ah, I mismerged pg_upgrade. The fix is trivial, AFAICS -- a function
call is missing a log file name to be specified. Strange that the
compiler didn't warn me of a broken printf format specifier. However,
pg_upgrade itself has been broken by an unrelated commit and that fix is
not so trivial, so I'm stuck waiting for that to be fixed. (We really
need automated pg_upgrade testing.)

Well, I ended up pushing the pg_upgrade fix anyway even though I
couldn't test it, because I had already committed it to my tree before
your patch. If it still doesn't work after the other guys fix the
outstanding problem I'll just push a new patch.

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

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

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@commandprompt.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@commandprompt.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@commandprompt.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@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Updated patch attached.

--
Álvaro Herrera <alvherre@commandprompt.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@2ndquadrant.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