Thread: CommitFest wrap-up
We're now just a day or two from the end of this CommitFest and there are still a LOT of open patches - to be specific, 23.Here's a brief synopsis of where we are with the others, all IMO of course. - fix for seg picksplit function - I don't have confidence this change is for the best and can't take responsibility for it. It needs review by a committer who understands this stuff better than me and can determine whether or not the change is really an improvement. - unlogged tables - This is not going to get fully resolved in the next two days. I'll move it to the next CF and keep plugging away at it. - instrument checkpoint sync calls - I plan to commit this in the next 48 hours. (Hopefully Greg Smith will do the cleanup I suggested, if not I'll do it.) - explain analyze getrusage tracking - It seems clear to mark this as Returned with Feedback. - synchronous replication - and... - synchronous replication, transaction-controlled - If we want to get this feature into 9.1, we had better get a move on. But I don't currently have it in my time budget to deal with this. - serializable lock consistency - I am fairly certain this needs rebasing. I don't have time to deal with it right away. That sucks, because I think this is a really important change. - MERGE command - Returned with Feedback? Not sure where we stand with this. - Add primary key using an existing index - Returned with Feedback unless a committer is available immediately to pick this up and finish it off. - SQL/MED - core functionality - Seems clear to move this to the next CF and keep working on it. - Idle in transaction cancellation V3 - I think this is waiting on further review. Can anyone work on this one RSN? - Writeable CTEs - I think we need Tom to pick this one up. - Per-column collation - Bump to next CF, unless Peter is planning to commit imminently. - Tab completion in psql for triggers on views - Added to CF late, suggest we bump it to the next CF where it will have a leg up by virtue of already being marked Ready for Committer. - SQL/MED - file_fdw - Discussion is ongoing, but I see no harm in marking this Returned with Feedback for now in anticipation of a new version before CF 2011-01. - SQL/MED - postgresql_fdw - Hasn't received as much review, I think, so should probably be moved to next CF as-is. - Label switcher function (trusted procedure) - I plan to commit this with whatever changes are needed within the next 48 hours. - Extensions - Still under active discussion, suggested we move to next CF. - Fix snapshot taking inconsistencies - Ready for committer. Can any committer pick this up? - Crash dump handler for Windows. Magnus? - Directory archive format for pg_dump. Heikki? - WIP patch for parallel dump. Returned with Feedback? All in all it's disappointing to have so many major patches outstanding at this point in the CommitFest, but I think we're just going to have to make the best of it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 12/13/2010 12:37 PM, Robert Haas wrote: > - SQL/MED - core functionality - Seems clear to move this to the next > CF and keep working on it. [...] > - SQL/MED - file_fdw - Discussion is ongoing, but I see no harm in > marking this Returned with Feedback for now in anticipation of a new > version before CF 2011-01. > - SQL/MED - postgresql_fdw - Hasn't received as much review, I think, > so should probably be moved to next CF as-is. > Don't we need the core patch before the FDW patches? I hope that the core patch is completed and committed ASAP so we have a chance to get the FDW patches in. cheers andrew
On Mon, Dec 13, 2010 at 12:55 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > > > On 12/13/2010 12:37 PM, Robert Haas wrote: >> >> - SQL/MED - core functionality - Seems clear to move this to the next >> CF and keep working on it. > > [...] >> >> - SQL/MED - file_fdw - Discussion is ongoing, but I see no harm in >> marking this Returned with Feedback for now in anticipation of a new >> version before CF 2011-01. >> - SQL/MED - postgresql_fdw - Hasn't received as much review, I think, >> so should probably be moved to next CF as-is. >> > > Don't we need the core patch before the FDW patches? I hope that the core > patch is completed and committed ASAP so we have a chance to get the FDW > patches in. The core patch will certainly need to be committed first. But I doubt that's going to happen in the next few days. If we get it in before the next CF starts, I think we'll be doing very well. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Dec 13, 2010 at 12:37:52PM -0500, Robert Haas wrote: > We're now just a day or two from the end of this CommitFest and there > are still a LOT of open patches - to be specific, 23.Here's a brief > synopsis of where we are with the others, all IMO of course. [snip] > - Writeable CTEs - I think we need Tom to pick this one up. What is it about this that's so complex? It's not like it could impinge on previous functionality, at least at the SQL level. > - Tab completion in psql for triggers on views - Added to CF late, > suggest we bump it to the next CF where it will have a leg up by > virtue of already being marked Ready for Committer. People are, by the way, allowed to commit patches outside of CFs. I had submitted it imagining that its triviality would allow this, which is why it got into the CF late in the first place. [etc., etc., etc.] > All in all it's disappointing to have so many major patches > outstanding at this point in the CommitFest, but I think we're just > going to have to make the best of it. I'm thinking that given all these givens, we should make the best of it with more CF in March. We don't want a repeat of the "last-minute giant change" anti-pattern, and even if we're releasing 9.1 in July, three months plus is plenty of time to shake things out. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Mon, Dec 13, 2010 at 2:19 PM, David Fetter <david@fetter.org> wrote: > On Mon, Dec 13, 2010 at 12:37:52PM -0500, Robert Haas wrote: >> We're now just a day or two from the end of this CommitFest and there >> are still a LOT of open patches - to be specific, 23.Here's a brief >> synopsis of where we are with the others, all IMO of course. > > [snip] >> - Writeable CTEs - I think we need Tom to pick this one up. > > What is it about this that's so complex? It's not like it could > impinge on previous functionality, at least at the SQL level. I didn't say it was complex, although it is. I said I think we need Tom to pick it up. That's partly because he's likely to have overpoweringly strong opinions on how it should work, which may make it unproductive for someone else to spend time on it. Also, it is complex, and regardless of what effects it has on anything else, it does need to work. Tom is good at that. >> - Tab completion in psql for triggers on views - Added to CF late, >> suggest we bump it to the next CF where it will have a leg up by >> virtue of already being marked Ready for Committer. > > People are, by the way, allowed to commit patches outside of CFs. I > had submitted it imagining that its triviality would allow this, which > is why it got into the CF late in the first place. You can insist all you like that your favorite patches are trivial, but that doesn't make it so. I am well aware that patches can be committed between CommitFests. For example: git log --author=Haas --since=2010-10-16 --before=2010-11-14 >> All in all it's disappointing to have so many major patches >> outstanding at this point in the CommitFest, but I think we're just >> going to have to make the best of it. > > I'm thinking that given all these givens, we should make the best of > it with more CF in March. We don't want a repeat of the "last-minute > giant change" anti-pattern, and even if we're releasing 9.1 in July, > three months plus is plenty of time to shake things out. -1. That's as likely to make the back-up of big patches worse as it is to make it better. Maybe more likely. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > We're now just a day or two from the end of this CommitFest and there > are still a LOT of open patches - to be specific, 23.Here's a brief > synopsis of where we are with the others, all IMO of course. Thanks for doing this! > - Extensions - Still under active discussion, suggested we move to > next CF. Well, it might be confusing to follow those threads at about any distance but in fact, the only active one left is about some details concerning the ALTER EXTENSION UPGRADE command, which is *not* to be in this commit fest but (hopefully) the next. I think the main extension patch is about as ready as it can be now, I've only been fixing nitpicks and interfacing for awhile (all along this commit fest) and the underlying code has been very stable. As we want to avoid pushing "big" patches into the last commit fest, I'd very like it if we could have a last round of review-then-commit on this patch. Of course it's still possible to work on it in between two commit fests, but that's not a good idea: this is a very restrained time when the more involved people here can work on their own ideas. So, who's in to finish up and commit this patch in this round? :) I certainly am ready to support last minute changes, given some are required. And if they are too big for the schedule, better shake the patch out now rather than let it bitrot another month. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Dec 13, 2010, at 12:04 PM, Dimitri Fontaine wrote: > So, who's in to finish up and commit this patch in this round? :) > I certainly am ready to support last minute changes, given some are > required. And if they are too big for the schedule, better shake the > patch out now rather than let it bitrot another month. I'll try to do another review this week. David
"David E. Wheeler" <david@kineticode.com> writes: > On Dec 13, 2010, at 12:04 PM, Dimitri Fontaine wrote: >> So, who's in to finish up and commit this patch in this round? :) > > I'll try to do another review this week. Thanks! -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On 12/13/10 9:37 AM, Robert Haas wrote:
> - synchronous replication - and...
> - synchronous replication, transaction-controlled - If we want to get
> this feature into 9.1, we had better get a move on. But I don't
> currently have it in my time budget to deal with this.
I thought we'd covered most of the major issues here. What's holding
these up?
-- -- Josh Berkus PostgreSQL Experts Inc.
http://www.pgexperts.com
Robert Haas wrote: > - instrument checkpoint sync calls - I plan to commit this in the next > 48 hours. (Hopefully Greg Smith will do the cleanup I suggested, if > not I'll do it.) > Yes, doing that tonight so you can have a simple (hopefully) bit of work to commit tomorrow. > - MERGE command - Returned with Feedback? Not sure where we stand with this. > That's waiting for me to do another round of review. I'm getting to that soon I hope, maybe tomorrow. > - SQL/MED - file_fdw - Discussion is ongoing, but I see no harm in > marking this Returned with Feedback for now in anticipation of a new > version before CF 2011-01. > - SQL/MED - postgresql_fdw - Hasn't received as much review, I think, > so should probably be moved to next CF as-is. > I was thinking of just moving both of those into the next CF without adding any clear resolution code--then they can get worked on as feasible after the core goes in. > All in all it's disappointing to have so many major patches > outstanding at this point in the CommitFest, but I think we're just > going to have to make the best of it. > I've done a pretty lame job of pushings thing forward here, but I don't think things have progressed that badly. The community produced several large and/or multi-part patches that the CF could have choked on, and instead they've been broken into digestible chunks and kept chewing through them. I'm just glad that's happening in this CF, rather than a pile like this showing up for the last one. Thanks for the wrap-up summary, I was going to do something like that myself tonight but you beat me to it. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services and Support www.2ndQuadrant.us "PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books
On Mon, Dec 13, 2010 at 4:52 PM, Josh Berkus <josh@agliodbs.com> wrote: > On 12/13/10 9:37 AM, Robert Haas wrote: >> - synchronous replication - and... >> - synchronous replication, transaction-controlled - If we want to get >> this feature into 9.1, we had better get a move on. But I don't >> currently have it in my time budget to deal with this. > > I thought we'd covered most of the major issues here. What's holding > these up? I don't know where you got that idea. We have two competing patches neither of which has been updated in several months. And neither of which has gotten a whole lot of review, either. We spent a lot of time arguing about basic design and then everyone got tired and went on doing other things. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Dec13, 2010, at 18:37 , Robert Haas wrote: > We're now just a day or two from the end of this CommitFest and there > are still a LOT of open patches - to be specific, 23.Here's a brief > synopsis of where we are with the others, all IMO of course. Thanks for putting this together! > - serializable lock consistency - I am fairly certain this needs > rebasing. I don't have time to deal with it right away. That sucks, > because I think this is a really important change. I can try to find some time to update the patch if it suffers from bit-rot. Would that help? best regards, Florian Pflug
On Tue, Dec 14, 2010 at 7:51 AM, Florian Pflug <fgp@phlo.org> wrote: >> - serializable lock consistency - I am fairly certain this needs >> rebasing. I don't have time to deal with it right away. That sucks, >> because I think this is a really important change. > I can try to find some time to update the patch if it suffers from bit-rot. Would that help? Yes! -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Dec 13, 2010 at 12:37 PM, Robert Haas <robertmhaas@gmail.com> wrote: > - fix for seg picksplit function - I don't have confidence this change > is for the best and can't take responsibility for it. It needs review > by a committer who understands this stuff better than me and can > determine whether or not the change is really an improvement. Still outstanding. > - unlogged tables - This is not going to get fully resolved in the > next two days. I'll move it to the next CF and keep plugging away at > it. Moved. > - instrument checkpoint sync calls - I plan to commit this in the next > 48 hours. (Hopefully Greg Smith will do the cleanup I suggested, if > not I'll do it.) Committed. > - explain analyze getrusage tracking - It seems clear to mark this as > Returned with Feedback. Marked return with Feedback. > - synchronous replication - and... > - synchronous replication, transaction-controlled - If we want to get > this feature into 9.1, we had better get a move on. But I don't > currently have it in my time budget to deal with this. Neither patch applies, and Simon's was also labelled WIP. Marked both Returned with Feedback. > - serializable lock consistency - I am fairly certain this needs > rebasing. I don't have time to deal with it right away. That sucks, > because I think this is a really important change. > - MERGE command - Returned with Feedback? Not sure where we stand with this. Still outstanding. > - Add primary key using an existing index - Returned with Feedback > unless a committer is available immediately to pick this up and finish > it off. Returned with Feedback. Looks like author is planning to rework this one. > - SQL/MED - core functionality - Seems clear to move this to the next > CF and keep working on it. Moved. > - Idle in transaction cancellation V3 - I think this is waiting on > further review. Can anyone work on this one RSN? Reviewed, but no doubt there's more work left to be done here than is going to happen in this CF. > - Writeable CTEs - I think we need Tom to pick this one up. > - Per-column collation - Bump to next CF, unless Peter is planning to > commit imminently. Both still outstanding. I suppose these will have to be moved to the next CommitFest. > - Tab completion in psql for triggers on views - Added to CF late, > suggest we bump it to the next CF where it will have a leg up by > virtue of already being marked Ready for Committer. Partially committed. I'll see if I can find time to commit the rest; otherwise, I'll move it along to the next CF. > - SQL/MED - file_fdw - Discussion is ongoing, but I see no harm in > marking this Returned with Feedback for now in anticipation of a new > version before CF 2011-01. > - SQL/MED - postgresql_fdw - Hasn't received as much review, I think, > so should probably be moved to next CF as-is. Per recommendation from Greg Smith, moved to next CF. > - Label switcher function (trusted procedure) - I plan to commit this > with whatever changes are needed within the next 48 hours. Committed. > - Extensions - Still under active discussion, suggested we move to next CF. > - Fix snapshot taking inconsistencies - Ready for committer. Can any > committer pick this up? > - Crash dump handler for Windows. Magnus? > - Directory archive format for pg_dump. Heikki? These are all still outstanding. > - WIP patch for parallel dump. Returned with Feedback? Returned with Feedback, with some reluctance, but nothing else seems reasonable. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Dec14, 2010, at 15:01 , Robert Haas wrote: > On Tue, Dec 14, 2010 at 7:51 AM, Florian Pflug <fgp@phlo.org> wrote: >>> - serializable lock consistency - I am fairly certain this needs >>> rebasing. I don't have time to deal with it right away. That sucks, >>> because I think this is a really important change. >> I can try to find some time to update the patch if it suffers from bit-rot. Would that help? > > Yes! I've rebased the patch to the current HEAD, and re-run my FK concurrency test suite, available from https://github.com/fgp/fk_concurrency, to verify that things still work. I've also asserts to the callers of heap_{update,delete,lock_tuple} to verify (and document) that update_xmax may only be InvalidTransactionId if a lockcheck_snapshot is passed to heap_{update,delete,lock_tuple}. Finally, I've improved the explanation in src/backend/executor/README of how row locks and REPEATABLE READ transactions interact, and tried to state the guarantees provided by FOR SHARE and FOR UPDATE locks more precisely. I've published my work to https://github.com/fgp/postgres/tree/serializable_lock_consistency, and attached an updated patch. I'd be happy to give write access to that GIT repository to anyone who wants to help getting this committed. best regards, Florian Pflug
Attachment
On Wed, Dec 15, 2010 at 9:20 AM, Florian Pflug <fgp@phlo.org> wrote: > On Dec14, 2010, at 15:01 , Robert Haas wrote: >> On Tue, Dec 14, 2010 at 7:51 AM, Florian Pflug <fgp@phlo.org> wrote: >>>> - serializable lock consistency - I am fairly certain this needs >>>> rebasing. I don't have time to deal with it right away. That sucks, >>>> because I think this is a really important change. >>> I can try to find some time to update the patch if it suffers from bit-rot. Would that help? >> >> Yes! > > I've rebased the patch to the current HEAD, and re-run my FK concurrency test suite, > available from https://github.com/fgp/fk_concurrency, to verify that things still work. Thanks, but, EWRONGTHREAD. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Dec15, 2010, at 16:45 , Robert Haas wrote: > On Wed, Dec 15, 2010 at 9:20 AM, Florian Pflug <fgp@phlo.org> wrote: >> On Dec14, 2010, at 15:01 , Robert Haas wrote: >>> On Tue, Dec 14, 2010 at 7:51 AM, Florian Pflug <fgp@phlo.org> wrote: >>>>> - serializable lock consistency - I am fairly certain this needs >>>>> rebasing. I don't have time to deal with it right away. That sucks, >>>>> because I think this is a really important change. >>>> I can try to find some time to update the patch if it suffers from bit-rot. Would that help? >>> >>> Yes! >> >> I've rebased the patch to the current HEAD, and re-run my FK concurrency test suite, >> available from https://github.com/fgp/fk_concurrency, to verify that things still work. > > Thanks, but, EWRONGTHREAD. Sorry for that. I wasn't sure whether to post this here or into the original thread, and it seems I ended up on the losing side of that 50-50 chance ;-) Want me to repost there, or just remember to use the correct thread next time? best regards, Florian Pflug
On Wed, Dec 15, 2010 at 10:57 AM, Florian Pflug <fgp@phlo.org> wrote: > On Dec15, 2010, at 16:45 , Robert Haas wrote: >> On Wed, Dec 15, 2010 at 9:20 AM, Florian Pflug <fgp@phlo.org> wrote: >>> On Dec14, 2010, at 15:01 , Robert Haas wrote: >>>> On Tue, Dec 14, 2010 at 7:51 AM, Florian Pflug <fgp@phlo.org> wrote: >>>>>> - serializable lock consistency - I am fairly certain this needs >>>>>> rebasing. I don't have time to deal with it right away. That sucks, >>>>>> because I think this is a really important change. >>>>> I can try to find some time to update the patch if it suffers from bit-rot. Would that help? >>>> >>>> Yes! >>> >>> I've rebased the patch to the current HEAD, and re-run my FK concurrency test suite, >>> available from https://github.com/fgp/fk_concurrency, to verify that things still work. >> >> Thanks, but, EWRONGTHREAD. > > Sorry for that. I wasn't sure whether to post this here or into the original thread, > and it seems I ended up on the losing side of that 50-50 chance ;-) > > Want me to repost there, or just remember to use the correct thread next time? Nah, don't bother reposting. It'd be helpful if you could add a link to that message on the CF app though. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Dec15, 2010, at 17:17 , Robert Haas wrote: > Nah, don't bother reposting. It'd be helpful if you could add a link > to that message on the CF app though. Already done. Seems we've hit a race condition there - you must have overlooked the signalling the semaphore on my rooftop did to warn you... best regards, Florian Pflug
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Dec 13, 2010 at 12:37 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> - fix for seg picksplit function - I don't have confidence this change
>> is for the best and can't take responsibility for it. It needs review
>> by a committer who understands this stuff better than me and can
>> determine whether or not the change is really an improvement.
> Still outstanding.
I will take a look at that one --- it is a bug fix at bottom, so we
can't just drop it for lack of reviewers.
>> - Writeable CTEs - I think we need Tom to pick this one up.
>> - Fix snapshot taking inconsistencies - Ready for committer. Can any
>> committer pick this up?
Will take a look at these two also.
regards, tom lane
Tom Lane wrote:<br /><blockquote cite="mid:7242.1292430568@sss.pgh.pa.us" type="cite"><blockquote type="cite"><blockquotetype="cite"><pre wrap="">- Writeable CTEs - I think we need Tom to pick this one up. - Fix snapshot taking inconsistencies - Ready for committer. Can any committer pick this up? </pre></blockquote></blockquote><pre wrap=""> Will take a look at these two also. </pre></blockquote><br /> I marked you down at the listed committer for them both. Thatleaves "serializable lock consistency" as the only patch that's left in "Ready for Committer" state; everything elseseemed to me to still have some kinks left to work out and I marked them returned. Given that patch has been in thatstate since 9/24 and Florian even took care of the bit rot yesterday, it's certainly been queued patiently enough waitingfor attention. Obviously you, Robert, and other committers can work on one of the patches I bounced instead if youthink they're close enough to slip in. But this serializable lock one is the only submission that I think hasn't gottena fair share of attention yet.<br /><br /><pre class="moz-signature" cols="72">-- Greg Smith 2ndQuadrant US <a class="moz-txt-link-abbreviated" href="mailto:greg@2ndQuadrant.com">greg@2ndQuadrant.com</a> Baltimore, MD PostgreSQL Training, Services and Support <a class="moz-txt-link-abbreviated" href="http://www.2ndQuadrant.us">www.2ndQuadrant.us</a> "PostgreSQL 9.0 High Performance": <a class="moz-txt-link-freetext" href="http://www.2ndQuadrant.com/books">http://www.2ndQuadrant.com/books</a> </pre>
On 15.12.2010 16:20, Florian Pflug wrote: > On Dec14, 2010, at 15:01 , Robert Haas wrote: >> On Tue, Dec 14, 2010 at 7:51 AM, Florian Pflug<fgp@phlo.org> wrote: >>>> - serializable lock consistency - I am fairly certain this needs >>>> rebasing. I don't have time to deal with it right away. That sucks, >>>> because I think this is a really important change. >>> I can try to find some time to update the patch if it suffers from bit-rot. Would that help? >> >> Yes! > > I've rebased the patch to the current HEAD, and re-run my FK concurrency test suite, > available from https://github.com/fgp/fk_concurrency, to verify that things still work. > > I've also asserts to the callers of heap_{update,delete,lock_tuple} to verify (and document) > that update_xmax may only be InvalidTransactionId if a lockcheck_snapshot is passed to > heap_{update,delete,lock_tuple}. > > Finally, I've improved the explanation in src/backend/executor/README of how row locks and > REPEATABLE READ transactions interact, and tried to state the guarantees provided by > FOR SHARE and FOR UPDATE locks more precisely. > > I've published my work to https://github.com/fgp/postgres/tree/serializable_lock_consistency, > and attached an updated patch. I'd be happy to give write access to that GIT repository > to anyone who wants to help getting this committed. Here's some typo & style fixes for that, also available at git://git.postgresql.org/git/users/heikki/postgres.git. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Attachment
On Dec17, 2010, at 16:49 , Heikki Linnakangas wrote: > On 15.12.2010 16:20, Florian Pflug wrote: >> On Dec14, 2010, at 15:01 , Robert Haas wrote: >>> On Tue, Dec 14, 2010 at 7:51 AM, Florian Pflug<fgp@phlo.org> wrote: >>>>> - serializable lock consistency - I am fairly certain this needs >>>>> rebasing. I don't have time to deal with it right away. That sucks, >>>>> because I think this is a really important change. >>>> I can try to find some time to update the patch if it suffers from bit-rot. Would that help? >>> >>> Yes! >> >> I've rebased the patch to the current HEAD, and re-run my FK concurrency test suite, >> available from https://github.com/fgp/fk_concurrency, to verify that things still work. >> >> I've also asserts to the callers of heap_{update,delete,lock_tuple} to verify (and document) >> that update_xmax may only be InvalidTransactionId if a lockcheck_snapshot is passed to >> heap_{update,delete,lock_tuple}. >> >> Finally, I've improved the explanation in src/backend/executor/README of how row locks and >> REPEATABLE READ transactions interact, and tried to state the guarantees provided by >> FOR SHARE and FOR UPDATE locks more precisely. >> >> I've published my work to https://github.com/fgp/postgres/tree/serializable_lock_consistency, >> and attached an updated patch. I'd be happy to give write access to that GIT repository >> to anyone who wants to help getting this committed. > > Here's some typo & style fixes for that, also available at git://git.postgresql.org/git/users/heikki/postgres.git. Thanks! FYI, I've pulled these into https://github.com/fgp/postgres/tree/serializable_lock_consistency best regards, Florian Pflug
On 17.12.2010 18:44, Florian Pflug wrote: > On Dec17, 2010, at 16:49 , Heikki Linnakangas wrote: >> On 15.12.2010 16:20, Florian Pflug wrote: >>> On Dec14, 2010, at 15:01 , Robert Haas wrote: >>>> On Tue, Dec 14, 2010 at 7:51 AM, Florian Pflug<fgp@phlo.org> wrote: >>>>>> - serializable lock consistency - I am fairly certain this needs >>>>>> rebasing. I don't have time to deal with it right away. That sucks, >>>>>> because I think this is a really important change. >>>>> I can try to find some time to update the patch if it suffers from bit-rot. Would that help? >>>> >>>> Yes! >>> >>> I've rebased the patch to the current HEAD, and re-run my FK concurrency test suite, >>> available from https://github.com/fgp/fk_concurrency, to verify that things still work. >>> >>> I've also asserts to the callers of heap_{update,delete,lock_tuple} to verify (and document) >>> that update_xmax may only be InvalidTransactionId if a lockcheck_snapshot is passed to >>> heap_{update,delete,lock_tuple}. >>> >>> Finally, I've improved the explanation in src/backend/executor/README of how row locks and >>> REPEATABLE READ transactions interact, and tried to state the guarantees provided by >>> FOR SHARE and FOR UPDATE locks more precisely. >>> >>> I've published my work to https://github.com/fgp/postgres/tree/serializable_lock_consistency, >>> and attached an updated patch. I'd be happy to give write access to that GIT repository >>> to anyone who wants to help getting this committed. >> >> Here's some typo& style fixes for that, also available at git://git.postgresql.org/git/users/heikki/postgres.git. > > Thanks! FYI, I've pulled these into https://github.com/fgp/postgres/tree/serializable_lock_consistency I think this patch is in pretty good shape now. The one thing I'm not too happy with is the API for heap_update/delete/lock_tuple. The return value is: * Normal, successful return value is HeapTupleMayBeUpdated, which * actually means we *did* update it. Failure return codesare * HeapTupleSelfUpdated, HeapTupleUpdated, or HeapTupleBeingUpdated * (the last only possible if wait == false). And: * In the failure cases, the routine returns the tuple's t_ctid and t_max * in ctid and update_xmax. * If ctid is the sameas t_self and update_xmax a valid transaction id, * the tuple was deleted. * If ctid differs from t_self, the tuplewas updated, ctid is the location * of the replacement tuple and update_xmax is the updating transaction's xid. * update_xmax must in this case be used to verify that the replacement tuple * matched. * Otherwise, if ctid is the same as t_self and update_xmax is * InvalidTransactionId, the tuple wasneither replaced nor deleted, but * locked by a transaction invisible to lockcheck_snapshot. This case can * thusonly arise if lockcheck_snapshot is a valid snapshot. That's quite complicated. I think we should bite the bullet and add a couple of more return codes to explicitly tell the caller what happened. I propose: HeapTupleMayBeUpdated- the tuple was actually updated (same as before) HeapTupleSelfUpdated - the tuple was updated by a later command in same xact (same as before) HeapTupleBeingUpdated - concurrent update in progress (same as before) HeapTupleUpdated - the tuple was updated by another xact. *update_xmax and *ctid are set to point to the replacement tuple. HeapTupleDeleted - the tuple was deleted by another xact HeapTupleLocked - lockcheck_snapshot was given, and the tuple was locked by another xact I'm not sure how to incorporate that into the current heap_delete/update/lock_tuple functions and HeapTupleSatisfiesUpdate. It would be nice to not copy-paste the logic to handle those into all three functions. Perhaps that common logic starting with the HeapTupleSatisfiesUpdate() call could be pulled into a common function. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Dec19, 2010, at 18:06 , Heikki Linnakangas wrote:
> I think this patch is in pretty good shape now.
Apart from the serious deficiency Robert found :-(
I'll still comment on your suggestions though, since
they'd also apply to the solution I suggested on the
other thread.
> The one thing I'm not too happy with is the API for heap_update/delete/lock_tuple. The return value is:
>
> <snipped comment citation>
>
> That's quite complicated. I think we should bite the bullet and add a couple of more return codes to explicitly tell
thecaller what happened. I propose:
Yeah, it's a bit of a mess. On the other hand, heap_{update,delete,lock_tuple} are only called from very few places
(simple_heap_update,simple_heap_delete, ExecUpdate, ExecLockRows and GetTupleForTrigger). Of these, only ExecUpdate and
ExecLockRowscare for update_xmax and ctid.
> HeapTupleMayBeUpdated- the tuple was actually updated (same as before)
> HeapTupleSelfUpdated - the tuple was updated by a later command in same xact (same as before)
> HeapTupleBeingUpdated - concurrent update in progress (same as before)
> HeapTupleUpdated - the tuple was updated by another xact. *update_xmax and *ctid are set to point to the replacement
tuple.
> HeapTupleDeleted - the tuple was deleted by another xact
> HeapTupleLocked - lockcheck_snapshot was given, and the tuple was locked by another xact
Hm, I'm not happy with HeapTupleMayBeUpdated meaning "The tuple was updated" while HeapTupleUpdated means "The tuple
wasn'tupdated, a concurrent transaction beat us to it" seems less than ideal. On the whole, I'd much rather have a
secondenum, say HO_Result for heap operation result, instead of miss-using HTSU_Result for this. HO_Result would have
thefollowing possible values
HeapOperationCompleted - the tuple was updated/deleted/locked
HeapOperationSelfModified - the tuple was modified by a later command in the same xact. We don't distinguish the
differentcases here since none of the callers care.
HeapOperationBeingModified - the tuple was updated/deleted/locked (and the lock conflicts) by a transaction still
in-progress.
HeapOperationConcurrentUpdate - the tuple was updated concurrently. *update_xmax and *ctid are set to point to the
replacementtuple.
HeapOperationConcurrentDelete - the tuple was deleted concurrently.
HeapOperationConcurrentLock - the tuple was locked concurrently (only if lockcheck_snapshot was provided).
If we do want to keep heap_{update,delete,lock_tuple} result HTSU_Result, we could also add an output parameter of type
HTSU_Failurewith the possible values
HTSUConcurrentUpdate
HTSUConcurrentDelete
HTSUConcurrentLock
and set it accordingly if we return HeapTupleUpdated.
> I'm not sure how to incorporate that into the current heap_delete/update/lock_tuple functions and
HeapTupleSatisfiesUpdate.It would be nice to not copy-paste the logic to handle those into all three functions. Perhaps
thatcommon logic starting with the HeapTupleSatisfiesUpdate() call could be pulled into a common function.
Hm, the logic in heap_lock_tuple is quite different from heap_delete and heap_update, since it needs to deal with
share-modelock acquisition. But for heap_{update,delete} unifying the logic should be possible.
best regards,
Florian Pflug
On Wed, Dec 15, 2010 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> - Writeable CTEs - I think we need Tom to pick this one up. >>> - Fix snapshot taking inconsistencies - Ready for committer. Can any >>> committer pick this up? > > Will take a look at these two also. Tom, what is your time frame on this? I think we should wrap up the CF without these and bundle 9.1alpha3 unless you plan to get to this in the next day or two. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Dec 15, 2010 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> - Writeable CTEs - I think we need Tom to pick this one up.
>>> - Fix snapshot taking inconsistencies - Ready for committer. Can any
>>> committer pick this up?
>> Will take a look at these two also.
> Tom, what is your time frame on this? I think we should wrap up the
> CF without these and bundle 9.1alpha3 unless you plan to get to this
> in the next day or two.
We probably shouldn't hold up the alpha for these, if there are no
other items outstanding.
regards, tom lane
On Tue, Dec 21, 2010 at 11:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, Dec 15, 2010 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> - Writeable CTEs - I think we need Tom to pick this one up. >>>> - Fix snapshot taking inconsistencies - Ready for committer. Can any >>>> committer pick this up? > >>> Will take a look at these two also. > >> Tom, what is your time frame on this? I think we should wrap up the >> CF without these and bundle 9.1alpha3 unless you plan to get to this >> in the next day or two. > > We probably shouldn't hold up the alpha for these, if there are no > other items outstanding. OK. I've moved them to the next CommitFest and marked this one closed. *bangs gavel* -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Dec 21, 2010 at 10:49 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Dec 21, 2010 at 11:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> On Wed, Dec 15, 2010 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>>> - Writeable CTEs - I think we need Tom to pick this one up. >>>>> - Fix snapshot taking inconsistencies - Ready for committer. Can any >>>>> committer pick this up? >> >>>> Will take a look at these two also. >> >>> Tom, what is your time frame on this? I think we should wrap up the >>> CF without these and bundle 9.1alpha3 unless you plan to get to this >>> in the next day or two. >> >> We probably shouldn't hold up the alpha for these, if there are no >> other items outstanding. > > OK. I've moved them to the next CommitFest and marked this one closed. Tom, are you still planning to pick these two up? They've been basically collecting dust for over two months now, or in one case three months, and we're running out of time. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Dec 21, 2010 at 10:49 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Dec 21, 2010 at 11:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Dec 15, 2010 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> - Writeable CTEs - I think we need Tom to pick this one up.
>>>> - Fix snapshot taking inconsistencies - Ready for committer. Can any
>>>> committer pick this up?
> Tom, are you still planning to pick these two up? They've been
> basically collecting dust for over two months now, or in one case
> three months, and we're running out of time.
Yes, I will get to them. I haven't yet put my head down into full
commit fest mode...
regards, tom lane