On Thu, Apr 9, 2015 at 4:30 AM, Jeff Janes <jeff.ja...@gmail.com> wrote: > On Mon, Apr 6, 2015 at 11:02 AM, Petr Jelinek <p...@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).
Yes. I think so. > Needs a Cat version bump. The committer who will pick up this patch will normally do it. Patch 1 is simple enough and looks fine to me. Regarding patch 2... I found for now some typos: + <title><structname>pg_tabesample_method</structname></title> + <productname>PostgreSQL</productname> distrribution: Also, I am wondering if the sampling logic based on block analysis is actually correct, for example for now this fails and I think that we should support it: =# with query_select as (select generate_series(1, 10) as a) select query_select.a from query_select tablesample system (100.0/11) REPEATABLE (9999); ERROR: 42P01: relation "query_select" does not exist How does the SQL spec define exactly TABLESAMPLE? Shouldn't we get a sample from a result set? Thoughts? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
