Prev: [HACKERS] ecmascript 5 DATESTYLE
Next: ecmascript 5 DATESTYLE
From: Florian Pflug on 18 May 2010 20:15
On May 17, 2010, at 3:30 , Robert Haas wrote:
> On Sun, May 16, 2010 at 9:07 PM, Florian Pflug <fgp(a)phlo.org> wrote:
>> On May 14, 2010, at 22:54 , Robert Haas wrote:
>>> On Thu, May 13, 2010 at 5:39 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
>>>> Florian Pflug <fgp(a)phlo.org> writes:
>>>>> All in all, I believe that SHARE and UPDATE row-level locks should be
>>>>> changed to cause concurrent UPDATEs to fail with a serialization
>>>>> error.
>>>>
>>>> I don't see an argument for doing that for FOR SHARE locks, and it
>>>> already happens for FOR UPDATE (at least if the row actually gets
>>>> updated). AFAICS this proposal mainly breaks things, in pursuit of
>>>> an unnecessary and probably-impossible-anyway goal of making FK locking
>>>> work with only user-level snapshots.
>>>
>>> After giving this considerable thought and testing the behavior at
>>> some length, I think the OP has it right. One thing I sometimes need
>>> to do is denormalize a copy of a field, e.g.
>>>
>>> <snipped example>
>>
>> I've whipped up a quick and still rather dirty patch that implements the behavior I proposed, at least for the case of conflicts between FOR UPDATE locks and updates. With the patch, any attempt to UPDATE or FOR UPDATE lock a row that has concurrently been FOR UPDATE locked will cause a serialization error. (The same for an actually updated row of course, but that happened before too).
>>
>> While this part of the patch was fairly straight forward, make FOR SHARE conflict too seems to be much harder. The assumption that a lock becomes irrelevant after the transaction(s) that held it completely is built deeply into the multi xact machinery that powers SHARE locks. That machinery therefore assumes that once all members of a multi xact have completed the multi xact is dead also. But my proposal depends on a SERIALIZABLE transaction being able to find if any of the lockers of a row are invisible under it's snapshot - for which it'd need any multi xact containing invisible xids to outlive its snapshot.
>
> Thanks for putting this together. I suggest adding it to the open
> CommitFest - even if we decide to go forward with this, I don't
> imagine anyone is going to be excited about changing it during beta.
>
> https://commitfest.postgresql.org/action/commitfest_view/open


Will do. Thanks for the link.

Here is an updated version that works for SHARE locks too.

