Re: postgres_fdw: using TABLESAMPLE to collect remote sample

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