Redesign tablesample method API, and do extensive code review.

The original implementation of TABLESAMPLE modeled the tablesample method
API on index access methods, which wasn't a good choice because, without
specialized DDL commands, there's no way to build an extension that can
implement a TSM.  (Raw inserts into system catalogs are not an acceptable
thing to do, because we can't undo them during DROP EXTENSION, nor will
pg_upgrade behave sanely.)  Instead adopt an API more like procedural
language handlers or foreign data wrappers, wherein the only SQL-level
support object needed is a single handler function identified by having
a special return type.  This lets us get rid of the supporting catalog
altogether, so that no custom DDL support is needed for the feature.

Adjust the API so that it can support non-constant tablesample arguments
(the original coding assumed we could evaluate the argument expressions at
ExecInitSampleScan time, which is undesirable even if it weren't outright
unsafe), and discourage sampling methods from looking at invisible tuples.
Make sure that the BERNOULLI and SYSTEM methods are genuinely repeatable
within and across queries, as required by the SQL standard, and deal more
honestly with methods that can't support that requirement.

Make a full code-review pass over the tablesample additions, and fix
assorted bugs, omissions, infelicities, and cosmetic issues (such as
failure to put the added code stanzas in a consistent ordering).
Improve EXPLAIN's output of tablesample plans, too.

Back-patch to 9.5 so that we don't have to support the original API
in production.

1 files changed, 284 insertions, 161 deletions

diff --git a/contrib/tsm_system_rows/tsm_system_rows.c b/contrib/tsm_system_rows/tsm_system_rows.c
index e325eaff498..f251e3e5e06 100644
--- a/contrib/tsm_system_rows/tsm_system_rows.c
+++ b/contrib/tsm_system_rows/tsm_system_rows.c
@@ -1,240 +1,356 @@
 /*-------------------------------------------------------------------------
  *
  * tsm_system_rows.c
- *	  interface routines for system_rows tablesample method
+ *	  support routines for SYSTEM_ROWS tablesample method
  *
+ * The desire here is to produce a random sample with a given number of rows
+ * (or the whole relation, if that is fewer rows).  We use a block-sampling
+ * approach.  To ensure that the whole relation will be visited if necessary,
+ * we start at a randomly chosen block and then advance with a stride that
+ * is randomly chosen but is relatively prime to the relation's nblocks.
  *
- * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
+ * Because of the dependence on nblocks, this method cannot be repeatable
+ * across queries.  (Even if the user hasn't explicitly changed the relation,
+ * maintenance activities such as autovacuum might change nblocks.)  However,
+ * we can at least make it repeatable across scans, by determining the
+ * sampling pattern only once on the first scan.  This means that rescans
+ * won't visit blocks added after the first scan, but that is fine since
+ * such blocks shouldn't contain any visible tuples anyway.
+ *
+ * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  contrib/tsm_system_rows_rowlimit/tsm_system_rows.c
+ *	  contrib/tsm_system_rows/tsm_system_rows.c
  *
  *-------------------------------------------------------------------------
  */
 
 #include "postgres.h"
 
-#include "fmgr.h"
-
-#include "access/tablesample.h"
 #include "access/relscan.h"
+#include "access/tsmapi.h"
+#include "catalog/pg_type.h"
 #include "miscadmin.h"
-#include "nodes/execnodes.h"
-#include "nodes/relation.h"
 #include "optimizer/clauses.h"
-#include "storage/bufmgr.h"
+#include "optimizer/cost.h"
 #include "utils/sampling.h"
 
 PG_MODULE_MAGIC;
 
