Re: TABLESAMPLE patch

On Mon, Apr 6, 2015 at 11:02 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

> On 06/04/15 14:30, Petr Jelinek wrote:
>
>> On 06/04/15 11:02, Simon Riggs wrote:
>>
>> Are we ready for a final detailed review and commit?
>>>
>>>
>> I plan to send v12 in the evening with some additional changes that came
>> up from Amit's comments + some improvements to error reporting. I think
>> it will be ready then.
>>
>>
> Ok so here it is.
>
> Changes vs v11:
> - changed input parameter list to expr_list
> - improved error reporting, particularly when input parameters are wrong
> - fixed SELECT docs to show correct syntax and mention that there can be
> more sampling methods
> - added name of the sampling method to the explain output - I don't like
> the code much there as it has to look into RTE but on the other hand I
> don't want to create new scan node just so it can hold the name of the
> sampling method for explain
> - made views containing TABLESAMPLE clause not autoupdatable
> - added PageIsAllVisible() check before trying to check for tuple
> visibility
> - some typo/white space fixes

Compiler warnings:
explain.c: In function 'ExplainNode':
explain.c:861: warning: 'sname' may be used uninitialized in this function

Docs spellings:

"PostgreSQL distrribution" extra r.

"The optional parameter REPEATABLE acceps" accepts. But I don't know
that 'accepts' is the right word. It makes the seed value sound optional
to REPEATABLE.

"each block having same chance" should have "the" before "same".

"Both of those sampling methods currently...". I think it should be
"these" not "those", as this sentence is immediately after their
introduction, not at a distance.

"...tuple contents and decides if to return in, or zero if none" Something
here is confusing. "return it", not "return in"?

Other comments:

Do we want tab completions for psql? (I will never remember how to spell
BERNOULLI).

Needs a Cat version bump.

Cheers,

Jeff