On Wed, Apr 1, 2015 at 6:31 PM, Tomas Vondra <tomas.von...@2ndquadrant.com>
> 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
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

Reply via email to