-/*
- * State
- */
+PG_FUNCTION_INFO_V1(tsm_system_rows_handler);
+
+
+/* Private state */
 typedef struct
 {
-	SamplerRandomState randstate;
 	uint32		seed;			/* random seed */
-	BlockNumber nblocks;		/* number of block in relation */
-	int32		ntuples;		/* number of tuples to return */
-	int32		donetuples;		/* tuples already returned */
+	int64		ntuples;		/* number of tuples to return */
+	int64		donetuples;		/* number of tuples already returned */
 	OffsetNumber lt;			/* last tuple returned from current block */
-	BlockNumber step;			/* step size */
+	BlockNumber doneblocks;		/* number of already-scanned blocks */
 	BlockNumber lb;				/* last block visited */
-	BlockNumber doneblocks;		/* number of already returned blocks */
-} SystemSamplerData;
-
-
-PG_FUNCTION_INFO_V1(tsm_system_rows_init);
-PG_FUNCTION_INFO_V1(tsm_system_rows_nextblock);
-PG_FUNCTION_INFO_V1(tsm_system_rows_nexttuple);
-PG_FUNCTION_INFO_V1(tsm_system_rows_examinetuple);
-PG_FUNCTION_INFO_V1(tsm_system_rows_end);
-PG_FUNCTION_INFO_V1(tsm_system_rows_reset);
-PG_FUNCTION_INFO_V1(tsm_system_rows_cost);
-
+	/* these three values are not changed during a rescan: */
+	BlockNumber nblocks;		/* number of blocks in relation */
+	BlockNumber firstblock;		/* first block to sample from */
+	BlockNumber step;			/* step size, or 0 if not set yet */
+} SystemRowsSamplerData;
+
+static void system_rows_samplescangetsamplesize(PlannerInfo *root,
+									RelOptInfo *baserel,
+									List *paramexprs,
+									BlockNumber *pages,
+									double *tuples);
+static void system_rows_initsamplescan(SampleScanState *node,
+						   int eflags);
+static void system_rows_beginsamplescan(SampleScanState *node,
+							Datum *params,
+							int nparams,
+							uint32 seed);
+static BlockNumber system_rows_nextsampleblock(SampleScanState *node);
+static OffsetNumber system_rows_nextsampletuple(SampleScanState *node,
+							BlockNumber blockno,
+							OffsetNumber maxoffset);
+static bool SampleOffsetVisible(OffsetNumber tupoffset, HeapScanDesc scan);
 static uint32 random_relative_prime(uint32 n, SamplerRandomState randstate);
 
+
 /*
- * Initializes the state.
+ * Create a TsmRoutine descriptor for the SYSTEM_ROWS method.
  */
 Datum
-tsm_system_rows_init(PG_FUNCTION_ARGS)
+tsm_system_rows_handler(PG_FUNCTION_ARGS)
 {
-	TableSampleDesc *tsdesc = (TableSampleDesc *) PG_GETARG_POINTER(0);
-	uint32		seed = PG_GETARG_UINT32(1);
-	int32		ntuples = PG_ARGISNULL(2) ? -1 : PG_GETARG_INT32(2);
-	HeapScanDesc scan = tsdesc->heapScan;
-	SystemSamplerData *sampler;
+	TsmRoutine *tsm = makeNode(TsmRoutine);
 
-	if (ntuples < 1)
-		ereport(ERROR,
-				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
-				 errmsg("invalid sample size"),
-				 errhint("Sample size must be positive integer value.")));
+	tsm->parameterTypes = list_make1_oid(INT8OID);
 
-	sampler = palloc0(sizeof(SystemSamplerData));
+	/* See notes at head of file */
+	tsm->repeatable_across_queries = false;
+	tsm->repeatable_across_scans = true;
 
-	/* Remember initial values for reinit */
-	sampler->seed = seed;
-	sampler->nblocks = scan->rs_nblocks;
-	sampler->ntuples = ntuples;
-	sampler->donetuples = 0;
-	sampler->lt = InvalidOffsetNumber;
-	sampler->doneblocks = 0;
-
-	sampler_random_init_state(sampler->seed, sampler->randstate);
-
-	/* Find relative prime as step size for linear probing. */
-	sampler->step = random_relative_prime(sampler->nblocks, sampler->randstate);
-
-	/*
-	 * Randomize start position so that blocks close to step size don't have
-	 * higher probability of being chosen on very short scan.
-	 */
-	sampler->lb = sampler_random_fract(sampler->randstate) *
-		(sampler->nblocks / sampler->step);
+	tsm->SampleScanGetSampleSize = system_rows_samplescangetsamplesize;
+	tsm->InitSampleScan = system_rows_initsamplescan;
+	tsm->BeginSampleScan = system_rows_beginsamplescan;
+	tsm->NextSampleBlock = system_rows_nextsampleblock;
+	tsm->NextSampleTuple = system_rows_nextsampletuple;
+	tsm->EndSampleScan = NULL;
 
-	tsdesc->tsmdata = (void *) sampler;
-
-	PG_RETURN_VOID();
+	PG_RETURN_POINTER(tsm);
 }
 
 /*
- * Get next block number or InvalidBlockNumber when we're done.
- *
- * Uses linear probing algorithm for picking next block.
+ * Sample size estimation.
  */
