Subject: postgres_fdw: using TABLESAMPLE to collect remote sample

From: Tomas Vondra <[email-masked]>
Date: Fri, 11 Feb 2022 18:02:37 +0100

Hi,

here's a small patch modifying postgres_fdw to use TABLESAMPLE to 
collect sample when running ANALYZE on a foreign table. Currently the 
sampling happens locally, i.e. we fetch all data from the remote server 
and then pick rows. But that's hugely expensive for large relations 
and/or with limited network bandwidth, of course.

Alexander Lepikhov mentioned this in [1], but it was actually proposed 
by Stephen in 2020 [2] but no patch even materialized.

So here we go. The patch does a very simple thing - it uses TABLESAMPLE 
to collect/transfer just a small sample from the remote node, saving 
both CPU and network.

And indeed, that helps quite a bit:

---------------------------------------------------------------------
create table t (a int);
Time: 10.223 ms

insert into t select i from generate_series(1,10000000) s(i);
Time: 552.207 ms

analyze t;
Time: 310.445 ms

CREATE FOREIGN TABLE ft (a int)
   SERVER foreign_server
   OPTIONS (schema_name 'public', table_name 't');
Time: 4.838 ms

analyze ft;
WARNING:  SQL: DECLARE c1 CURSOR FOR SELECT a FROM public.t TABLESAMPLE 
SYSTEM(0.375001)
Time: 44.632 ms

alter foreign table ft options (sample 'false');
Time: 4.821 ms

analyze ft;
WARNING:  SQL: DECLARE c1 CURSOR FOR SELECT a FROM public.t
Time: 6690.425 ms (00:06.690)
---------------------------------------------------------------------

6690ms without sampling, and 44ms with sampling - quite an improvement. 
Of course, the difference may be much smaller/larger in other cases.

Now, there's a couple issues ...

Firstly, the FDW API forces a bit strange division of work between 
different methods and duplicating some of it (and it's already mentioned 
in postgresAnalyzeForeignTable). But that's minor and can be fixed.

The other issue is which sampling method to use - we have SYSTEM and 
BERNOULLI built in, and the tsm_system_rows as an extension (and _time, 
but that's not very useful here). I guess we'd use one of the built-in 
ones, because that'll work on more systems out of the box.

But that leads to the main issue - determining the fraction of rows to 
sample. We know how many rows we want to sample, but we have no idea how 
many rows there are in total. We can look at reltuples, but we can't be 
sure how accurate / up-to-date that value is.

The patch just trusts it unless it's obviously bogus (-1, 0, etc.) and 
applies some simple sanity checks, but I wonder if we need to do more 
(e.g. look at relation size and adjust reltuples by current/relpages).

FWIW this is yet a bit more convoluted when analyzing partitioned table 
with foreign partitions, because we only ever look at relation sizes to 
determine how many rows to sample from each partition.

regards

[1] 
https://www.postgresql.org/message-id/bdb0bea2-a0da-1f1d-5c92-96ff90c198eb%40postgrespro.ru

[2] 
https://www.postgresql.org/message-id/20200829162231.GE29590%40tamriel.snowman.net

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index bf12eac0288..68f875c47a6 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -2267,6 +2267,26 @@ deparseAnalyzeSizeSql(StringInfo buf, Relation rel)
 	appendStringInfo(buf, "::pg_catalog.regclass) / %d", BLCKSZ);
 }
 
