From: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
---|---|
To: | Erik Rijkers <er(at)xs4all(dot)nl> |
Cc: | Kevin Grittner <kgrittn(at)mail(dot)com>, pgsql-hackers(at)postgresql(dot)org, Andres Freund <andres(at)2ndquadrant(dot)com> |
Subject: | Re: foreign key locks |
Date: | 2013-01-18 20:02:04 |
Message-ID: | 20130118200204.GH4063@alvh.no-ip.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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).
>
> 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 == 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/heapam.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 = BufferGetPage(buf);
xlrec.target.node = rel->rd_node;
@@ -4846,13 +4846,18 @@ l4:
xlrec.xmax = new_xmax;
xlrec.infobits_set = compute_infobits(new_infomask, new_infomask2);
- rdata.data = (char *) &xlrec;
- rdata.len = SizeOfHeapLockUpdated;
- rdata.buffer = buf;
- rdata.buffer_std = true;
- rdata.next = NULL;
+ rdata[0].data = (char *) &xlrec;
+ rdata[0].len = SizeOfHeapLockUpdated;
+ rdata[0].buffer = InvalidBuffer;
+ rdata[0].next = &(rdata[1]);
+
+ rdata[1].data = NULL;
+ rdata[1].len = 0;
+ rdata[1].buffer = buf;
+ rdata[1].buffer_std = true;
+ rdata[1].next = NULL;
- recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_LOCK_UPDATED, &rdata);
+ recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_LOCK_UPDATED, rdata);
PageSetLSN(page, recptr);
PageSetTLI(page, ThisTimeLineID);
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachment | Content-Type | Size |
---|---|---|
xlog-lock-updated.spec | text/plain | 622 bytes |
From | Date | Subject | |
---|---|---|---|
Next Message | Boszormenyi Zoltan | 2013-01-18 20:08:30 | Re: Re: Proposal for Allow postgresql.conf values to be changed via SQL [review] |
Previous Message | Tom Lane | 2013-01-18 19:49:59 | Re: could not create directory "...": File exists |