Re: TABLESAMPLE patch

On Fri, Apr 10, 2015 at 9:58 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> On 09/04/15 21:30, Peter Eisentraut wrote:
>>
>>
>> In the SQL standard, the TABLESAMPLE clause is attached to a table
>> expression (<table primary>), which includes table functions,
>> subqueries, CTEs, etc. In the proposed patch, it is attached to a table
>> name, allowing only an ONLY clause. So this is a significant deviation.
>>
>
> I wouldn't call something that implements subset of standard a deviation.
> Especially if other major dbs have chosen same approach (afaik the only db
> that supports sampling over something besides physical relations is Oracle
> but their sampling works slightly differently than what standard has).
>
>> Obviously, doing block sampling on a physical table is a significant use
>> case, but we should be clear about which restrictions and tradeoffs were
>> are making now and in the future, especially if we are going to present
>> extension interfaces. The fact that physical tables are interchangeable
>> with other relation types, at least in data-reading contexts, is a
>> feature worth preserving.
>
>
> Yes, but I don't think there is anything that prevents us from adding this
> in the future. The sampling scan could made to be able to read both directly
> from heap and from executor subnode which is doable even if it won't be
> extremely pretty (but it should be easy to encapsulate into 2 internal
> interfaces as the heap reading is encapsulated to 1 internal interface
> already). Another approach would be having two different executor nodes -
> SampingScan and SamplingFilter and letting planner pick one depending on
> what is the source for TABLESAMPLE clause.
>
> The extension api is currently mainly:
> nextblock - gets next blockid to read from heap
> nextuple - gets next tupleid to read current block
> examinetuple - lets the extension to decide if tuple should be indeed
> returned (this one is optional)
>
> For the executor node reading we'd probably just call the examinetuple as
> there are no block ids or tuple ids there. This makes the API look slightly
> schizophrenic but on the other hand it gives the plugins control over how
> physical relation is read if that's indeed the source. And I guess we could
> let the plugin specify if it supports the heap access (nextblock/nexttuple)
> and if it doesn't then planner would always choose SamplingFilter over
> SequentialScan for physical relation instead of SamplingScan.
>
> All of this is possible to add without breaking compatibility with what is
> proposed for commit currently.
>
> The reasons why we need the nextblock and nexttuple interfaces and the
> ability to read the heap directly are a) block sampling can't be done by
> reading from another executor node, b) performance.
>
>>
>> It may be worth thinking about some examples of other sampling methods,
>> in order to get a better feeling for whether the interfaces are
>> appropriate.
>>
>
> There is one additional method which is just purely for testing the
> interface and that uses column value to determine if the tuple should be
> returned or not (which is useless in practice obviously as you can do that
> using WHERE, it just shows how to use the interface).
>
> I would like to eventually have something that's time limited rather than
> size limited for example. I didn't think much about other sampling
> algorithms but Simon proposed some and they should work with the current
> API.
>
>> Earlier in the thread, someone asked about supporting specifying a
>> number of rows instead of percents. While not essential, that seems
>> pretty useful, but I wonder how that could be implemented later on if we
>> take the approach that the argument to the sampling method can be an
>> arbitrary quantity that is interpreted only by the method.
>>
>
> Well, you can have two approaches to this, either allow some specific set of
> keywords that can be used to specify limit, or you let sampling methods
> interpret parameters, I believe the latter is more flexible. There is
> nothing stopping somebody writing sampling method which takes limit as
> number of rows, or anything else.
>
> Also for example for BERNOULLI to work correctly you'd need to convert the
> number of rows to fraction of table anyway (and that's exactly what the one
> database which has this feature does internally) and then it's no different
> than passing (SELECT 100/reltuples*number_of_rows FROM tablename) as a
> parameter.

Mentioning again that patch 1 is interesting as a separate change to
move the sampling logic out of the ANALYZE code in its own portion.

I had a look at patch 2. Here are some comments:
1)
+ <row>
+ <entry><structfield>tsmseqscan</structfield></entry>
+ <entry><type>bool</type></entry>
+ <entry></entry>
+ <entry>Does the sampling method scan the whole table sequentially?
+ </entry>
+ </row>
+
+ <row>
+ <entry><structfield>tsmpagemode</structfield></entry>
+ <entry><type>bool</type></entry>
+ <entry></entry>
+ <entry>Does the sampling method always read whole pages?
+ </entry>
+ </row>
I think that those descriptions using question marks are not adapted.
They could be reformulated as follows:
If true, the sampling method scans the whole table sequentially.
If true, the sampling method always reads the pages completely.

