Re: TABLESAMPLE patch

On Mon, Apr 6, 2015 at 11:32 PM, 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
>

New changes looks fine to me except for one typo.

+ The optional parameter <literal>REPEATABLE</literal> acceps any
number
+ or expression producing a number and is used as random seed for
+ sampling.

typo

/acceps/accepts

So the patch for implementation of SYSTEM and BERNOULLI TABLESAMPLE
methods is "Ready For Committer". I could not get chance to review the
DDL patch (patch to implement user defined TABLESAMPLE methods).

Note to Committer - None of us is very clear on what should be the
implementation for Tablesample clause incase of UPDATE/DELETE
statement. I am of opinion that either we support to Update/Delete
based on Tablesample clause or prohibit it in all cases, however Peter
thinks it is okay to support for the same in FROM,USING clause of
Update, Delete respectively.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com