On Wed, Sep 2, 2015 at 9:08 AM, Andres Freund <and...@anarazel.de> wrote:
> On 2015-02-27 13:50:22 -0500, Corey Huinker wrote:
> > +static DefElem*
> > +get_option(List *options, char *optname)
> > +{
> > + ListCell *lc;
> > +
> > + foreach(lc, options)
> > + {
> > + DefElem *def = (DefElem *) lfirst(lc);
> > +
> > + if (strcmp(def->defname, optname) == 0)
> > + return def;
> > + }
> > + return NULL;
> > +}
>
>
> > /*
> > * Do nothing in EXPLAIN (no ANALYZE) case. node->fdw_state stays
> NULL.
> > @@ -915,6 +933,23 @@ postgresBeginForeignScan(ForeignScanState *node,
> int eflags)
> > server = GetForeignServer(table->serverid);
> > user = GetUserMapping(userid, server->serverid);
> >
> > + /* Reading table options */
> > + fsstate->fetch_size = -1;
> > +
> > + def = get_option(table->options, "fetch_size");
> > + if (!def)
> > + def = get_option(server->options, "fetch_size");
> > +
> > + if (def)
> > + {
> > + fsstate->fetch_size = strtod(defGetString(def), NULL);
> > + if (fsstate->fetch_size < 0)
> > + elog(ERROR, "invalid fetch size for foreign table
> \"%s\"",
> > + get_rel_name(table->relid));
> > + }
> > + else
> > + fsstate->fetch_size = 100;
>
> I don't think it's a good idea to make such checks at runtime - and
> either way it's somethign that should be reported back using an
> ereport(), not an elog.
> Also, it seems somewhat wrong to determine this at execution
> time. Shouldn't this rather be done when creating the foreign scan node?
> And be a part of the scan state?
>
I agree, that was my original plan, but I wasn't familiar enough with the
FDW architecture to know where the table struct and the scan struct were
both exposed in the same function.
What I submitted incorporated some of Kyotaro's feedback (and working
patch) to my original incomplete patch.
>
> Have you thought about how this option should cooperate with join
> pushdown once implemented?
>
I hadn't until now, but I think the only sensible thing would be to
disregard table-specific settings once a second foreign table is detected,
and instead consider only the server-level setting.
I suppose one could argue that if ALL the tables in the join had the same
table-level setting, we should go with that, but I think that would be
complicated, expensive, and generally a good argument for changing the
server setting instead.
