Re: TABLESAMPLE patch is really in pretty sad shape - Mailing list pgsql-hackers

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.

I haven't actually written any code, but I've written a tsmapi.h file
modeled on fdwapi.h, and rewritten tablesample-method.sgml to match;
those two files are attached.  Some notes:

* This is predicated on the assumption that we'll get rid of the
pg_tablesample_method catalog and instead have a single FDW-style handler
function for each sample method.  That fixes the problem with the contrib
modules being broken in DROP/pg_upgrade cases.

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

* 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 got rid of the ExamineTuple function altogether.  As I said before,
I think what that is mostly doing is encouraging people to do unsound
things.  But in any case, there is no need for it because NextSampleTuple
*can* look at the HeapScanDesc state if it really wants to: that lets it
identify visible tuples, or even inspect the tuple contents.  In the
attached, I documented the visible-tuples aspect but did not suggest
examining tuple contents.

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

Barring objections, I'll press forward with turning this into code
over the next few days.

            regards, tom lane

/*-------------------------------------------------------------------------
 *
 * tsmapi.h
 *      API for tablesample methods
 *
 * Copyright (c) 2015, PostgreSQL Global Development Group
 *
 * src/include/access/tsmapi.h
 *
 *-------------------------------------------------------------------------
 */
#ifndef TSMAPI_H
#define TSMAPI_H

#include "nodes/execnodes.h"
#include "nodes/relation.h"


/*
 * Callback function signatures --- see tablesample-method.sgml for more info.
 */

typedef void (*SampleScanCost_function) (PlannerInfo *root,
                                         RelOptInfo *baserel,
                                         List *paramexprs,
                                         BlockNumber *pages,
                                         double *tuples);

typedef void (*BeginSampleScan_function) (SampleScanState *node,
                                          int eflags,
                                          uint32 seed,
                                          Datum *params,
                                          bool *paramisnull,
                                          int nparams);

typedef BlockNumber (*NextSampleBlock_function) (SampleScanState *node);

typedef OffsetNumber (*NextSampleTuple_function) (SampleScanState *node,
                                                  BlockNumber blockno,
                                                  OffsetNumber maxoffset);

typedef void (*ReScanSampleScan_function) (SampleScanState *node);

typedef void (*EndSampleScan_function) (SampleScanState *node);

/*
 * TsmRoutine is the struct returned by a tablesample method's handler
 * function.  It provides pointers to the callback functions needed by the
 * planner and executor, as well as additional information about the method.
 *
 * More function pointers are likely to be added in the future.
 * Therefore it's recommended that the handler initialize the struct with
 * makeNode(TsmRoutine) so that all fields are set to NULL.  This will
 * ensure that no fields are accidentally left undefined.
 */
typedef struct TsmRoutine
{
    NodeTag        type;

    /* List of datatype OIDs for the arguments of the TABLESAMPLE clause */
    List       *parameterTypes;

    /* Can method produce repeatable samples across, or even within, queries? */
    bool        repeatable_across_queries;
    bool        repeatable_across_scans;

    /* Functions for planning a SampleScan on a physical table */
    SampleScanCost_function SampleScanCost;

    /* Functions for executing a SampleScan on a physical table */
    BeginSampleScan_function BeginSampleScan;
    NextSampleBlock_function NextSampleBlock;    /* can be NULL */
    NextSampleTuple_function NextSampleTuple;
    ReScanSampleScan_function ReScanSampleScan;
    EndSampleScan_function EndSampleScan;        /* can be NULL */
} TsmRoutine;

#endif   /* TSMAPI_H */