Thread: postgres_fdw: using TABLESAMPLE to collect remote sample

postgres_fdw: using TABLESAMPLE to collect remote sample

From

Tomas Vondra

Date:

11 February 2022, 20:02:37

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

Attachment

postgres-fdw-analyze-sample-20220211.patch

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Tom Lane

Date:

11 February 2022, 20:39:43

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> 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.

This is great if the remote end has TABLESAMPLE, but pre-9.5 servers
don't, and postgres_fdw is supposed to still work with old servers.
So you need some conditionality for that.

            regards, tom lane

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Robert Haas

Date:

11 February 2022, 20:43:31

On Fri, Feb 11, 2022 at 12:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> > 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.
>
> This is great if the remote end has TABLESAMPLE, but pre-9.5 servers
> don't, and postgres_fdw is supposed to still work with old servers.
> So you need some conditionality for that.

I think it's going to be necessary to compromise on that at some
point. I don't, for example, think it would be reasonable for
postgres_fdw to have detailed knowledge of which operators can be
pushed down as a function of the remote PostgreSQL version. Nor do I
think that we care about whether this works at all against, say,
PostgreSQL 8.0. I'm not sure where it's reasonable to draw a line and
say we're not going to expend any more effort, and maybe 15 with 9.5
is a small enough gap that we still care at least somewhat about
compatibility. But even that is not 100% obvious to me.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Tom Lane

Date:

11 February 2022, 21:06:01

Robert Haas <robertmhaas@gmail.com> writes:
> I think it's going to be necessary to compromise on that at some
> point.

Sure.  The existing postgres_fdw documentation [1] already addresses
this issue:

    postgres_fdw can be used with remote servers dating back to PostgreSQL
    8.3. Read-only capability is available back to 8.1. A limitation
    however is that postgres_fdw generally assumes that immutable built-in
    functions and operators are safe to send to the remote server for
    execution, if they appear in a WHERE clause for a foreign table. Thus,
    a built-in function that was added since the remote server's release
    might be sent to it for execution, resulting in “function does not
    exist” or a similar error. This type of failure can be worked around
    by rewriting the query, for example by embedding the foreign table
    reference in a sub-SELECT with OFFSET 0 as an optimization fence, and
    placing the problematic function or operator outside the sub-SELECT.

While I'm not opposed to moving those goalposts at some point,
I think pushing them all the way up to 9.5 for this one easily-fixed
problem is not very reasonable.

Given other recent discussion, an argument could be made for moving
the cutoff to 9.2, on the grounds that it's too hard to test against
anything older.  But that still leaves us needing a version check
if we want to use TABLESAMPLE.

            regards, tom lane

[1] https://www.postgresql.org/docs/devel/postgres-fdw.html#id-1.11.7.45.17

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Robert Haas

Date:

11 February 2022, 21:13:46

On Fri, Feb 11, 2022 at 1:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> While I'm not opposed to moving those goalposts at some point,
> I think pushing them all the way up to 9.5 for this one easily-fixed
> problem is not very reasonable.
>
> Given other recent discussion, an argument could be made for moving
> the cutoff to 9.2, on the grounds that it's too hard to test against
> anything older.  But that still leaves us needing a version check
> if we want to use TABLESAMPLE.

OK, thanks.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Tomas Vondra

Date:

11 February 2022, 22:52:46

On 2/11/22 19:13, Robert Haas wrote:
> On Fri, Feb 11, 2022 at 1:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> While I'm not opposed to moving those goalposts at some point,
>> I think pushing them all the way up to 9.5 for this one easily-fixed
>> problem is not very reasonable.
>>
>> Given other recent discussion, an argument could be made for moving
>> the cutoff to 9.2, on the grounds that it's too hard to test against
>> anything older.  But that still leaves us needing a version check
>> if we want to use TABLESAMPLE.
> 
> OK, thanks.
> 

Yeah, I think checking server_version is fairly simple. Which is mostly
why I haven't done anything about that in the submitted patch, because
the other issues (what fraction to sample) seemed more important.

regards

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

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Sofia Kopikova

Date:

16 February 2022, 15:56:58

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.

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:

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

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

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

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

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Tomas Vondra

Date:

18 February 2022, 16:28:48