2) Shouldn't there be some EXPLAIN output in the regression tests?
3) The documentation should clearly state that TABLESAMPLE can only be
used on matviews and tables, and can only accept items directly
referenced in a FROM clause, aka no WITH or no row subsets in a
subquery. As things stand, TABLESAMPLE being mentioned in the line of
ONLY, users may think that views are supported as ONLY can be used
with views as well.
4)
+ The <literal>BERNOULLI</literal> scans whole table and returns
+ individual rows with equal probability. Additional sampling methods
+ may be installed in the database via extensions.
In this patch extensions are mentioned but this is implemented only in
patch 3. Hence this reference should be removed.
5)
- * whether a nondefault buffer access strategy can be used, and whether
+ * whether a nondefault buffer access strategy can be used and whether
Noise here?
6) If the sample method strategies are extended at some point, I think
that you want to use a bitmap in heap_beginscan_ss and friends and not
a set of boolean arguments.
7) This is surprising and I can't understand why things are mixed up here:
- scan->rs_pageatatime = IsMVCCSnapshot(snapshot);
+ scan->rs_pageatatime = allow_pagemode && IsMVCCSnapshot(snapshot);
Isn't what you want here a different parameter? I am sure that we do
not want to mix up visibility with MVCC snapshots and page mode.
8) I have done some tests with RLS and things seem to work (filters of
SELECT policies are taken into account after fetching the tuples from
the scan), but I would recommend adding some regression tests in this
area as TABLESAMPLE is a new type of physical heap scan.
9) s/visibilty/visibility
10) s/acceps/accepts.
11) s/dont't/don't
12) Is actually tsmexaminetuple necessary now? This is not used in
this patch at all, and looks to me like an optimization needed for
custom sampling methods. Keeping in mind the core feature I think that
this should be removed for now, let's reintroduce it later if there is
a real test case that shows up, catalogs are extensible AFAIK.
13) Some regression tests with pg_tablesample_method would be welcome.
14) Some comments about this part:
+void
+sampler_random_init_state(long seed, SamplerRandomState randstate)
+{
+ randstate[0] = 0x330e;
+ randstate[1] = (unsigned short) seed;
+ randstate[2] = (unsigned short) (seed >> 16);
+}

/* Select a random value R uniformly distributed in (0 - 1) */
double
-sampler_random_fract()
+sampler_random_fract(SamplerRandomState randstate)
{
- return ((double) random() + 1) / ((double) MAX_RANDOM_VALUE + 2);
+ return pg_erand48(randstate);
}
Hm. Doesn't this impact the selectivity of ANALYZE as well? I think
that we should be careful and use separate methods. I think that you
should use RAND48_SEED_0, _rand48_seed and friends as well instead of
hardcoding those values in your code.
15)
/*
+ * RangeTableSample - represents <table> TABLESAMPLE <method>
(<params>) REPEATABLE (<num>)
+ *
+ * We are more generic than SQL Standard so we pass generic function
+ * arguments to the sampling method.
+ */
This comment should be reformulated...

And some tests:
1) The patch fails correctly when something else than a table or a
matview is used:
=# select * from aav tablesample BERNOULLI (5.5) REPEATABLE (1);
ERROR: 42601: TABLESAMPLE clause can only be used on tables and
materialized views
2) Already mentioned upthread, but WITH clause fails strangely:
=# with query_select as (select a from aa) select

query_select.a from
query_select tablesample BERNOULLI (5.5) REPEATABLE (1);
ERROR: 42P01: relation "query_select" does not exist
I guess that this patch should only allow direct references to tables
or matviews in a FROM clause.
3) Using ALIAS with subqueries...
This works:
=# select i from aa as bb(i) tablesample BERNOULLI (5.5) REPEATABLE (1);
i
---
1
6
(2 rows)
Now I find surprising to see a failure here referring to a syntax failure:
=# select i from (select a from aa) as b(i) tablesample BERNOULLI
(5.5) REPEATABLE (1);
ERROR: 42601: syntax error at or near "tablesample"
4) A dummy sampling method:
=# explain select a from only test tablesample toto (100) REPEATABLE
(10) union select b from aa;
ERROR: 42704: tablesample method "toto" does not exist
5) REPEATABLE and NULL:
=# explain select a from only test tablesample bernoulli (100)
REPEATABLE (NULL) union select b from aa;
ERROR: 22023: REPEATABLE clause must be NOT NULL numeric value
6) Funnily I could not trigger this error:
+ if (base_rte->tablesample)
+ return gettext_noop("Views containing TABLESAMPLE are
not automatically updatable.");
7) Please add a test for that as well for both bernouilli and system:
+ ereport(ERROR,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("invalid sample size"),
+ errhint("Sample size must be numeric
value between 0 and 100 (inclusive).")));

The regression tests should cover as well those error scenarios.

Just a suggestion: but for 9.5 perhaps we should aim just at patches 1
and 2, and drop the custom TABLESAMPLE methods.

Regards,
--
Michael