Re: TABLESAMPLE patch is really in pretty sad shape

On 2015-07-19 22:56, Tom Lane wrote:
> Since I'm not observing any movement on the key question of redesigning
> the tablesample method API, and I think that's something that's absolutely
> necessary to fix for 9.5, attached is an attempt to respecify the API.
>

Sorry, I got something similar to what you posted written as well, but I
didn't want to submit before I have more working code done.

>
> * I got rid of the TableSampleDesc struct altogether in favor of giving
> the execution functions access to the whole SampleScanState executor
> state node. If we add support for filtering at the join level, filtering
> in indexscan nodes, etc, those would become separate sets of API functions
> in my view; there is no need to pretend that this set of API functions
> works for anything except the SampleScan case. This way is more parallel
> to the FDW precedent, too. In particular it lets tablesample code get at
> the executor's EState, which the old API did not, but which might be
> necessary for some scenarios.

Ok.

>
> * You might have expected me to move the tsmseqscan and tsmpagemode flags
> into the TsmRoutine struct, but instead this API puts equivalent flags
> into the SampleScanState struct. The reason for that is that it lets
> the settings be determined at runtime after inspecting the TABLESAMPLE
> parameters, which I think would be useful. For example, whether to use
> the bulkread strategy should probably depend on what the sampling
> percentage is.
>

I think this ignores one aspect of the old API. That is, we allowed the
sampling method to specify which kind of input parameters it accepts.
This is why for example the tsm_system_rows accepts integer while system
and bernoulli both accept float8. This part of the API is certainly not
needed for bernoulli and system sampling but if we ever want to do
something more sophisticated like stratified sampling which Simon
mentioned several times, specifying just percentages is not going to be
enough.

>
> * As written, this still allows TABLESAMPLE parameters to have null
> values, but I'm pretty strongly tempted to get rid of that: remove
> the paramisnull[] argument and make the core code reject null values.
> I can't see any benefit in allowing null values that would justify the
> extra code and risks-of-omission involved in making every tablesample
> method check this for itself.
>

I am for not allowing NULLs.

> * I specified that omitting NextSampleBlock is allowed and causes the
> core code to do a standard seqscan, including syncscan support, which
> is a behavior that's impossible with the current API. If we fix
> the bernoulli code to have history-independent sampling behavior,
> I see no reason that syncscan shouldn't be enabled for it.
>

Umm, we were actually doing syncscan as well before.

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