Hi,

here's a slightly updated version of the patch series. The 0001 part
adds tracking of server_version_num, so that it's possible to enable
other features depending on it. In this case it's used to decide whether
TABLESAMPLE is supported.

The 0002 part modifies the sampling. I realized we can do something
similar even on pre-9.5 releases, by running "WHERE random() < $1". Not
perfect, because it still has to read the whole table, but still better
than also sending it over the network.

There's a "sample" option for foreign server/table, which can be used to
disable the sampling if needed.

A simple measurement on a table with 10M rows, on localhost.

  old:        6600ms
  random:      450ms
  tablesample:  40ms (system)
  tablesample: 200ms (bernoulli)

Local analyze takes ~190ms, so that's quite close.


regards

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

On 2/22/22 21:12, Tomas Vondra wrote:
> On 2/22/22 01:36, Fujii Masao wrote:
>>
>>
>> On 2022/02/18 22:28, Tomas Vondra wrote:
>>> Hi,
>>>
>>> here's a slightly updated version of the patch series.
>>
>> Thanks for updating the patches!
>>
>>
>>> The 0001 part
>>> adds tracking of server_version_num, so that it's possible to enable
>>> other features depending on it.
>>
>> Like configure_remote_session() does, can't we use PQserverVersion()
>> instead of implementing new function GetServerVersion()?
>>
> 
> Ah! My knowledge of libpq is limited, so I wasn't sure this function
> exists. It'll simplify the patch.
> 

And here's the slightly simplified patch, without the part adding the
unnecessary GetServerVersion() function.


regards

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

Attachment

0001-postgres_fdw-sample-data-on-remote-node-for-20220222.patch

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Andres Freund

Date:

22 March 2022, 04:18:20

Hi,

On 2022-02-23 00:51:24 +0100, Tomas Vondra wrote:
> And here's the slightly simplified patch, without the part adding the
> unnecessary GetServerVersion() function.

Doesn't apply anymore: http://cfbot.cputube.org/patch_37_3535.log
Marked as waiting-on-author.

Greetings,

Andres Freund

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Tom Lane

Date:

17 July 2022, 00:57:44

Andres Freund <andres@anarazel.de> writes:
> On 2022-02-23 00:51:24 +0100, Tomas Vondra wrote:
>> And here's the slightly simplified patch, without the part adding the
>> unnecessary GetServerVersion() function.

> Doesn't apply anymore: http://cfbot.cputube.org/patch_37_3535.log
> Marked as waiting-on-author.

Here's a rebased version that should at least pass regression tests.

I've not reviewed it in any detail, but:

* I'm not really on board with defaulting to SYSTEM sample method,
and definitely not on board with not allowing any other choice.
We don't know enough about the situation in a remote table to be
confident that potentially-nonrandom sampling is OK.  So personally
I'd default to BERNOULLI, which is more reliable and seems plenty fast
enough given your upthread results.  It could be an idea to extend the
sample option to be like "sample [ = methodname ]", if you want more
flexibility, but I'd be happy leaving that for another time.

* The code depending on reltuples is broken in recent server versions,
and might give useless results in older ones too (if reltuples =
relpages = 0).  Ideally we'd retrieve all of reltuples, relpages, and
pg_relation_size(rel), and do the same calculation the planner does.
Not sure if pg_relation_size() exists far enough back though.

* Copying-and-pasting all of deparseAnalyzeSql (twice!) seems pretty
bletcherous.  Why not call that and then add a WHERE clause to its
result, or just add some parameters to it so it can do that itself?

* More attention to updating relevant comments would be appropriate,
eg here you've not bothered to fix the adjacent falsified comment:

     /* We've retrieved all living tuples from foreign server. */
-    *totalrows = astate.samplerows;
+    if (do_sample)
+        *totalrows = reltuples;
+    else
+        *totalrows = astate.samplerows;

* Needs docs obviously.  I'm not sure if the existing regression
testing covers the new code adequately or if we need more cases.

Having said that much, I'm going to leave it in Waiting on Author
state.

            regards, tom lane

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 8f4d8a5022..cdf12e53e8 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -2297,6 +2297,31 @@ deparseAnalyzeSizeSql(StringInfo buf, Relation rel)
     appendStringInfo(buf, "::pg_catalog.regclass) / %d", BLCKSZ);
 }

