Re: TABLESAMPLE patch

On 04/04/15 14:57, Amit Kapila wrote:
>
> 1.
> +tablesample_clause:
> +TABLESAMPLE ColId '(' func_arg_list ')' opt_repeatable_clause
>
>
> Why do you want to allow func_arg_list?
>
> Basically if user tries to pass multiple arguments in
> TABLESAMPLE method's clause like (10,20), then I think
> that should be caught in grammer level as an error.

It will be reported as error during parse analysis if the TABLESAMPLE
method does not accept two parameters, same as when the expression used
wrong type for example.

>
> SQL - 2003 specs says that argument to REPAEATABLE and TABLESAMPLE
> method is same <numeric value expr>
>
> It seems to me that you want to allow it to make it extendable
> to user defined Tablesample methods, but not sure if it is
> right to use func_arg_list for the same because sample method
> arguments can have different definition than function arguments.
> Now even if we want to keep sample method arguments same as
> function arguments that sounds like a separate discussion.
>

Yes I want extensibility here. And I think the tablesample method
arguments are same thing as function arguments given that in the end
they are arguments for the init function of tablesampling method.

I would be ok with just expr_list, naming parameters here isn't overly
important, but ability to have different types and numbers of parameters
for custom TABLESAMPLE methods *is* important.

> In general, I feel you already have good basic infrastructure for
> supportting other sample methods, but it is better to keep the new
> DDL's for doing the same as a separate patch than this patch, as that
> way we can reduce the scope of this patch, OTOH if you or others
> feel that it is mandatory to have new DDL's support for other
> tablesample methods then we have to deal with this now itself.

Well I did attach it as separate diff mainly for that reason. I agree
that DDL does not have to be committed immediately with the rest of the
patch (although it's the simplest part of the patch IMHO).

>
> 2.
> postgres=# explain update test_tablesample TABLESAMPLE system(30) set id
> = 2;
> ERROR: syntax error at or near "TABLESAMPLE"
> LINE 1: explain update test_tablesample TABLESAMPLE system(30) set i...
>
> postgres=# Delete from test_tablesample TABLESAMPLE system(30);
> ERROR: syntax error at or near "TABLESAMPLE"
> LINE 1: Delete from test_tablesample TABLESAMPLE system(30);
>
> Isn't TABLESAMPLE clause suppose to work with Update/Delete
> statements?
>

It's supported in the FROM part of UPDATE and USING part of DELETE. I
think that that's sufficient.

Standard is somewhat useless for UPDATE and DELETE as it only defines
quite limited syntax there. From what I've seen when doing research
MSSQL also only supports it in their equivalent of FROM/USING list,
Oracle does not seem to support their SAMPLING clause outside of SELECTs
at all and if I got the cryptic DB2 manual correctly I think they don't
support it outside of (sub)SELECTs either.

> 4.
> SampleTupleVisible()
> {
> ..
> else
> {
> /* No pagemode, we have to check the tuple itself. */
> Snapshot
> snapshot = scan->rs_snapshot;
> Bufferbuffer = scan->rs_cbuf;
>
> bool visible =
> HeapTupleSatisfiesVisibility(tuple, snapshot, buffer);
> ..
> }
>
> I think it is better to check if PageIsAllVisible() in above
> code before visibility check as that can avoid visibility checks.

It's probably even better to do that one level up in the samplenexttup()
and only call the SampleTupleVisible if page is not allvisible
(PageIsAllVisible() is cheap).

>
> 6.
> extern TableSampleClause *
> ParseTableSample(ParseState *pstate, char *samplemethod, Node *repeatable,
> List *sampleargs)
>
> It is better to expose function (usage of extern) via header file.
> Is there a need to mention extern here?
>

Eh, stupid copy-paste error when copying function name from header to
actual source file.

> 7.
> ParseTableSample()
> {
> ..
> arg = coerce_to_specific_type(pstate, arg, INT4OID, "REPEATABLE");
> ..
> }
>
> What is the reason for coercing value of REPEATABLE clause to INT4OID
> when numeric value is expected for the clause. If user gives the
> input value as -2.3, it will generate a seed which doesn't seem to
> be right.
>

Because the REPEATABLE is numeric expression so it can produce whatever
number but we need int4 internally (well float4 would also work just the
code would be slightly uglier). And we do this type of coercion even for
table data (you can insert -2.3 into integer column and it will work) so
I don't see what's wrong with it here.

>
> 8.
> +DATA(insert OID = 3295 ( tsm_system_initPGNSP PGUID 12 1 0 0 0 f f f f
> t f v 3 0 2278 "2281
> 23 700" _null_ _null_ _null_ _null_tsm_system_init _null_ _null_ _null_ ));
> +DATA(insert OID = 3301 ( tsm_bernoulli_initPGNSP PGUID 12 1 0 0 0 f f
> f f t f v 3 0 2278 "2281
> 23 700" _null_ _null_ _null_ _null_tsm_bernoulli_init _null_ _null_
> _null_ ));
>
> Datatype for second argument is kept as 700 (Float4), shouldn't
> it be 1700 (Numeric)?
>

Why is that? Given the sampling error I think the float4 is enough for
specifying the percentage and it makes the calculations much easier and
faster than dealing with Numeric would.

> 9.
> postgres=# explain SELECT t1.id <http://t1.id> FROM test_tablesample as
> t1 TABLESAMPLE SYSTEM (
> 10), test_tablesample as t2 TABLESAMPLE BERNOULLI (20);
> QUERY PLAN
> ----------------------------------------------------------------------------
> Nested Loop (cost=0.00..4.05 rows=100 width=4)
> -> Sample Scan on test_tablesample t1 (cost=0.00..0.01 rows=1 width=4)
> -> Sample Scan on test_tablesample t2 (cost=0.00..4.02 rows=2 width=0)
> (3 rows)
>
> Isn't it better to display sample method name in explain.
> I think it can help in case of join queries.
> We can use below format to display:
> Sample Scan (System) on test_tablesample ...

Good idea.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services