>
>
> Looks pretty good. You seem to have added a blank line at the end of
> postgres_fdw.c, which should be stripped back out.
>
Done.
>
> > I'm not too keen on having *no* new regression tests, but defer to your
> > judgement.
>
> So... I'm not *opposed* to regression tests. I just don't see a
> reasonable way to write one. If you've got an idea, I'm all ears.
>
The possible tests might be:
- creating a server/table with fetch_size set
- altering an existing server/table to have fetch_size set
- verifying that the value exists in pg_foreign_server.srvoptions and
pg_foreign_table.ftoptions.
- attempting to set fetch_size to a non-integer value
None of which are very interesting, and none of which actually test what
fetch_size was actually used.
But I'm happy to add any of those that seem worthwhile.
> > Still not sure what you mean by remote execution options. But it might be
> > simpler now that the patch is closer to your expectations.
>
> I'm talking about the documentation portion of the patch, which
> regrettably seems to have disappeared between v2 and v3.
>
Ah, didn't realize you were speaking about the documentation, and since
that section was new, I wasn't familiar with the name. Moved.
...and not sure why the doc change didn't make it into the last patch, but
it's in this one.
I also moved the paragraph starting "When using the extensions option, *it
is the user's responsibility* that..." such that it is in the same
varlistentry as "extensions", though maybe that change should be delegated
to the patch that created the extensions option.
Enjoy.
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 86a5789..f89de2f 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -131,6 +131,17 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
/* check list syntax, warn about uninstalled extensions */
(void) ExtractExtensionList(defGetString(def), true);
}
+ else if (strcmp(def->defname, "fetch_size") == 0)
+ {
+ int fetch_size;
+
+ fetch_size = strtol(defGetString(def), NULL,10);
+ if (fetch_size <= 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s requires a non-negative integer value",
+ def->defname)));
+ }
}
PG_RETURN_VOID();
@@ -162,6 +173,9 @@ InitPgFdwOptions(void)
/* updatable is available on both server and table */
{"updatable", ForeignServerRelationId, false},
{"updatable", ForeignTableRelationId, false},
+ /* fetch_size is available on both server and table */
+ {"fetch_size", ForeignServerRelationId, false},
+ {"fetch_size", ForeignTableRelationId, false},
{NULL, InvalidOid, false}
};
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 374faf5..f71bf61 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -68,7 +68,9 @@ enum FdwScanPrivateIndex
/* SQL statement to execute remotely (as a String node) */
FdwScanPrivateSelectSql,
/* Integer list of attribute numbers retrieved by the SELECT */
- FdwScanPrivateRetrievedAttrs
+ FdwScanPrivateRetrievedAttrs,
+ /* Integer representing the desired fetch_size */
+ FdwScanPrivateFetchSize
};
/*
@@ -126,6 +128,8 @@ typedef struct PgFdwScanState
/* working memory contexts */
MemoryContext batch_cxt; /* context holding current batch of tuples */
MemoryContext temp_cxt; /* context for per-tuple temporary data */
+
+ int fetch_size; /* number of tuples per fetch */
} PgFdwScanState;
/*
@@ -380,6 +384,7 @@ postgresGetForeignRelSize(PlannerInfo *root,
fpinfo->fdw_startup_cost = DEFAULT_FDW_STARTUP_COST;
fpinfo->fdw_tuple_cost = DEFAULT_FDW_TUPLE_COST;
fpinfo->shippable_extensions = NIL;
+ fpinfo->fetch_size = 100;
foreach(lc, fpinfo->server->options)
{
@@ -394,16 +399,17 @@ postgresGetForeignRelSize(PlannerInfo *root,
else if (strcmp(def->defname, "extensions") == 0)
fpinfo->shippable_extensions =
ExtractExtensionList(defGetString(def), false);
+ else if (strcmp(def->defname, "fetch_size") == 0)
+ fpinfo->fetch_size = strtol(defGetString(def), NULL,10);
}
foreach(lc, fpinfo->table->options)
{
DefElem *def = (DefElem *) lfirst(lc);
if (strcmp(def->defname, "use_remote_estimate") == 0)
- {
fpinfo->use_remote_estimate = defGetBoolean(def);
- break; /* only need the one value */
- }
+ else if (strcmp(def->defname, "fetch_size") == 0)
+ fpinfo->fetch_size = strtol(defGetString(def), NULL,10);
}
/*
@@ -1069,6 +1075,9 @@ postgresGetForeignPlan(PlannerInfo *root,
*/
fdw_private = list_make2(makeString(sql.data),
retrieved_attrs);
+ fdw_private = list_make3(makeString(sql.data),
+ retrieved_attrs,
+ makeInteger(fpinfo->fetch_size));
/*
* Create the ForeignScan node from target list, filtering expressions,
@@ -1147,6 +1156,8 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
FdwScanPrivateSelectSql));
fsstate->retrieved_attrs = (List *) list_nth(fsplan->fdw_private,
FdwScanPrivateRetrievedAttrs);
+ fsstate->fetch_size = intVal(list_nth(fsplan->fdw_private,
+ FdwScanPrivateFetchSize));
/* Create contexts for batches of tuples and per-tuple temp workspace. */
fsstate->batch_cxt = AllocSetContextCreate(estate->es_query_cxt,
@@ -2275,15 +2286,11 @@ fetch_more_data(ForeignScanState *node)
{
PGconn *conn = fsstate->conn;
char sql[64];
- int fetch_size;
int numrows;
int i;
- /* The fetch size is arbitrary, but shouldn't be enormous. */
- fetch_size = 100;
-
snprintf(sql, sizeof(sql), "FETCH %d FROM c%u",
- fetch_size, fsstate->cursor_number);
+ fsstate->fetch_size, fsstate->cursor_number);
res = PQexec(conn, sql);
/* On error, report the original query, not the FETCH. */
@@ -2311,7 +2318,7 @@ fetch_more_data(ForeignScanState *node)
fsstate->fetch_ct_2++;
/* Must be EOF if we didn't get as many tuples as we asked for. */
- fsstate->eof_reached = (numrows < fetch_size);
+ fsstate->eof_reached = (numrows < fsstate->fetch_size);
PQclear(res);
res = NULL;
@@ -2685,6 +2692,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
int fetch_size;
int numrows;
int i;
+ ListCell *lc;
/* Allow users to cancel long query */
CHECK_FOR_INTERRUPTS();
@@ -2695,8 +2703,27 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
* then just adjust rowstoskip and samplerows appropriately.
*/
- /* The fetch size is arbitrary, but shouldn't be enormous. */
fetch_size = 100;
+ foreach(lc, server->options)
+ {
+ DefElem *def = (DefElem *) lfirst(lc);
+
+ if (strcmp(def->defname, "fetch_size") == 0)
+ {
+ fetch_size = strtol(defGetString(def), NULL,10);
+ break;
+ }
+ }
+ foreach(lc, table->options)
+ {
+ DefElem *def = (DefElem *) lfirst(lc);
+
+ if (strcmp(def->defname, "fetch_size") == 0)
+ {
+ fetch_size = strtol(defGetString(def), NULL,10);
+ break;
+ }
+ }
/* Fetch some rows */
snprintf(fetch_sql, sizeof(fetch_sql), "FETCH %d FROM c%u",
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 8553536..4b67c37 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -53,6 +53,8 @@ typedef struct PgFdwRelationInfo
ForeignTable *table;
ForeignServer *server;
UserMapping *user; /* only set in use_remote_estimate mode */
+
+ int fetch_size; /* fetch size for this remote table */
} PgFdwRelationInfo;
/* in postgres_fdw.c */
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 5a829d5..a90983c 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -290,17 +290,30 @@
be considered shippable to the remote server.
This option can only be specified for foreign servers, not per-table.
</para>
+
+ <para>
+ When using the <literal>extensions</literal> option, <emphasis>it is the
+ user's responsibility</> that the listed extensions exist and behave
+ identically on both the local and remote servers. Otherwise, remote
+ queries may fail or behave unexpectedly.
+ </para>
</listitem>
</varlistentry>
- </variablelist>
+ <varlistentry>
+ <term><literal>fetch_size</literal></term>
+ <listitem>
+ <para>
+ This option specifies the number of rows <filename>postgres_fdw</>
+ should get in each fetch operation. It can be specified for a foreign
+ table or a foreign server. The option specified on a table overrides
+ an option specified for the server.
+ The default is <literal>100</>.
+ </para>
+ </listitem>
+ </varlistentry>
- <para>
- When using the <literal>extensions</literal> option, <emphasis>it is the
- user's responsibility</> that the listed extensions exist and behave
- identically on both the local and remote servers. Otherwise, remote
- queries may fail or behave unexpectedly.
- </para>
+ </variablelist>
</sect3>
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
