Re: TABLESAMPLE patch

On Wed, Apr 1, 2015 at 6:31 PM, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)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
unreserved to other category. In-fact last weekend I have spent some
time to see the exact reason for shift/reduce errors and tried some ways
but didn't find a way to get away with the same. Now I am planning to
spend some more time on the same probably in next few days and then
still if I cannot find a way, I will share my findings and then once
re-review
the changes made by Petr in last version. I think overall the patch is in
good shape now although I haven't looked into DDL support part of the
patch which I thought could be done in a separate patch as well.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com