Re: foreign key locks, 2nd attempt

On Thu, Feb 23, 2012 at 12:43:11PM -0300, Alvaro Herrera wrote:
> Excerpts from Simon Riggs's message of jue feb 23 06:18:57 -0300 2012:
> > On Wed, Feb 22, 2012 at 5:00 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> >
> > >> All in all, I think this is in pretty much final shape. ??Only pg_upgrade
> > >> bits are still missing. ??If sharp eyes could give this a critical look
> > >> and knuckle-cracking testers could give it a spin, that would be
> > >> helpful.
> > >
> > > Lack of pg_upgrade support leaves this version incomplete, because that
> > > omission would constitute a blocker for beta 2. ??This version changes as much
> > > code compared to the version I reviewed at the beginning of the CommitFest as
> > > that version changed overall. ??In that light, it's time to close the books on
> > > this patch for the purpose of this CommitFest; I'm marking it Returned with
> > > Feedback. ??Thanks for your efforts thus far.
>
> Now this is an interesting turn of events. I must thank you for your
> extensive review effort in the current version of the patch, and also
> thank you and credit you for the idea that initially kicked this patch
> from the older, smaller, simpler version I wrote during the 9.1 timeline
> (which you also reviewed exhaustively). Without your and Simon's
> brilliant ideas, this patch wouldn't exist at all.
>
> I completely understand that you don't want to review this latest
> version of the patch; it's a lot of effort and I wouldn't inflict it on
> anybody who hasn't not volunteered. However, it doesn't seem to me that
> this is reason to boot the patch from the commitfest. I think the thing
> to do would be to remove yourself from the reviewers column and set it
> back to "needs review", so that other reviewers can pick it up.

It would indeed be wrong to change any patch from Needs Review to Returned
with Feedback on account of a personal distaste for reviewing the patch. I
hope I did not harbor such a motive here. Rather, this CommitFest has given
your patch its fair shake, and I and other reviewers would better serve the
CF's needs by reviewing, say, "ECPG FETCH readahead" instead of your latest
submission. Likewise, you would better serve the CF by evaluating one of the
four non-committer patches that have been Ready for Committer since January.
That's not to imply that the goals of the CF align with my goals, your goals,
or broader PGDG goals. The patch status on commitfest.postgresql.org does
exist solely for the advancement of the CF, and I have set it in accordingly.

> As for the late code churn, it mostly happened as a result of your
> own feedback; I would have left most of it in the original state, but as
> I went ahead it seemed much better to refactor things. This is mostly
> in heapam.c. As for multixact.c, it also had a lot of churn, but that
> was mostly to restore it to the state it has in the master branch,
> dropping much of the code I had written to handle multixact truncation.
> The new code there and in the vacuum code path (relminmxid and so on) is
> a lot smaller than that other code was, and it's closely based on
> relfrozenxid which is a known piece of technology.

I appreciate that.

> > However, review of such a large patch should not be simply pass or
> > fail. We should be looking back at the original problem and ask
> > ourselves whether some subset of the patch could solve a useful subset
> > of the problem. For me, that seems quite likely and this is very
> > definitely an important patch.

Incidentally, I find it harmful to think of "Returned with Feedback" as
"fail". For large patches, it's healthier to think of a CF as a bimonthly
project status meeting with stakeholders. When the project is done,
wonderful! When there's work left, that's no great surprise.

> > Even if we can't solve some part of the problem we can at least commit
> > some useful parts of infrastructure to allow later work to happen more
> > smoothly and quickly.
> >
> > So please let's not focus on the 100%, lets focus on 80/20.
>
> Well, we have the patch I originally posted in the 9.1 timeframe.
> That's a lot smaller and simpler. However, that solves only part of the
> blocking problem, and in particular it doesn't fix the initial deadlock
> reports from Joel Jacobson at Glue Finance (now renamed Trustly, in case
> you wonder about his change of email address) that started this effort
> in the first place. I don't think we can cut down to that and still
> satisfy the users that requested this; and Glue was just the first one,
> because after I started blogging about this, some more people started
> asking for it.
>
> I don't think there's any useful middlepoint between that one and the
> current one, but maybe I'm wrong.

Nothing additional comes to my mind, either. This patch is monolithic.

Thanks,
nm