Re: TABLESAMPLE patch

On Wed, Apr 1, 2015 at 6:31 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>
> 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 was added later.
>
> 2) transformTableEntry() in parse_clause modifies the comment, in a way
>    that made sense before part of the code was moved to 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 (both the code and
>    comments) which is a bit awkward as the only part that uses it is
>    analyze.c. The other samplers using this 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 recommend adding 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.)
>

I am still not sure whether it is okay to move REPEATABLE from