-Datum
-tsm_system_rows_nextblock(PG_FUNCTION_ARGS)
+static void
+system_rows_samplescangetsamplesize(PlannerInfo *root,
+									RelOptInfo *baserel,
+									List *paramexprs,
+									BlockNumber *pages,
+									double *tuples)
 {
-	TableSampleDesc *tsdesc = (TableSampleDesc *) PG_GETARG_POINTER(0);
-	SystemSamplerData *sampler = (SystemSamplerData *) tsdesc->tsmdata;
+	Node	   *limitnode;
+	int64		ntuples;
+	double		npages;
 
-	sampler->lb = (sampler->lb + sampler->step) % sampler->nblocks;
-	sampler->doneblocks++;
+	/* Try to extract an estimate for the limit rowcount */
+	limitnode = (Node *) linitial(paramexprs);
+	limitnode = estimate_expression_value(root, limitnode);
 
-	/* All blocks have been read, we're done */
-	if (sampler->doneblocks > sampler->nblocks ||
-		sampler->donetuples >= sampler->ntuples)
-		PG_RETURN_UINT32(InvalidBlockNumber);
+	if (IsA(limitnode, Const) &&
+		!((Const *) limitnode)->constisnull)
+	{
+		ntuples = DatumGetInt64(((Const *) limitnode)->constvalue);
+		if (ntuples < 0)
+		{
+			/* Default ntuples if the value is bogus */
+			ntuples = 1000;
+		}
+	}
+	else
+	{
+		/* Default ntuples if we didn't obtain a non-null Const */
+		ntuples = 1000;
+	}
 
-	PG_RETURN_UINT32(sampler->lb);
-}
+	/* Clamp to the estimated relation size */
+	if (ntuples > baserel->tuples)
+		ntuples = (int64) baserel->tuples;
+	ntuples = clamp_row_est(ntuples);
 
-/*
- * Get next tuple offset in current block or InvalidOffsetNumber if we are done
- * with this block.
- */
-Datum
-tsm_system_rows_nexttuple(PG_FUNCTION_ARGS)
-{
-	TableSampleDesc *tsdesc = (TableSampleDesc *) PG_GETARG_POINTER(0);
-	OffsetNumber maxoffset = PG_GETARG_UINT16(2);
-	SystemSamplerData *sampler = (SystemSamplerData *) tsdesc->tsmdata;
-	OffsetNumber tupoffset = sampler->lt;
+	if (baserel->tuples > 0 && baserel->pages > 0)
+	{
+		/* Estimate number of pages visited based on tuple density */
+		double		density = baserel->tuples / (double) baserel->pages;
 
-	if (tupoffset == InvalidOffsetNumber)
-		tupoffset = FirstOffsetNumber;
+		npages = ntuples / density;
+	}
 	else
-		tupoffset++;
-
-	if (tupoffset > maxoffset ||
-		sampler->donetuples >= sampler->ntuples)
-		tupoffset = InvalidOffsetNumber;
+	{
+		/* For lack of data, assume one tuple per page */
+		npages = ntuples;
+	}
 
-	sampler->lt = tupoffset;
+	/* Clamp to sane value */
+	npages = clamp_row_est(Min((double) baserel->pages, npages));
 
-	PG_RETURN_UINT16(tupoffset);
+	*pages = npages;
+	*tuples = ntuples;
 }
 
 /*
- * Examine tuple and decide if it should be returned.
+ * Initialize during executor setup.
  */
-Datum
-tsm_system_rows_examinetuple(PG_FUNCTION_ARGS)
+static void
+system_rows_initsamplescan(SampleScanState *node, int eflags)
 {
-	TableSampleDesc *tsdesc = (TableSampleDesc *) PG_GETARG_POINTER(0);
-	bool		visible = PG_GETARG_BOOL(3);
-	SystemSamplerData *sampler = (SystemSamplerData *) tsdesc->tsmdata;
-
-	if (!visible)
-		PG_RETURN_BOOL(false);
-
-	sampler->donetuples++;
-
-	PG_RETURN_BOOL(true);
+	node->tsm_state = palloc0(sizeof(SystemRowsSamplerData));
+	/* Note the above leaves tsm_state->step equal to zero */
 }
 
 /*
- * Cleanup method.
+ * Examine parameters and prepare for a sample scan.
  */
-Datum
-tsm_system_rows_end(PG_FUNCTION_ARGS)
+static void
+system_rows_beginsamplescan(SampleScanState *node,
+							Datum *params,
+							int nparams,
+							uint32 seed)
 {
-	TableSampleDesc *tsdesc = (TableSampleDesc *) PG_GETARG_POINTER(0);
+	SystemRowsSamplerData *sampler = (SystemRowsSamplerData *) node->tsm_state;
+	int64		ntuples = DatumGetInt64(params[0]);
+
+	if (ntuples < 0)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_TABLESAMPLE_ARGUMENT),
+				 errmsg("sample size must not be negative")));
 
