Date: Fri, 18 Jan 2013 17:02:04 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Erik Rijkers <er@xs4all.nl>
Cc: Kevin Grittner <kgrittn@mail.com>, pgsql-hackers@postgresql.org,
	Andres Freund <andres@2ndquadrant.com>
Subject: Re: foreign key locks
Message-ID: <20130118200204.GH4063@alvh.no-ip.org>
References: <20121129152326.69330@gmx.com>
	<20121129160220.GA4942@alvh.no-ip.org>
	<20121129161654.GB4942@alvh.no-ip.org>
	<b1a78c715f9195e6f96546c8e723afd8.squirrel@webmail.xs4all.nl>
	<20121228204847.GD4150@alvh.no-ip.org>
	<20130110210040.GB4110@alvh.no-ip.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="kXdP64Ggrk/fb43R"
Content-Disposition: inline
In-Reply-To: <20130110210040.GB4110@alvh.no-ip.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--kXdP64Ggrk/fb43R
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Alvaro Herrera wrote:
> Here's version 28 of this patch.  The only substantive change here from
> v26 is that I've made GetTupleForTrigger() use either LockTupleExclusive
> or LockTupleNoKeyExclusive, depending on whether the key columns are
> being modified by the update.  This needs to go through EvalPlanQual, so
> that function is now getting the lock mode as a parameter instead of
> hardcoded LockTupleExclusive.  (All other users of GetTupleForTrigger
> still use LockTupleExclusive, so there's no change for anybody other
> than FOR EACH ROW BEFORE UPDATE triggers).
>=20
> At this point, I think I've done all I wanted to do in connection with
> this patch, and I intend to commit.  I think this is a good time to get
> it committed, so that people can play with it more extensively and
> report any breakage I might have caused before we even get into beta.

While trying this out this morning I noticed a bug in the XLog code:
trying to lock the updated version of a tuple when the page that
contains the updated version requires a backup block, would cause this:

PANIC:  invalid xlog record length 0

The reason is that there is an (unknown to me) rule that there must be
some data not associated with a buffer:

	/*
	 * NOTE: We disallow len =3D=3D 0 because it provides a useful bit of extra
	 * error checking in ReadRecord.  This means that all callers of
	 * XLogInsert must supply at least some not-in-a-buffer data. [...]
	 */

This seems pretty strange to me.  And having the rule be spelled out
only in a comment within XLogInsert and not at its top, and not nearby
the XLogRecData struct definition either, seems pretty strange to me.
I wonder how come every PG hacker except me knows this.

For the curious, I attach an isolationtester spec file I used to
reproduce the problem (and verify the fix).

To fix the crash I needed to do this:

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/hea=
pam.c
index 99fced1..9762890 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -4838,7 +4838,7 @@ l4:
 		{
 			xl_heap_lock_updated xlrec;
 			XLogRecPtr	recptr;
-			XLogRecData	rdata;
+			XLogRecData	rdata[2];
 			Page		page =3D BufferGetPage(buf);
=20
 			xlrec.target.node =3D rel->rd_node;
@@ -4846,13 +4846,18 @@ l4:
 			xlrec.xmax =3D new_xmax;
 			xlrec.infobits_set =3D compute_infobits(new_infomask, new_infomask2);
=20
-			rdata.data =3D (char *) &xlrec;
-			rdata.len =3D SizeOfHeapLockUpdated;
-			rdata.buffer =3D buf;
-			rdata.buffer_std =3D true;
-			rdata.next =3D NULL;
+			rdata[0].data =3D (char *) &xlrec;
+			rdata[0].len =3D SizeOfHeapLockUpdated;
+			rdata[0].buffer =3D InvalidBuffer;
+			rdata[0].next =3D &(rdata[1]);
+
+			rdata[1].data =3D NULL;
+			rdata[1].len =3D 0;
+			rdata[1].buffer =3D buf;
+			rdata[1].buffer_std =3D true;
+			rdata[1].next =3D NULL;
=20
-			recptr =3D XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_LOCK_UPDATED, &rdata);
+			recptr =3D XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_LOCK_UPDATED, rdata);
=20
 			PageSetLSN(page, recptr);
 			PageSetTLI(page, ThisTimeLineID);

--=20
=C1lvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--kXdP64Ggrk/fb43R
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="xlog-lock-updated.spec"

setup
{
  CREATE TABLE foo (
	key		serial PRIMARY KEY,
	value	int
  );

  INSERT INTO foo SELECT value FROM generate_series(1, 226) AS value;
}

teardown
{
  DROP TABLE foo;
}

session "s1"
step "s1b"	{ BEGIN ISOLATION LEVEL REPEATABLE READ; }
step "s1s"	{ SELECT * FROM foo LIMIT 0; }	# obtain snapshot
step "s1l"	{ SELECT * FROM foo WHERE key = 1 FOR KEY SHARE; } # obtain lock
step "s1c"	{ COMMIT; }

session "s2"
step "s2b"  { BEGIN; }
step "s2u"	{ UPDATE foo SET value = 2 WHERE key = 1; }
step "s2c"	{ COMMIT; }

session "s3"
step "s3ck" { CHECKPOINT; }

permutation "s1b" "s1s" "s2b" "s2u" "s3ck" "s1l" "s1c" "s2c"

--kXdP64Ggrk/fb43R
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


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

--kXdP64Ggrk/fb43R--