Re: TABLESAMPLE patch - Mailing list pgsql-hackers

From Jeff Janes
Subject Re: TABLESAMPLE patch
Date
Msg-id CAMkU=1wL4byNeDZieYyiwQ3q68G=oNPiYgmQf0Q8b4fZgGyTQw@mail.gmail.com
Whole thread Raw
In response to Re: TABLESAMPLE patch  (Petr Jelinek <petr@2ndquadrant.com>)
Responses Re: TABLESAMPLE patch
List pgsql-hackers
On Mon, Apr 6, 2015 at 11:02 AM, Petr Jelinek <petr@2ndquadrant.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

pgsql-hackers by date:

Previous
From: David Steele
Date:
Subject: Re: deparsing utility commands
Next
From: Andres Freund
Date:
Subject: Re: "rejected" vs "returned with feedback" in new CF app