Re: 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(at)dunslane(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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(at)fetter(dot)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(at)gmail(dot)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(at)kineticode(dot)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(at)2ndQuadrant(dot)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(at)agliodbs(dot)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(at)phlo(dot)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(at)gmail(dot)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(at)phlo(dot)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

On Wed, Dec 15, 2010 at 9:20 AM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
> On Dec14, 2010, at 15:01 , Robert Haas wrote:
>> On Tue, Dec 14, 2010 at 7:51 AM, Florian Pflug <fgp(at)phlo(dot)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(at)phlo(dot)org> wrote:
>> On Dec14, 2010, at 15:01 , Robert Haas wrote:
>>> On Tue, Dec 14, 2010 at 7:51 AM, Florian Pflug <fgp(at)phlo(dot)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(at)phlo(dot)org> wrote:
> On Dec15, 2010, at 16:45 , Robert Haas wrote:
>> On Wed, Dec 15, 2010 at 9:20 AM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
>>> On Dec14, 2010, at 15:01 , Robert Haas wrote:
>>>> On Tue, Dec 14, 2010 at 7:51 AM, Florian Pflug <fgp(at)phlo(dot)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(at)gmail(dot)com> writes:
> On Mon, Dec 13, 2010 at 12:37 PM, Robert Haas <robertmhaas(at)gmail(dot)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:
>>> - 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.
>

I marked you down at the listed committer for them both. That leaves
"serializable lock consistency" as the only patch that's left in "Ready
for Committer" state; everything else seemed to me to still have some
kinks left to work out and I marked them returned. Given that patch has
been in that state since 9/24 and Florian even took care of the bit rot
yesterday, it's certainly been queued patiently enough waiting for
attention. Obviously you, Robert, and other committers can work on one
of the patches I bounced instead if you think they're close enough to
slip in. But this serializable lock one is the only submission that I
think hasn't gotten a fair share of attention yet.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services and Support www.2ndQuadrant.us
"PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books

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(at)phlo(dot)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

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(at)phlo(dot)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(at)phlo(dot)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 codes are
* 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 same as t_self and update_xmax a valid transaction id,
* the tuple was deleted.
* If ctid differs from t_self, the tuple was 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 was neither replaced nor deleted, but
* locked by a transaction invisible to lockcheck_snapshot. This case can
* thus only 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 the caller 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 ExecLockRows care 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't updated, a concurrent transaction beat us to it" seems less than ideal. On the whole, I'd much rather have a second enum, say HO_Result for heap operation result, instead of miss-using HTSU_Result for this. HO_Result would have the following 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 different cases 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 replacement tuple.
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_Failure with 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 that common 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-mode lock 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(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
> On Wed, Dec 15, 2010 at 11:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Dec 15, 2010 at 11:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> wrote:
> On Tue, Dec 21, 2010 at 11:12 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Wed, Dec 15, 2010 at 11:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
> On Tue, Dec 21, 2010 at 10:49 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Tue, Dec 21, 2010 at 11:12 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Dec 15, 2010 at 11:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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