-	pfree(tsdesc->tsmdata);
+	sampler->seed = seed;
+	sampler->ntuples = ntuples;
+	sampler->donetuples = 0;
+	sampler->lt = InvalidOffsetNumber;
+	sampler->doneblocks = 0;
+	/* lb will be initialized during first NextSampleBlock call */
+	/* we intentionally do not change nblocks/firstblock/step here */
 
-	PG_RETURN_VOID();
+	/*
+	 * We *must* use pagemode visibility checking in this module, so force
+	 * that even though it's currently default.
+	 */
+	node->use_pagemode = true;
 }
 
 /*
- * Reset state (called by ReScan).
+ * Select next block to sample.
+ *
+ * Uses linear probing algorithm for picking next block.
  */
-Datum
-tsm_system_rows_reset(PG_FUNCTION_ARGS)
+static BlockNumber
+system_rows_nextsampleblock(SampleScanState *node)
 {
-	TableSampleDesc *tsdesc = (TableSampleDesc *) PG_GETARG_POINTER(0);
-	SystemSamplerData *sampler = (SystemSamplerData *) tsdesc->tsmdata;
+	SystemRowsSamplerData *sampler = (SystemRowsSamplerData *) node->tsm_state;
+	HeapScanDesc scan = node->ss.ss_currentScanDesc;
 
-	sampler->lt = InvalidOffsetNumber;
-	sampler->donetuples = 0;
-	sampler->doneblocks = 0;
+	/* First call within scan? */
+	if (sampler->doneblocks == 0)
+	{
+		/* First scan within query? */
+		if (sampler->step == 0)
+		{
+			/* Initialize now that we have scan descriptor */
+			SamplerRandomState randstate;
+
+			/* If relation is empty, there's nothing to scan */
+			if (scan->rs_nblocks == 0)
+				return InvalidBlockNumber;
+
+			/* We only need an RNG during this setup step */
+			sampler_random_init_state(sampler->seed, randstate);
+
+			/* Compute nblocks/firstblock/step only once per query */
+			sampler->nblocks = scan->rs_nblocks;
 
-	sampler_random_init_state(sampler->seed, sampler->randstate);
-	sampler->step = random_relative_prime(sampler->nblocks, sampler->randstate);
-	sampler->lb = sampler_random_fract(sampler->randstate) * (sampler->nblocks / sampler->step);
+			/* Choose random starting block within the relation */
+			/* (Actually this is the predecessor of the first block visited) */
+			sampler->firstblock = sampler_random_fract(randstate) *
+				sampler->nblocks;
+
+			/* Find relative prime as step size for linear probing */
+			sampler->step = random_relative_prime(sampler->nblocks, randstate);
+		}
+
+		/* Reinitialize lb */
+		sampler->lb = sampler->firstblock;
+	}
+
+	/* If we've read all blocks or returned all needed tuples, we're done */
+	if (++sampler->doneblocks > sampler->nblocks ||
+		sampler->donetuples >= sampler->ntuples)
+		return InvalidBlockNumber;
+
+	/*
+	 * It's probably impossible for scan->rs_nblocks to decrease between scans
+	 * within a query; but just in case, loop until we select a block number
+	 * less than scan->rs_nblocks.  We don't care if scan->rs_nblocks has
+	 * increased since the first scan.
+	 */
+	do
+	{
+		/* Advance lb, using uint64 arithmetic to forestall overflow */
+		sampler->lb = ((uint64) sampler->lb + sampler->step) % sampler->nblocks;
+	} while (sampler->lb >= scan->rs_nblocks);
 
-	PG_RETURN_VOID();
+	return sampler->lb;
 }
 
 /*
- * Costing function.
+ * Select next sampled tuple in current block.
+ *
+ * In block sampling, we just want to sample all the tuples in each selected
+ * block.
+ *
+ * When we reach end of the block, return InvalidOffsetNumber which tells
+ * SampleScan to go to next block.
  */