+/*
+ * Construct SELECT statement to acquire numbe of rows of given relation.
+ *
+ * Note: Maybe this should compare relpages and current relation size
+ * and adjust reltuples accordingly?
+ */
+void
+deparseAnalyzeTuplesSql(StringInfo buf, Relation rel)
+{
+	StringInfoData relname;
+
+	/* We'll need the remote relation name as a literal. */
+	initStringInfo(&relname);
+	deparseRelation(&relname, rel);
+
+	appendStringInfoString(buf, "SELECT reltuples FROM pg_catalog.pg_class WHERE oid = ");
+	deparseStringLiteral(buf, relname.data);
+	appendStringInfoString(buf, "::pg_catalog.regclass");
+}
+
 /*
  * Construct SELECT statement to acquire sample rows of given relation.
  *
@@ -2328,6 +2348,72 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs)
 	deparseRelation(buf, rel);
 }
 
+/*
+ * Construct SELECT statement to acquire sample rows of given relation,
+ * by sampling a fraction of the table using TABLESAMPLE SYSTEM.
+ *
+ * SELECT command is appended to buf, and list of columns retrieved
+ * is returned to *retrieved_attrs.
+ *
+ * Note: We could allow selecting system/bernoulli, and maybe even the
+ * optional TSM modules (especially tsm_system_rows would help).
+ */
+void
+deparseAnalyzeSampleSql(StringInfo buf, Relation rel, List **retrieved_attrs, double sample_frac)
+{
+	Oid			relid = RelationGetRelid(rel);
+	TupleDesc	tupdesc = RelationGetDescr(rel);
+	int			i;
+	char	   *colname;
+	List	   *options;
+	ListCell   *lc;
+	bool		first = true;
+
+	*retrieved_attrs = NIL;
+
+	appendStringInfoString(buf, "SELECT ");
+	for (i = 0; i < tupdesc->natts; i++)
+	{
+		/* Ignore dropped columns. */
+		if (TupleDescAttr(tupdesc, i)->attisdropped)
+			continue;
+
+		if (!first)
+			appendStringInfoString(buf, ", ");
+		first = false;
+
+		/* Use attribute name or column_name option. */
+		colname = NameStr(TupleDescAttr(tupdesc, i)->attname);
+		options = GetForeignColumnOptions(relid, i + 1);
+
+		foreach(lc, options)
+		{
+			DefElem    *def = (DefElem *) lfirst(lc);
+
+			if (strcmp(def->defname, "column_name") == 0)
+			{
+				colname = defGetString(def);
+				break;
+			}
+		}
+
+		appendStringInfoString(buf, quote_identifier(colname));
+
+		*retrieved_attrs = lappend_int(*retrieved_attrs, i + 1);
+	}
+
+	/* Don't generate bad syntax for zero-column relation. */
+	if (first)
+		appendStringInfoString(buf, "NULL");
+
+	/*
+	 * Construct FROM clause
+	 */
+	appendStringInfoString(buf, " FROM ");
+	deparseRelation(buf, rel);
+	appendStringInfo(buf, " TABLESAMPLE SYSTEM(%f)", (100.0 * sample_frac));
+}
+
 /*
  * Construct a simple "TRUNCATE rel" statement
  */
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index fc3ce6a53a2..02be48ec82c 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -121,7 +121,8 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
 			strcmp(def->defname, "updatable") == 0 ||
 			strcmp(def->defname, "truncatable") == 0 ||
 			strcmp(def->defname, "async_capable") == 0 ||
