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