+/*
+ * Construct SELECT statement to acquire the number of rows in given relation.
+ *
+ * We just need an estimate here, so consult pg_class.reltuples instead
+ * of doing anything as expensive as COUNT(*).
+ *
+ * Note: Maybe this should compare relpages and current relation size
+ * and adjust reltuples accordingly, like the planner does?
+ * XXX: it needs some work anyway, because it won't do anything sane
+ * for a never-analyzed table with reltuples = -1.
+ */
+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.
  *
@@ -2358,6 +2383,135 @@ 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
+deparseAnalyzeTableSampleSql(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 SELECT statement to acquire sample rows of given relation,
+ * by sampling a fraction of the table using random().
+ *
+ * SELECT command is appended to buf, and list of columns retrieved
+ * is returned to *retrieved_attrs.
+ */
+void
+deparseAnalyzeLegacySampleSql(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, " WHERE random() < %f", sample_frac);
+}
+
 /*
  * Construct a simple "TRUNCATE rel" statement
  */
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 5f2ef88cf3..482c21032b 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9532,7 +9532,7 @@ DO $d$
     END;
 $d$;
 ERROR:  invalid option "password"
-HINT:  Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr,
port,options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout,
sslmode,sslcompression, sslcert, sslkey, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer, ssl_min_protocol_version,
ssl_max_protocol_version,gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost,
fdw_tuple_cost,extensions, updatable, truncatable, fetch_size, batch_size, async_capable, parallel_commit,
keep_connections
+HINT:  Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr,
port,options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout,
sslmode,sslcompression, sslcert, sslkey, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer, ssl_min_protocol_version,
ssl_max_protocol_version,gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost,
fdw_tuple_cost,extensions, updatable, truncatable, fetch_size, batch_size, async_capable, parallel_commit,
keep_connections,sample 
 CONTEXT:  SQL statement "ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')"
 PL/pgSQL function inline_code_block line 3 at EXECUTE
 -- If we add a password for our user mapping instead, we should get a different
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 572591a558..3749d4701a 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -122,7 +122,8 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
             strcmp(def->defname, "truncatable") == 0 ||
             strcmp(def->defname, "async_capable") == 0 ||
             strcmp(def->defname, "parallel_commit") == 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);
@@ -254,6 +255,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 955a428e3d..cd2088e2ae 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;
+    int            server_version_num;
+    bool        do_sample = true;    /* enabled by default */
+    bool        use_tablesample = true;
+    double        sample_frac = -1.0;
+    double        reltuples;

     /* Initialize workspace state */
     astate.rel = relation;
@@ -5018,20 +5086,81 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
     user = GetUserMapping(relation->rd_rel->relowner, table->serverid);
     conn = GetConnection(user, false, NULL);