-Datum
-tsm_system_rows_cost(PG_FUNCTION_ARGS)
+static OffsetNumber
+system_rows_nextsampletuple(SampleScanState *node,
+							BlockNumber blockno,
+							OffsetNumber maxoffset)
 {
-	PlannerInfo *root = (PlannerInfo *) PG_GETARG_POINTER(0);
-	Path	   *path = (Path *) PG_GETARG_POINTER(1);
-	RelOptInfo *baserel = (RelOptInfo *) PG_GETARG_POINTER(2);
-	List	   *args = (List *) PG_GETARG_POINTER(3);
-	BlockNumber *pages = (BlockNumber *) PG_GETARG_POINTER(4);
-	double	   *tuples = (double *) PG_GETARG_POINTER(5);
-	Node	   *limitnode;
-	int32		ntuples;
+	SystemRowsSamplerData *sampler = (SystemRowsSamplerData *) node->tsm_state;
+	HeapScanDesc scan = node->ss.ss_currentScanDesc;
+	OffsetNumber tupoffset = sampler->lt;
 
-	limitnode = linitial(args);
-	limitnode = estimate_expression_value(root, limitnode);
+	/* Quit if we've returned all needed tuples */
+	if (sampler->donetuples >= sampler->ntuples)
+		return InvalidOffsetNumber;
 
-	if (IsA(limitnode, RelabelType))
-		limitnode = (Node *) ((RelabelType *) limitnode)->arg;
+	/*
+	 * Because we should only count visible tuples as being returned, we need
+	 * to search for a visible tuple rather than just let the core code do it.
+	 */
 
-	if (IsA(limitnode, Const))
-		ntuples = DatumGetInt32(((Const *) limitnode)->constvalue);
-	else
+	/* We rely on the data accumulated in pagemode access */
+	Assert(scan->rs_pageatatime);
+	for (;;)
 	{
-		/* Default ntuples if the estimation didn't return Const. */
-		ntuples = 1000;
+		/* Advance to next possible offset on page */
+		if (tupoffset == InvalidOffsetNumber)
+			tupoffset = FirstOffsetNumber;
+		else
+			tupoffset++;
+
+		/* Done? */
+		if (tupoffset > maxoffset)
+		{
+			tupoffset = InvalidOffsetNumber;
+			break;
+		}
+
+		/* Found a candidate? */
+		if (SampleOffsetVisible(tupoffset, scan))
+		{
+			sampler->donetuples++;
+			break;
+		}
 	}
 
-	*pages = Min(baserel->pages, ntuples);
-	*tuples = ntuples;
-	path->rows = *tuples;
+	sampler->lt = tupoffset;
 
-	PG_RETURN_VOID();
+	return tupoffset;
 }
 
+/*
+ * Check if tuple offset is visible
+ *
+ * In pageatatime mode, heapgetpage() already did visibility checks,
+ * so just look at the info it left in rs_vistuples[].
+ */
+static bool
+SampleOffsetVisible(OffsetNumber tupoffset, HeapScanDesc scan)
+{
+	int			start = 0,
+				end = scan->rs_ntuples - 1;
+
+	while (start <= end)
+	{
+		int			mid = (start + end) / 2;
+		OffsetNumber curoffset = scan->rs_vistuples[mid];
+
+		if (tupoffset == curoffset)
+			return true;
+		else if (tupoffset < curoffset)
+			end = mid - 1;
+		else
+			start = mid + 1;
+	}
+
+	return false;
+}
 
+/*
+ * Compute greatest common divisor of two uint32's.
+ */
 static uint32
 gcd(uint32 a, uint32 b)
 {
@@ -250,22 +366,29 @@ gcd(uint32 a, uint32 b)
 	return b;
 }
 
+/*
+ * Pick a random value less than and relatively prime to n, if possible
+ * (else return 1).
+ */
 static uint32
 random_relative_prime(uint32 n, SamplerRandomState randstate)
 {
-	/* Pick random starting number, with some limits on what it can be. */
-	uint32		r = (uint32) sampler_random_fract(randstate) * n / 2 + n / 4,
-				t;
+	uint32		r;
+
+	/* Safety check to avoid infinite loop or zero result for small n. */
+	if (n <= 1)
+		return 1;
 
 	/*
 	 * This should only take 2 or 3 iterations as the probability of 2 numbers
-	 * being relatively prime is ~61%.
+	 * being relatively prime is ~61%; but just in case, we'll include a
+	 * CHECK_FOR_INTERRUPTS in the loop.
 	 */
-	while ((t = gcd(r, n)) > 1)
+	do
 	{
 		CHECK_FOR_INTERRUPTS();
-		r /= t;
-	}
+		r = (uint32) (sampler_random_fract(randstate) * n);
+	} while (r == 0 || gcd(r, n) > 1);
 
 	return r;
 }