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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