+    /* We'll need server version, so fetch it now. */
+    server_version_num = PQserverVersion(conn);
+
+    /* disable tablesample on old remote servers */
+    if (server_version_num < 95000)
+        use_tablesample = false;
+
+    /*
+     * 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 && use_tablesample)
+        deparseAnalyzeTableSampleSql(&sql, relation, &astate.retrieved_attrs, sample_frac);
+    else if (do_sample)
+        deparseAnalyzeLegacySampleSql(&sql, relation, &astate.retrieved_attrs, sample_frac);
+    else
+        deparseAnalyzeSql(&sql, relation, &astate.retrieved_attrs);

     /* 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)
@@ -5119,7 +5248,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 21f2b20ce8..b0d9cf4298 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -211,8 +211,15 @@ 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 deparseAnalyzeTableSampleSql(StringInfo buf, Relation rel,
+                                         List **retrieved_attrs,
+                                         double sample_frac);
+extern void deparseAnalyzeLegacySampleSql(StringInfo buf, Relation rel,
+                                          List **retrieved_attrs,
+                                          double sample_frac);
 extern void deparseTruncateSql(StringInfo buf,
                                List *rels,
                                DropBehavior behavior,

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Tomas Vondra

Date:

18 July 2022, 14:36:23

On 7/16/22 23:57, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2022-02-23 00:51:24 +0100, Tomas Vondra wrote:
>>> And here's the slightly simplified patch, without the part adding the
>>> unnecessary GetServerVersion() function.
> 
>> Doesn't apply anymore: http://cfbot.cputube.org/patch_37_3535.log
>> Marked as waiting-on-author.
> 
> Here's a rebased version that should at least pass regression tests.
>

Thanks. I've been hacking on this over the past few days, and by
coincidence I've been improving exactly the stuff you've pointed out in
the review. 0001 is just the original patch rebased, 0002 includes all
the various changes.

> I've not reviewed it in any detail, but:
> 
> * I'm not really on board with defaulting to SYSTEM sample method,
> and definitely not on board with not allowing any other choice.
> We don't know enough about the situation in a remote table to be
> confident that potentially-nonrandom sampling is OK.  So personally
> I'd default to BERNOULLI, which is more reliable and seems plenty fast
> enough given your upthread results.  It could be an idea to extend the
> sample option to be like "sample [ = methodname ]", if you want more
> flexibility, but I'd be happy leaving that for another time.
> 

I agree, I came roughly to the same conclusion, so I replaced the simple
on/off option with these options:

off - Disables the remote sampling, so we just fetch everything and do
sampling on the local node, just like today.

random - Remote sampling, but "naive" implementation using random()
function. The advantage is this reduces the amount of data we need to
transfer, but it still reads the whole table. This should work for all
server versions, I believe.

system - TABLESAMPLE system method.

bernoulli - TABLESAMOLE bernoulli (default for 9.5+)

auto - picks bernoulli on 9.5+, random on older servers.

I'm not sure about custom TABLESAMPLE methods - that adds more
complexity to detect if it's installed, it's trickier to decide what's
the best choice (for "auto" to make decide), and the parameter is also
different (e.g. system_rows uses number of rows vs. sampling rate).

> * The code depending on reltuples is broken in recent server versions,
> and might give useless results in older ones too (if reltuples =
> relpages = 0).  Ideally we'd retrieve all of reltuples, relpages, and
> pg_relation_size(rel), and do the same calculation the planner does.
> Not sure if pg_relation_size() exists far enough back though.
> 

Yes, I noticed that too, and the reworked code should deal with this
reltuples=0 (by just disabling remote sampling).

I haven't implemented the reltuples/relpages correction yet, but I don't
think compatibility would be an issue - deparseAnalyzeSizeSql() already
calls pg_relation_size(), after all.

FWIW it seems a bit weird being so careful about adjusting reltuples,
when acquire_inherited_sample_rows() only really looks at relpages when
deciding how many rows to sample from each partition. If our goal is to
use a more accurate reltuples, maybe we should do that in the first step
already. Otherwise we may end up build with a sample that does not
reflect sizes of the partitions correctly.

Of course, the sample rate also matters for non-partitioned tables.

> * Copying-and-pasting all of deparseAnalyzeSql (twice!) seems pretty
> bletcherous.  Why not call that and then add a WHERE clause to its
> result, or just add some parameters to it so it can do that itself?
> 

Right. I ended up refactoring this into a single function, with a
"method" parameter that determines if/how we do the remote sampling.

> * More attention to updating relevant comments would be appropriate,
> eg here you've not bothered to fix the adjacent falsified comment:
> 
>      /* We've retrieved all living tuples from foreign server. */
> -    *totalrows = astate.samplerows;
> +    if (do_sample)
> +        *totalrows = reltuples;
> +    else
> +        *totalrows = astate.samplerows;
> 

Yep, fixed.

> * Needs docs obviously.  I'm not sure if the existing regression
> testing covers the new code adequately or if we need more cases.
> 

Yep, I added the "sampling_method" to postgres-fdw.sgml.

> Having said that much, I'm going to leave it in Waiting on Author
> state.

Thanks. I'll switch this to "needs review" now.

regards

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

Hi!

Here's an updated patch, including all the changes proposed by Tom in
his review. There were two more points left [1], namely:

1) The useless test added to postgres_fdw.sql consuming a lot of time.

