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