-			strcmp(def->defname, "keep_connections") == 0)
+			strcmp(def->defname, "keep_connections") == 0 ||
+			strcmp(def->defname, "sample") == 0)
 		{
 			/* these accept only boolean values */
 			(void) defGetBoolean(def);
@@ -252,6 +253,10 @@ InitPgFdwOptions(void)
 		{"keep_connections", ForeignServerRelationId, false},
 		{"password_required", UserMappingRelationId, false},
 
+		/* sampling is available on both server and table */
+		{"sample", ForeignServerRelationId, false},
+		{"sample", ForeignTableRelationId, false},
+
 		/*
 		 * sslcert and sslkey are in fact libpq options, but we repeat them
 		 * here to allow them to appear in both foreign server context (when
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 56654844e8f..be04ceb01ef 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4961,6 +4961,68 @@ postgresAnalyzeForeignTable(Relation relation,
 	return true;
 }
 
+/*
+ * postgresCountTuplesForForeignTable
+ *		Count tuples in foreign table (just get pg_class.reltuples).
+ *
+ * Note: It's unclear how accurate reltuples is, maybe size that using
+ * relpages and simple assumptions (1 tuples per page, ...)? Using
+ * tsm_system_rows wold make this somewhat unnecessary.
+ */
+static double
+postgresCountTuplesForForeignTable(Relation relation)
+{
+	ForeignTable *table;
+	UserMapping *user;
+	PGconn	   *conn;
+	StringInfoData sql;
+	PGresult   *volatile res = NULL;
+	double		reltuples = -1;
+
+	/*
+	 * Now we have to get the number of pages.  It's annoying that the ANALYZE
+	 * API requires us to return that now, because it forces some duplication
+	 * of effort between this routine and postgresAcquireSampleRowsFunc.  But
+	 * it's probably not worth redefining that API at this point.
+	 */
+
+	/*
+	 * Get the connection to use.  We do the remote access as the table's
+	 * owner, even if the ANALYZE was started by some other user.
+	 */
+	table = GetForeignTable(RelationGetRelid(relation));
+	user = GetUserMapping(relation->rd_rel->relowner, table->serverid);
+	conn = GetConnection(user, false, NULL);
+
+	/*
+	 * Construct command to get page count for relation.
+	 */
+	initStringInfo(&sql);
+	deparseAnalyzeTuplesSql(&sql, relation);
+
+	/* In what follows, do not risk leaking any PGresults. */
+	PG_TRY();
+	{
+		res = pgfdw_exec_query(conn, sql.data, NULL);
+		if (PQresultStatus(res) != PGRES_TUPLES_OK)
+			pgfdw_report_error(ERROR, res, conn, false, sql.data);
+
+		if (PQntuples(res) != 1 || PQnfields(res) != 1)
+			elog(ERROR, "unexpected result from deparseAnalyzeSizeSql query");
+		reltuples = strtod(PQgetvalue(res, 0, 0), NULL);
+	}
+	PG_FINALLY();
+	{
+		if (res)
+			PQclear(res);
+	}
+	PG_END_TRY();
+
+	ReleaseConnection(conn);
+
+	return reltuples;
+}
+
 /*
  * Acquire a random sample of rows from foreign table managed by postgres_fdw.
  *
@@ -4991,6 +5053,12 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
 	unsigned int cursor_number;
 	StringInfoData sql;
 	PGresult   *volatile res = NULL;
+	ListCell   *lc;
+
+	/* sampling enabled by default */
+	bool		do_sample = true;
+	double		sample_frac = -1.0;
+	double		reltuples;
 
 	/* Initialize workspace state */
 	astate.rel = relation;
@@ -5018,20 +5086,74 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
 	user = GetUserMapping(relation->rd_rel->relowner, table->serverid);
 	conn = GetConnection(user, false, NULL);
 
+	/*
+	 * Should we use TABLESAMPLE to collect the remote sample?
+	 */
+	foreach(lc, server->options)
+	{
+		DefElem    *def = (DefElem *) lfirst(lc);
+
+		if (strcmp(def->defname, "sample") == 0)
+		{
+			do_sample = defGetBoolean(def);
+			break;
+		}
+	}
+	foreach(lc, table->options)
+	{
+		DefElem    *def = (DefElem *) lfirst(lc);
+
+		if (strcmp(def->defname, "sample") == 0)
+		{
+			do_sample = defGetBoolean(def);
+			break;
+		}
+	}
+
+	if (do_sample)
+	{
+		reltuples = postgresCountTuplesForForeignTable(relation);
+
+		if ((reltuples <= 0) || (targrows >= reltuples))
+			do_sample = false;
+
+		sample_frac = targrows / reltuples;
+
+		/* Let's sample a bit more, we'll reduce the sample locally. */
+		sample_frac *= 1.25;
+
+		/* Sanity checks. */
+		sample_frac = Min(1.0, Max(0.0, sample_frac));
+
+		/*
+		 * When sampling too large fraction, just read everything.
+		 *
+		 * XXX It's not clear where exactly the threshold is, with slow
+		 * network it may be cheaper to sample even 90%.
+		 */
+		if (sample_frac > 0.5)
+			do_sample = false;
+	}
+
 	/*
 	 * Construct cursor that retrieves whole rows from remote.
 	 */
 	cursor_number = GetCursorNumber(conn);
 	initStringInfo(&sql);
 	appendStringInfo(&sql, "DECLARE c%u CURSOR FOR ", cursor_number);
-	deparseAnalyzeSql(&sql, relation, &astate.retrieved_attrs);
+
+	if (do_sample)
+		deparseAnalyzeSampleSql(&sql, relation, &astate.retrieved_attrs, sample_frac);
+	else
+		deparseAnalyzeSql(&sql, relation, &astate.retrieved_attrs);
+
+	elog(WARNING, "SQL: %s", sql.data);
 
 	/* In what follows, do not risk leaking any PGresults. */
 	PG_TRY();
 	{
 		char		fetch_sql[64];
 		int			fetch_size;
-		ListCell   *lc;
 
 		res = pgfdw_exec_query(conn, sql.data, NULL);
 		if (PQresultStatus(res) != PGRES_COMMAND_OK)
@@ -5120,7 +5242,10 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
 	*totaldeadrows = 0.0;
 
 	/* We've retrieved all living tuples from foreign server. */
-	*totalrows = astate.samplerows;
+	if (do_sample)
+		*totalrows = reltuples;
+	else
+		*totalrows = astate.samplerows;
 
 	/*
 	 * Emit some interesting relation info
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 8ae79e97e44..5e4f732e563 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -208,8 +208,12 @@ extern void deparseDirectDeleteSql(StringInfo buf, PlannerInfo *root,
 								   List *returningList,
 								   List **retrieved_attrs);
 extern void deparseAnalyzeSizeSql(StringInfo buf, Relation rel);
+extern void deparseAnalyzeTuplesSql(StringInfo buf, Relation rel);
 extern void deparseAnalyzeSql(StringInfo buf, Relation rel,
 							  List **retrieved_attrs);
+extern void deparseAnalyzeSampleSql(StringInfo buf, Relation rel,
+									List **retrieved_attrs,
+									double sample_frac);
 extern void deparseTruncateSql(StringInfo buf,
 							   List *rels,
 							   DropBehavior behavior,

---

From: Sofia Kopikova <[email-masked]>
Date: Wed, 16 Feb 2022 15:56:58 +0300

Hello,
 
I find it a great idea to use TABLESAMPLE in postgres_fdw ANALYZE.
Let me offer you some ideas how to resolve you problems.
 
> ones, because that'll work on more systems out of the box.
 
It’s hard to choose between speed and quality, but I think we need
SYSTEM method here. This patch is for speeding-up ANALYZE,
and SYSTEM method will faster than BERNOULLI on fraction
values to 50%.
 
> (e.g. look at relation size and adjust reltuples by current/relpages).
 
I found a query on Stackoverflow (it does similar thing to what
estimate_rel_size does) that may help with it. So that makes
tsm_system_rows unnecessary.
 
----------------------------------------------------------------------
SELECT
    (CASE WHEN relpages = 0 THEN float8 '0'
                ELSE reltuples / relpages END
    * (pg_relation_size(oid) / pg_catalog.current_setting('block_size')::int)
    )::bigint
FROM pg_class c
WHERE c.oid = 'tablename'::regclass;
----------------------------------------------------------------------
 
And one more refactoring note. New function deparseAnalyzeSampleSql
duplicates function deparseAnalyzeSql (is previous in file deparse.c)
except for the new last line. I guess it would be better to add new
parameter — double sample_frac — to existing function
deparseAnalyzeSql and use it as a condition for adding
"TABLESAMPLE SYSTEM..." to SQL query (set it to zero when
do_sample is false). Or you may also add do_sample as a parameter to
deparseAnalyzeSql, but as for me that’s redundantly.
 
Stackoverflow:  https://stackoverflow.com/questions/7943233/fast-way-to-discover-the-row-count-of-a-table-in-postgresql
 
--
Sofia Kopikova
Postgres Professional:  http://www.postgrespro.com
The Russian Postgres Company

---

From: James Finnerty <[email-masked]>
Date: Thu, 15 Dec 2022 16:34:35 +0000

This patch looks good to me.  I have two very minor nits: The inflation of the sample size by 10% is arbitrary but it doesn't seem unreasonable or concerning.  It just makes me curious if there are any known cases that motivated adding this logic.  Secondly, if the earliest non-deprecated version of PostgreSQL supports sampling, then you could optionally remove the logic that tests for that. The affected lines should be unreachable.

---

From: Tom Lane <[email-masked]>
Date: Thu, 15 Dec 2022 11:46:56 -0500

James Finnerty <[email-masked]> writes:
> This patch looks good to me.  I have two very minor nits: The inflation
> of the sample size by 10% is arbitrary but it doesn't seem unreasonable
> or concerning.  It just makes me curious if there are any known cases
> that motivated adding this logic.

I wondered why, too.

> Secondly, if the earliest non-deprecated version of PostgreSQL supports
> sampling, then you could optionally remove the logic that tests for
> that. The affected lines should be unreachable.

We've tried to keep postgres_fdw compatible with quite ancient remote
servers (I think the manual claims back to 8.3, though it's unlikely
anyone's tested that far back lately).  This patch should not move those
goalposts, especially if it's easy not to.

			regards, tom lane

---

From: Tomas Vondra <[email-masked]>
Date: Wed, 21 Dec 2022 13:08:08 +0100

>> that motivated adding this logic.

Not sure about known cases, but the motivation is explained in the
comment before the 10% is applied.

The repluples value is never going to be spot on, and we use that to
determine what fraction of the table to sample (because all built-in
TABLESAMPLE methods only accept fraction to sample, not expected number
of rows).

If pg_class.reltuples is lower (than the actual row count), we'll end up
with sample fraction too high. That's mostly harmless, as we'll then
discard some of the rows locally.

But if the pg_class.reltuples is higher, we'll end up sampling too few
rows (less than targrows). This can happen e.g. after a bulk delete.

Yes, the 10% value is mostly arbitrary, and maybe it's pointless. How
much may the stats change with 10% larger sample? Probably not much, so
is it really solving anything?

Also, maybe it's fixing the issue at the wrong level - if stale
reltuples are an issue, maybe the right fix is making it more accurate
on the source instance. Decrease autovacuum_vacuum_scale_factor, or
maybe also look at pg_stat_all_tables or something.

regards

---

From: Tomas Vondra <[email-masked]>
Date: Sat, 31 Dec 2022 00:03:08 +0100

Pushed.

After thinking about it a bit more I decided to rip out the 10% sampling
rate inflation. While stale reltuples may be an issue, but I couldn't
convince myself this would be an improvement overall, or that this is
the right place to deal with it.

Firstly, if reltuples is too low (i.e. when the table grew), everything
is "fine" - we sample more than targrows and then remove some of that
locally. We end up with same sample size as without this patch - which
is not necessarily the right sample size (more about that in a minute),
but if it was good enough for now ... improving this bit was not the
ambition of this patch.

If reltuples is too high (e.g. right after bulk DELETE) we may end up
sampling too few rows. If it's 1/2 what it should be, we'll end up
sampling only 15k rows instead of 30k rows. But it's unclear how much
should we adjust the value - 10% as was in the patch (kinda based on
autovacuum_analyze_scale_factor being 10%)?

I'd argue that's too low to make any difference - cases that are only
~10% off should work fine (we don't have perfect stats anyway). And for
the really bad cases 10% is not going to make a difference.

And if the reltuples is that off, it'll probably affect even queries
planned on the remote node (directly or pushed down), so I guess the
right fix should be making sure reltuples is not actually off - make
autovacuum more aggressive or whatever.

A thing that bothers me is that calculating sample size for partitioned
tables is a bit cyclic - we look at number of blocks, and divide the
whole sample based on that. And that's generally correlated with the
reltuples, but OTOH it may also be wrong - and not necessarily in the
same way. For example you might delete 90% of a table, which will make
the table overrepresented in the sample. But then reltuples may be quite
accurate thanks to recent vacuum - so fixing this by blindly adjusting
the sampling rate seems misguided. If this turns out to be an issue in
practice, we need to rethink acquire_inherited_sample_rows as a whole
(not just for foreign tables).

regards

---

From: Tom Lane <[email-masked]>
Date: Fri, 30 Dec 2022 23:42:24 -0500

Tomas Vondra <[email-masked]> writes:
> rate inflation.

+1.  I'm not sure if there's anything more we need to do there, but
that didn't seem like that was it.

I notice that the committed patch still has a reference to that hack
though:

+            * Ensure the sampling rate is between 0.0 and 1.0, even after the
+            * 10% adjustment above.  (Clamping to 0.0 is just paranoia.)

Clamping still seems like a wise idea, but the comment is just
confusing now.

Also, I wonder if there is any possibility of ANALYZE failing
with

ERROR:  TABLESAMPLE clause can only be applied to tables and materialized views

I think the patch avoids that, but only accidentally, because
reltuples will be 0 or -1 for a view.  Maybe it'd be a good
idea to pull back relkind along with reltuples, and check
that too?

			regards, tom lane

---

From: Tomas Vondra <[email-masked]>
Date: Sat, 31 Dec 2022 15:16:38 +0100

> Clamping still seems like a wise idea, but the comment is just

Yeah, I missed that reference. Will fix.

> idea to pull back relkind along with reltuples, and check

Not sure. I guess we can rely on reltuples being 0 or -1 in such cases,
but maybe it'd be good to at least mention that in a comment? We're not
going to use other reltuples values for views etc.

regards

---

From: Tom Lane <[email-masked]>
Date: Sat, 31 Dec 2022 12:23:51 -0500

> going to use other reltuples values for views etc.

Would anyone ever point a foreign table at a sequence?  I guess it
would be a weird use-case, but it's possible, or was till now.

			regards, tom lane