I decided to just rip this out and replace it with a much simpler test,
that simply changes the sampling method and does ANALYZE. Ideally we'd
also verify we actually generated the right SQL query to be executed on
the remote server, but there doesn't seem to be a good way to do that
(e.g. we can't do EXPLAIN to show the "Remote SQL" on the "ANALYZE").

So I guess just changing the method and running ANALYZE will have to be
enough for now (coverage report should at least tell us we got to all
the SQL variants).


2) the logic disabling sampling when the fraction gets too high

I based the condition on the absolute number of "unsampled" rows, Tom
suggested maybe it should be just a simple condition on sample_frac
(e.g. 95%). But neither of us was sure what would be a good value.

So I did a couple tests to get at least some idea what would be a good
threshold, and it turns out the threshold is mostly useless. I'll
discuss the measurements I did in a bit (some of the findings are a bit
amusing or even hilarious, actually), but the main reasons are:

- The sampling overhead is negligible (compared to transferring the
data, even on localhost), and the behavior is very smooth. So almost
never exceed reading everything, unless sample_frac >= 0.99 and even
there it's a tiny difference. So there's almost no risk, I think.

- For small tables it doesn't really matter. It's going to be fast
anyway, and the difference between reading 10000 or 11000 rows is going
to be just noise. Who cares if this takes 10 or 11 ms ...

- For large tables, we'll never even get to these high sample_frac
values. Imagine a table with 10M rows - the highers stats target we
allow is 10k, and the sample size is 300 * target, so 3M rows. It
doesn't matter if the condition is 0.95 or 0.99, because for this table
we'll never ask for a sample above 30%.

- For the tables in between it might be more relevant, but the simple
truth is that reading the row and sampling it remotely is way cheaper
than the network transfer, even if on localhost. The data suggest that
reading+sampling a row costs ~0.2us at most, but sending it is ~1.5us
(localhost) or ~5.5us (local network).

So I just removed the threshold from the patch, and we'll request
sampling even with sample_frac=100% (if that happens).


sampling test
-------------

I did a simple test to collect some data - create a table, and sample
various fractions in the ways discussed in this patch - either locally
or through a FDW.

This required a bit of care to ensure the sampling happens in the right
place (and e.g. we don't push the random() or tablesample down), which I
did by pointing the FDW table not to a remote table, but to a view with
an optimization fence (OFFSET 0). See the run-local.sh script.

The script populates the table with different numbers of rows, samples
different fractions of it, etc. I also did this from a different
machine, to see what a bit more network latency would do (that's what
run-remote.sh is for).


results (fdw-sampling-test.pdf)
-------------------------------

The attached PDF shows the results - first page is for the foreign table
in the same instance (i.e. localhost, latency ~0.01ms), second page is
for FDW pointing to a machine in the same network (latency ~0.1ms).

Left column is always the table "directly", right column is through the
FDW. On the x-axis is the fraction of the table we sample, y-axis is
duration in milliseconds. "full" means "reading everything" (i.e. what
the FDW does now), the other options should be clear I think.

The first two dataset sizes (10k and 10M rows) are tiny (10M is ~2GB),
and fit into RAM, which is 8GB. The 100M is ~21GB, so much larger.

In the "direct" (non-FDW) sampling, the various sampling methods start
losing to seqscan fairly soon - "random" is consistently slower,
"bernoulli" starts losing at ~30%, "system" as ~80%. This is not very
surprising, particularly for bernoulli/random which actually read all
the rows anyway. But the overhead is pretty limited to ~30% on top of
the seqscan.

But in the "FDW" sampling (right column), it's entirely different story,
and all the methods clearly win over just transferring everything and
only then doing the sampling.

Who cares if the remote sampling means means we have to pay 0.2us
instead of 0.15us (per row), when the transfer costs 1.5us per row?

The 100M case shows an interesting behavior for the "system" method,
which quickly spikes to ~2x of the "full" method when sampling ~20% of
the table, and then gradually improves again.

My explanation is that this is due to "system" making the I/O patter
increasingly more random, because it jumps blocks in a way that makes
readahead impossible. And then as the fraction increases, it becomes
more sequential again.

All the other methods are pretty much equal to just scanning everything
sequentially, and sampling rows one by one.

The "system" method in TABLESAMPLE would probably benefit from explicit
prefetching, I guess. For ANALYZE this probably is not a huge, as we'll
never sample this large fraction for large tables (for 100M rows we peak
at ~3% with target 10000, which is way before the peak). And for smaller
tables we're more likely to hit cache (which is why the smaller data
sets don't have this issue). But for explicit TABLESAMPLE queries that
may not be the case.

Although, ANALYZE uses something like "system" to sample rows too, no?

However, even this is not an issue for the FDW case - in that case it
still clearly wins over the current "local sample" approach, because
transferring the data is so expensive which makes the "peak" into a tiny
hump.

The second page (different machine, local network) tells the same story,
except that the differences are even clearer.


ANALYZE test
------------

So I changed the patch, and did a similar test by running ANALYZE either
on the local or foreign table, using the different sampling methods.
This does not require the hacks to prevent pushdown etc. but it also
means we can't determine sample_frac directly, only through statistics
target (which is capped to 10k).

In this case, "local" means ANALYZE on the local table (which you might
think of as the "baseline"), and "off" means reading all data without
remote sampling.

For the two smaller data sets (10k and 10M rows), the benefits are
pretty obvious. We're very close to the "local" results, because we save
a lot on copying only some of the rows. For 10M we only get to ~30%
before we hit target=10k, which is we don't see it get closer to "off".

But now we get to the *hilarious* thing - if you look at the 10M result,
you may notice that *all* the sampling methods beat ANALYZE on the local
table.

For "system" (which wins from the very beginning) we might make some
argument that the algorithm is simpler than what ANALYZE does, skips
blocks differently, etc. - perhaps ...

But bernoulli/random are pretty much ideal sampling, reading/sampling
all rows. And yet both methods start winning after crossing ~1% on this
tables. In other words, it's about 3x faster to ANALYZE a table through
FDW than directly ;-)

