Re: TABLESAMPLE patch

On 10/04/15 06:46, Michael Paquier wrote:
>
> 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.
>

Agreed, I didn't like it much either, was just trying to copy style of
indexam.

> 2) Shouldn't there be some EXPLAIN output in the regression tests?

Done.

> 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.

I added it to standard compatibility section.

> 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.

They can be installed even without patch 3. The patch 3 just adds SQL
interface for it. This is like saying nobody can write index am which
certainly is possible even if not easy.

> 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.

Which can be done once this is actually an issue, it's internal API so
it's fine to change it when needed.

> 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.

The whole point of pagemode is to make visibility checks for all tuples
on single page in one go because the check requires buffer lock so those
two are very connected.

> 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.

Hmm RLS, I freely admit that I don't know what all needs to be tested
there, but I added some simple test for it.

> 9) s/visibilty/visibility
> 10) s/acceps/accepts.
> 11) s/dont't/don't

I can't write apparently :)

> 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.

Yes, for some sampling methods it's useful as demonstrated by the test
module. I was actually asked off-list for this by a committer and Simon
also sees use for it and I personally see it as good thing for
extendability as well so I'd really like to keep it.

> 13) Some regression tests with pg_tablesample_method would be welcome.

Not sure what you mean by that.

> 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

Well, ANALZE is using pg_erand48 anyway and I use random() as seed so I
think it shouldn't but I agree that it's something committer should pay
attention to.

> should use RAND48_SEED_0, _rand48_seed and friends as well instead of
> hardcoding those values in your code.

Yes, done.

> 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...
>

Done.

>
> 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.

I added CTE check and regression test for it.

> 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"

Well that's because we handle this already in parser.

> 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.

Done.

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

I agree that DDL patch is not that important to get in (and I made it
last patch in the series now), which does not mean somebody can't write
the extension with new tablesample method.

In any case attached another version.

Changes:
- I addressed the comments from Michael

- I moved the interface between nodeSampleScan and the actual sampling
method to it's own .c file and added TableSampleDesc struct for it. This
makes the interface cleaner and will make it more straightforward to
extend for subqueries in the future (nothing really changes just some
functions were renamed and moved). Amit suggested this at some point and
I thought it's not needed at that time but with the possible future
extension to subquery support I changed my mind.

- renamed heap_beginscan_ss to heap_beginscan_sampling to avoid
confusion with sync scan

- reworded some things and more typo fixes

- Added two sample contrib modules demonstrating row limited and time
limited sampling. I am using linear probing for both of those as the
builtin block sampling is not well suited for row limited or time
limited sampling. For row limited I originally thought of using the
Vitter's reservoir sampling but that does not fit well with the executor
as it needs to keep the reservoir of all the output tuples in memory
which would have horrible memory requirements if the limit was high. The
linear probing seems to work quite well for the use case of "give me 500
random rows from table".

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