Re: TABLESAMPLE patch is really in pretty sad shape

On 15 July 2015 at 05:58, Noah Misch <noah(at)leadboat(dot)com> wrote:

> > > 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.
>

Thank you for the summary of points. I agree with that list.

I will work on the re-review as you suggest.

1 and 4 relate to the sample API exposed, which needs some rework. We'll
see how big that is; at this time I presume not that hard, but I will wait
for Petr's opinion also when he returns on Friday.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services