Anyway, those issues have impact on this patch, I think. I believe the
results show what the patch does is reasonable.


regards

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

Hi,

here's two minor postgres_fdw patches, addressing the two issues
discussed last week.


1) 0001-Fix-stale-comment-in-postgres_fdw.patch

The first one cleans up the comment referencing the sample rate
adjustment. While doing that, I realized without the adjustment we
should never get sample_rate outside the valid range because we do:

    if ((reltuples <= 0) || (targrows >= reltuples))
        method = ANALYZE_SAMPLE_OFF;

So I removed the clamping, or rather replaced it with an assert.


2) 0002-WIP-check-relkind-in-FDW-analyze.patch

The second patch adds the relkind check, so that we only issue
TABLESAMPLE on valid relation types (tables, matviews). But I'm not sure
we actually need it - the example presented earlier was foreign table
pointing to a sequence. But that actually works fine even without this
patch, because fore sequences we have reltuples=1, which disables
sampling due to the check mentioned above (because targrows >= 1).

The other issue with this patch is that it seems wrong to check the
relkind value locally - instead we should check this on the remote
server, because that's where we'll run TABLESAMPLE. Currently there are
no differences between versions, but what if we implement TABLESAMPLE
for another relkind in the future? Then we'll either fail to use
sampling or (worse) we'll try using TABLESAMPLE for unsupported relkind,
depending on which of the servers has newer version.


regards

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

I've pushed the comment/assert cleanup.

Here's a cleaned up version of the relkind check. This is almost
identical to the patch from yesterday (plus the renames and updates of
comments for modified functions).

The one difference is that I realized the relkind check does not
actually say we can't do sampling - it just means we can't use
TABLESAMPLE to do it. We could still use "random()" ...

Furthermore, I don't think we should silently disable sampling when the
user explicitly requests TABLESAMPLE by specifying bernoulli/system for
the table - IMHO it's less surprising to just fail in that case.

So we now do this:

    if (!can_tablesample && (method == ANALYZE_SAMPLE_AUTO))
        method = ANALYZE_SAMPLE_RANDOM;

Yes, we may still disable sampling when reltuples is -1, 0 or something
like that. But that's a condition that is expected for new relations and
likely to fix itself, which is not the case for relkind.

Of course, all relkinds that don't support TABLESAMPLE currently have
reltuples value that will disable sampling anyway (e.g. views have -1).
So we won't actually fallback to random() anyway, because we can't
calculate the sample fraction.

