Re: Import Statistics in postgres_fdw before resorting to sampling.

Hi, thanks for working on this. Generally I think that this is a good
idea to avoid fetching rows from a foreign table to compute statistics
that may already be available on the foreign server.

I started reviewing the v7 patch and here are my initial comments. I
still want to do another round of review and run some more tests.

+ if (fdwroutine->ImportStatistics != NULL &&
+ fdwroutine->StatisticsAreImportable != NULL &&
+ fdwroutine->StatisticsAreImportable(onerel))
+ import_stats = true;
+ else
+ {
+ if (fdwroutine->AnalyzeForeignTable != NULL)
+ ok = fdwroutine->AnalyzeForeignTable(onerel,
+ &acquirefunc,
+ &relpages);
+
+ if (!ok)
+ {
+ ereport(WARNING,
+ errmsg("skipping \"%s\" -- cannot analyze this foreign table.",
+ RelationGetRelationName(onerel)));
+ relation_close(onerel, ShareUpdateExclusiveLock);
+ return;
+ }
+ }
+
if (fdwroutine->AnalyzeForeignTable != NULL)
ok = fdwroutine->AnalyzeForeignTable(onerel,
&acquirefunc,
&relpages);

if (!ok)
{
ereport(WARNING,
(errmsg("skipping \"%s\" --- cannot analyze this foreign table",
RelationGetRelationName(onerel))));
relation_close(onerel, ShareUpdateExclusiveLock);
return;
}

It seems that we have the same code within the else branch after the if/else
check, is this correct?

---

+ * Every row of the result should be an attribute that we specificially

s/specificially/specifically

---

+ if (TupleDescAttr(tupdesc, i)->attisdropped)
+ continue;
+
+ /* Ignore generated columns. */
+ if (TupleDescAttr(tupdesc, i)->attgenerated)
+ continue;
+
+ attname = NameStr(TupleDescAttr(tupdesc, i)->attname);

Wouldn't be better to call TupleDescAttr a single time and save the value?
Form_pg_attribute attr = TupleDescAttr(tupdesc, i - 1);

/* Ignore dropped columns. */
if (attr->attisdropped)
continue;
...

--
Matheus Alcantara
EDB: https://www.enterprisedb.com