On 03/15/15 16:21, Petr Jelinek wrote:
>
> I also did all the other adjustments we talked about up-thread and
> rebased against current master (there was conflict with 31eae6028).
>
Hi,
I did a review of the version submitted on 03/15 today, and only found a
few minor issues:
1) The documentation of the pg_tablesample_method catalog is missing documentation of the 'tsmpagemode' column, which
wasadded later.
2) transformTableEntry() in parse_clause modifies the comment, in a way that made sense before part of the code was
movedto a separate function. I suggest to revert the comment changes, and instead add the comment to
transformTableSampleEntry()
3) The "shared" parts of the block sampler in sampling.c (e.g. in BlockSampler_Next) reference Vitter's algorithm
(boththe code and comments) which is a bit awkward as the only part that uses it is analyze.c. The other samplers
usingthis code (system / bernoulli) don't use Vitter's algorithm.
I don't think it's possible to separate this piece of code, though. It simply has to be in there somewhere, so I'd
recommendadding here a simple comment explaining that it's needed because of analyze.c.
Otherwise I think the patch is OK. I'll wait for Petr to fix these
issues, and then mark it as ready for committer.
What do you think, Amit? (BTW you should probably add yourself as
reviewer in the CF app, as you've provided a lot of feedback here.)
--
Tomas Vondra http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