That's a bit annoying for foreign tables pointing at a view, which is a
more likely use case than table pointing at a sequence. And likely more
of an issue, because views may return a many rows (while sequences have
only a single row).

But I realized we could actually still do "random()" sampling:

    SELECT * FROM t ORDER BY random() LIMIT $X;

where $X is the target number of rows for sample for the table. Which
would result in plans like this (given sufficient work_mem values)

                              QUERY PLAN
  -------------------------------------------------------------------
   Limit (actual rows=30000 loops=1)
     ->  Sort (actual rows=30000 loops=1)
           Sort Key: (random())
           Sort Method: top-N heapsort  Memory: 3916kB
           ->  Append (actual rows=1000000 loops=1)

Even with lower work_mem values this would likely be a win, due to
saving on network transfers.


regards

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

Attachment

0001-Check-relkind-before-using-TABLESAMPLE-in-postgres_f.patch

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Tom Lane

Date:

06 January 2023, 19:58:52

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> The one difference is that I realized the relkind check does not
> actually say we can't do sampling - it just means we can't use
> TABLESAMPLE to do it. We could still use "random()" ...

> Furthermore, I don't think we should silently disable sampling when the
> user explicitly requests TABLESAMPLE by specifying bernoulli/system for
> the table - IMHO it's less surprising to just fail in that case.

Agreed on both points.  This patch looks good to me.

> Of course, all relkinds that don't support TABLESAMPLE currently have
> reltuples value that will disable sampling anyway (e.g. views have -1).
> So we won't actually fallback to random() anyway, because we can't
> calculate the sample fraction.
> That's a bit annoying for foreign tables pointing at a view, which is a
> more likely use case than table pointing at a sequence.

Right, that's a case worth being concerned about.

> But I realized we could actually still do "random()" sampling:
>     SELECT * FROM t ORDER BY random() LIMIT $X;

Hmm, interesting idea, but it would totally bollix our correlation
estimates.  Not sure that those are worth anything for remote views,
but still...

            regards, tom lane

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Tomas Vondra

Date:

07 January 2023, 01:41:48

On 1/6/23 17:58, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
>> The one difference is that I realized the relkind check does not
>> actually say we can't do sampling - it just means we can't use
>> TABLESAMPLE to do it. We could still use "random()" ...
> 
>> Furthermore, I don't think we should silently disable sampling when the
>> user explicitly requests TABLESAMPLE by specifying bernoulli/system for
>> the table - IMHO it's less surprising to just fail in that case.
> 
> Agreed on both points.  This patch looks good to me.
> 

Good, I'll get this committed.The "ORDER BY random()" idea is a possible
improvement, can be discussed on it's own.

>> Of course, all relkinds that don't support TABLESAMPLE currently have
>> reltuples value that will disable sampling anyway (e.g. views have -1).
>> So we won't actually fallback to random() anyway, because we can't
>> calculate the sample fraction.
>> That's a bit annoying for foreign tables pointing at a view, which is a
>> more likely use case than table pointing at a sequence.
> 
> Right, that's a case worth being concerned about.
> 
>> But I realized we could actually still do "random()" sampling:
>>     SELECT * FROM t ORDER BY random() LIMIT $X;
> 
> Hmm, interesting idea, but it would totally bollix our correlation
> estimates.  Not sure that those are worth anything for remote views,
> but still...

But isn't that an issue that we already have? I mean, if we do ANALYZE
on a foreign table pointing to a view, we fetch all the results. But if
the view does not have a well-defined ORDER BY, a trivial plan change
may change the order of results and thus the correlation.

Actually, how is a correlation even defined for a view?

It's true this "ORDER BY random()" thing would make it less stable, as
it would change the correlation on every run. Although - the calculated
correlation is actually quite stable, because it's guaranteed to be
pretty close to 0 because we make the order random.

Maybe that's actually better than the current state where it depends on
the plan? Why not to look at the relkind and just set correlation to 0.0
in such cases?

But if we want to restore that current behavior (where it depends on the
actual query plan), we could do something like this:

   SELECT * FROM the_remote_view ORDER BY row_number() over ();

But yeah, this makes the remote sampling more expensive. Probably still
a win because of network costs, but not great.

