On Tue, Jul 14, 2015 at 11:14:55AM +0100, Simon Riggs wrote:
> On 13 July 2015 at 14:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > TBH, I think the right thing to do at this point is to revert the entire
> > patch and send it back for ground-up rework. I think the high-level
> > design is wrong in many ways and I have about zero confidence in most
> > of the code details as well.
> >
>
> Based on the various comments here, I don't see that as the right course of
> action at this point.
>
> There are no issues relating to security or data loss, just various fixable
> issues in a low-impact feature, which in my view is an important feature
> also.
>
> > If it's
> > to stay, it *must* get a line-by-line review from some committer-level
> > person; and I think there are other more important things for us to be
> > doing for 9.5.
> >
>
> Honestly, I am very surprised by this.
Tom's partial review found quite a crop of unvarnished bugs:
1. sample node can give different tuples across rescans within an executor run
2. missing dependency machinery to restrict dropping a sampling extension
3. missing "pg_dump --binary-upgrade" treatment
4. "potential core dumps due to dereferencing values that could be null"
5. factually incorrect comments
6. null argument checks in strict functions
7. failure to check for constisnull
8. "failure to sanity-check" ntuples
9. arithmetic errors in random_relative_prime()
(That's after sifting out design counterproposals, feature requests, and other
topics of regular disagreement. I erred on the side of leaving things off
that list.) Finding one or two like that during a complete post-commit review
would be business as usual. Finding nine in a partial review diagnoses a
critical shortfall in pre-commit review vigilance. Fixing the bugs found to
date will not cure that shortfall. A qualified re-review could cure it.
nm