(This message mainly serves as a way to link the updated patch to the commit fest entry. Is this how I'm supposed to do that, or am I doing something wrong?)

best regards,
Florian Pflug
From: Florian Pflug on 20 May 2010 22:20
On May 19, 2010, at 2:15 , Florian Pflug wrote:
> On May 17, 2010, at 3:30 , Robert Haas wrote:
>> On Sun, May 16, 2010 at 9:07 PM, Florian Pflug <fgp(a)phlo.org> wrote:
>>> On May 14, 2010, at 22:54 , Robert Haas wrote:
>>>> On Thu, May 13, 2010 at 5:39 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
>>>>> Florian Pflug <fgp(a)phlo.org> writes:
>>>>>> All in all, I believe that SHARE and UPDATE row-level locks should be
>>>>>> changed to cause concurrent UPDATEs to fail with a serialization
>>>>>> error.
>>>>>
>>>>> I don't see an argument for doing that for FOR SHARE locks, and it
>>>>> already happens for FOR UPDATE (at least if the row actually gets
>>>>> updated). AFAICS this proposal mainly breaks things, in pursuit of
>>>>> an unnecessary and probably-impossible-anyway goal of making FK locking
>>>>> work with only user-level snapshots.
>>>>
>>>> After giving this considerable thought and testing the behavior at
>>>> some length, I think the OP has it right. One thing I sometimes need
>>>> to do is denormalize a copy of a field, e.g.
>>>>
>>>> <snipped example>
>>>
>>> I've whipped up a quick and still rather dirty patch that implements the behavior I proposed, at least for the case of conflicts between FOR UPDATE locks and updates. With the patch, any attempt to UPDATE or FOR UPDATE lock a row that has concurrently been FOR UPDATE locked will cause a serialization error. (The same for an actually updated row of course, but that happened before too).
>>>
>>> While this part of the patch was fairly straight forward, make FOR SHARE conflict too seems to be much harder. The assumption that a lock becomes irrelevant after the transaction(s) that held it completely is built deeply into the multi xact machinery that powers SHARE locks. That machinery therefore assumes that once all members of a multi xact have completed the multi xact is dead also. But my proposal depends on a SERIALIZABLE transaction being able to find if any of the lockers of a row are invisible under it's snapshot - for which it'd need any multi xact containing invisible xids to outlive its snapshot.
>>
>> Thanks for putting this together. I suggest adding it to the open
>> CommitFest - even if we decide to go forward with this, I don't
>> imagine anyone is going to be excited about changing it during beta.
>>
>> https://commitfest.postgresql.org/action/commitfest_view/open
>
>
> Will do. Thanks for the link.
>
> Here is an updated version that works for SHARE locks too.

Forgetting to run "make check" before sending a patch is bad, as I just proved :-(

For the archives' and the commitfest app's sake, here is a version that actually passes the regression tests.

To make up for it, I also did some testing with a custom pgbench script & schema and proved the effectiveness of this patch. I ran this with "pgbench -s 10 -j 10 -c 10 -t 1000 -n -f fkbench.pgbench" on both HEAD and HEAD+patch. The former errors out quickly with "database inconsistent" while the later completes the pgbench run without errors.

The patch still needs more work, at least on the comments & documentation side of things, but I'm going to let this rest now while we're in beta.

Patch, pgbench script and schema attached.
From: Florian Pflug on 21 May 2010 07:40
On May 21, 2010, at 4:20 , Florian Pflug wrote:
> On May 19, 2010, at 2:15 , Florian Pflug wrote:
>> On May 17, 2010, at 3:30 , Robert Haas wrote:
>>> On Sun, May 16, 2010 at 9:07 PM, Florian Pflug <fgp(a)phlo.org> wrote:
>>>> On May 14, 2010, at 22:54 , Robert Haas wrote:
>>>>> On Thu, May 13, 2010 at 5:39 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
>>>>>> Florian Pflug <fgp(a)phlo.org> writes:
>>>>>>> All in all, I believe that SHARE and UPDATE row-level locks should be
>>>>>>> changed to cause concurrent UPDATEs to fail with a serialization
>>>>>>> error.
>>>>>>
>>>>>> I don't see an argument for doing that for FOR SHARE locks, and it
>>>>>> already happens for FOR UPDATE (at least if the row actually gets
>>>>>> updated). AFAICS this proposal mainly breaks things, in pursuit of
>>>>>> an unnecessary and probably-impossible-anyway goal of making FK locking
>>>>>> work with only user-level snapshots.
>>>>>
>>>>> After giving this considerable thought and testing the behavior at
>>>>> some length, I think the OP has it right. One thing I sometimes need
>>>>> to do is denormalize a copy of a field, e.g.
>>>>>
>>>>> <snipped example>
>>>>
>>>> I've whipped up a quick and still rather dirty patch that implements the behavior I proposed, at least for the case of conflicts between FOR UPDATE locks and updates. With the patch, any attempt to UPDATE or FOR UPDATE lock a row that has concurrently been FOR UPDATE locked will cause a serialization error. (The same for an actually updated row of course, but that happened before too).
>>>>
>>>> While this part of the patch was fairly straight forward, make FOR SHARE conflict too seems to be much harder. The assumption that a lock becomes irrelevant after the transaction(s) that held it completely is built deeply into the multi xact machinery that powers SHARE locks. That machinery therefore assumes that once all members of a multi xact have completed the multi xact is dead also. But my proposal depends on a SERIALIZABLE transaction being able to find if any of the lockers of a row are invisible under it's snapshot - for which it'd need any multi xact containing invisible xids to outlive its snapshot.
>>>
>>> Thanks for putting this together. I suggest adding it to the open
>>> CommitFest - even if we decide to go forward with this, I don't
>>> imagine anyone is going to be excited about changing it during beta.
>>>
>>> https://commitfest.postgresql.org/action/commitfest_view/open
>>
>>
>> Will do. Thanks for the link.
>>
>> Here is an updated version that works for SHARE locks too.
>
> Forgetting to run "make check" before sending a patch is bad, as I just proved :-(
>
> For the archives' and the commitfest app's sake, here is a version that actually passes the regression tests.
>
> To make up for it, I also did some testing with a custom pgbench script & schema and proved the effectiveness of this patch. I ran this with "pgbench -s 10 -j 10 -c 10 -t 1000 -n -f fkbench.pgbench" on both HEAD and HEAD+patch. The former errors out quickly with "database inconsistent" while the later completes the pgbench run without errors.
>
> The patch still needs more work, at least on the comments & documentation side of things, but I'm going to let this rest now while we're in beta.
>
> Patch, pgbench script and schema attached.

Great, now my mail client decided to send encode those attachments with MacBinary instead of sending them as plain text :-(

Not sure if MUAs other than Mail.app can open those, so I'm resending this. Really sorry for the noise, guys

best regards,
Florian Pflug

 | 
Pages: 1
Prev: [HACKERS] ecmascript 5 DATESTYLE
Next: ecmascript 5 DATESTYLE