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