regards

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

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Tomas Vondra

Date:

07 January 2023, 17:07:58

On 1/6/23 23:41, Tomas Vondra wrote:
> On 1/6/23 17:58, Tom Lane wrote:
>> Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
>>> The one difference is that I realized the relkind check does not
>>> actually say we can't do sampling - it just means we can't use
>>> TABLESAMPLE to do it. We could still use "random()" ...
>>
>>> Furthermore, I don't think we should silently disable sampling when the
>>> user explicitly requests TABLESAMPLE by specifying bernoulli/system for
>>> the table - IMHO it's less surprising to just fail in that case.
>>
>> Agreed on both points.  This patch looks good to me.
>>
> 
> Good, I'll get this committed.The "ORDER BY random()" idea is a possible
> improvement, can be discussed on it's own.
> 

Pushed.

>>> Of course, all relkinds that don't support TABLESAMPLE currently have
>>> reltuples value that will disable sampling anyway (e.g. views have -1).
>>> So we won't actually fallback to random() anyway, because we can't
>>> calculate the sample fraction.
>>> That's a bit annoying for foreign tables pointing at a view, which is a
>>> more likely use case than table pointing at a sequence.
>>
>> Right, that's a case worth being concerned about.
>>
>>> But I realized we could actually still do "random()" sampling:
>>>     SELECT * FROM t ORDER BY random() LIMIT $X;
>>
>> Hmm, interesting idea, but it would totally bollix our correlation
>> estimates.  Not sure that those are worth anything for remote views,
>> but still...
> 
> But isn't that an issue that we already have? I mean, if we do ANALYZE
> on a foreign table pointing to a view, we fetch all the results. But if
> the view does not have a well-defined ORDER BY, a trivial plan change
> may change the order of results and thus the correlation.
> 
> Actually, how is a correlation even defined for a view?
> 
> It's true this "ORDER BY random()" thing would make it less stable, as
> it would change the correlation on every run. Although - the calculated
> correlation is actually quite stable, because it's guaranteed to be
> pretty close to 0 because we make the order random.
> 
> Maybe that's actually better than the current state where it depends on
> the plan? Why not to look at the relkind and just set correlation to 0.0
> in such cases?
> 
> But if we want to restore that current behavior (where it depends on the
> actual query plan), we could do something like this:
> 
>    SELECT * FROM the_remote_view ORDER BY row_number() over ();
> 
> But yeah, this makes the remote sampling more expensive. Probably still
> a win because of network costs, but not great.
> 

I've been thinking about this a bit more, and I'll consider submitting a
patch for the next CF. IMHO it's probably better to accept correlation
being 0 in these cases - it's more representative of what we know about
the view / plan output (or rather lack of knowledge).

However, maybe views are not the best / most common example to think
about. I'd imagine it's much more common to reference a regular table,
but the table gets truncated / populated quickly, and/or the autovacuum
workers are busy so it takes time to update reltuples. But in this case
it's also quite simple to fix the correlation by just ordering by ctid
(which I guess we might do based on the relkind).

There's a minor issue with partitioned tables, with foreign partitions
pointing to remote views. This is kinda broken, because the sample size
for individual relations is determined based on relpages. But that's 0
for views, so these partitions get ignored when building the sample. But
that's a pre-existing issue.


regards

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

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

From

Tom Lane

Date:

07 January 2023, 18:20:10

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> However, maybe views are not the best / most common example to think
> about. I'd imagine it's much more common to reference a regular table,
> but the table gets truncated / populated quickly, and/or the autovacuum
> workers are busy so it takes time to update reltuples. But in this case
> it's also quite simple to fix the correlation by just ordering by ctid
> (which I guess we might do based on the relkind).

> There's a minor issue with partitioned tables, with foreign partitions
> pointing to remote views. This is kinda broken, because the sample size
> for individual relations is determined based on relpages. But that's 0
> for views, so these partitions get ignored when building the sample. But
> that's a pre-existing issue.

I wonder if we should stop consulting reltuples directly at all,
and instead do

"EXPLAIN SELECT * FROM remote_table"

to get the remote planner's estimate of the number of rows involved.
Even for a plain table, that's likely to be a better number.

            regards, tom lane