Re: [POC] FETCH limited by bytes. - Mailing list pgsql-hackers

Hello,

> > > @@ -915,6 +933,23 @@ postgresBeginForeignScan(ForeignScanState *node,
> > int eflags)
...
> > > +     def = get_option(table->options, "fetch_size");
> 
> > 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.

Sorry, it certainly shouldn't be a good place to do such thing. I
easily selected the place in order to avoid adding new similar
member in multiple existing structs (PgFdwRelationInfo and
PgFdwScanState).

Having a new member fetch_size is added in PgFdwRelationInfo and
PgFdwScanState, I think postgresGetForeignRelSize is the best
place to do that, from the point that it collects basic
information needed to calculate scan costs.

# Fetch sizes of foreign join would be the future issue..

> typedef struct PgFdwRelationInfo
> { ...
+ int    fetch_size;   /* fetch size for this remote table */

====================
> postgreGetForeignRelSize()
> { ...
>     fpinfo->table = GetForeignTable(foreigntableid);
>     fpinfo->server = GetForeignServer(fpinfo->table->serverid);
>  
+     def = get_option(table->options, "fetch_size");
+     ..
+     fpinfo->fetch_size = strtod(defGetString...

Also it is doable in postgresGetForeignPlan and doing there
removes redundant copy of fetch_size from PgFdwRelation to
PgFdwScanState but theoretical basis would be weak.

regards,

> > 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.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center