Re: [v9.3] writable foreign tables

2012/11/20 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Kohei KaiGai wrote:
>>>> This design tries to kill two-birds with one-stone.
>>>> It enables to add multiple number of pseudo-columns,
>>>> not only "rowid", and makes possible to push-down
>>>> complex calculation of target list into external computing
>>>> resource.
>>>>
>>>> For example, when user gives the following query:
>>>>
>>>> SELECT ((c1 - c2) * (c2 - c3))^2 FROM ftable
>>>>
>>>> it contains a complex calculation in the target-list,
>>>> thus, it also takes CPU cycles of local process.
>>>>
>>>> If we can replace the "((c1 - c2) * (c2 - c3))^2" by
>>>> a reference to a pseudo-column that also references
>>>> the calculation result on external node, it effectively
>>>> off-load CPU cycles.
>>>>
>>>> In this case, all we need to do is (1) acquire a slot
>>>> for pseudo-column at GetForeignRelInfo (2) replace
>>>> TargetEntry::expr by Var node that reference this
>>>> pseudo-column.
>>>>
>>>> It makes sense for performance optimization, so I don't
>>>> want to restrict this handler for "rowid" only.
>
>>> I understand.
>>>
>>> But I think that you still can do that with the change that
>>> I suggest. I suggest that GetForeignRelInfo (or whatever the
>>> name ends up being) gets the AttrNumber of the proposed "rowid"
>>> column in addition to the parameters you need for what
>>> you want to do.
>>>
>>> Then nothing would keep you from defining those
>>> pseudo-columns. But all the setup necessary for the "rowid"
>>> column could be moved out of the FDW. So for the 99% of all
>>> FDW which are only interested in the rowid, things would
>>> get much simpler and they don't all have to implement the
>>> same code.
>
>> All we have to do at get_relation_info() to deal with pseudo-
>> columns (including alternatives of complex calculation, not
>> only "rowid") is just expansion of rel->max_attr.
>> So, if FDW is not interested in something except for "rowid",
>> it can just inform the caller "Yeah, we need just one slot for
>> a pseudo-column of rowid". Otherwise, it can return another
>> value to acquire the slot for arbitrary pseudo-column.
>> I don't think it is a problematic design.
>>
>> However, I'm skeptical 99% of FDWs don't support target-list
>> push-down. At least, it was very desired feature when I had
>> a talk at PGconf.EU last month. :-)
>
> I agree that PostgreSQL should make this technique possible.
>
> My idea should not make this any more difficult.
>
>> So, if we rename it to GetForeignRelWidth, is it defined as
>> follows?
>>
>> extern AttrNumber
>> GetForeignRelWidth(PlannerInfo *root,
>> RelOptInfo *baserel,
>> Oid foreigntableid,
>> bool inhparent,
>> List *targetList);
>>
>> Right now, inhparent makes no sense because foreign table
>> does not support table inheritance, but it seems to me we
>> shall have this functionality near future.
>
> I am thinking of this declaration:
>
> extern bool
> GetForeignRelWidth(PlannerInfo *root,
> RelOptInfo *baserel,
> Oid foreigntableid,
> bool inhparent,
> List *targetList,
> AttrNumber rowidAttr);
>
> Let me illustrate my idea with some code.
>
> Here's your fileGetForeignRelInfo:
>
>
> static void
> fileGetForeignRelInfo(PlannerInfo *root,
> RelOptInfo *baserel,
> Oid foreigntableid,
> bool inhparent, List *targetList)
> {
> FileFdwPlanState *fdw_private;
> ListCell *cell;
>
> fdw_private = (FileFdwPlanState *)
> palloc0(sizeof(FileFdwPlanState));
>
> foreach(cell, targetList)
> {
> TargetEntry *tle = lfirst(cell);
>
> if (tle->resjunk &&
> tle->resname && strcmp(tle->resname, "rowid")==0)
> {
> Bitmapset *temp = NULL;
> AttrNumber anum_rowid;
> DefElem *defel;
>
> pull_varattnos((Node *)tle, baserel->relid, &temp);
> anum_rowid = bms_singleton_member(temp)
> + FirstLowInvalidHeapAttributeNumber;
> /* adjust attr_needed of baserel */
> if (anum_rowid > baserel->max_attr)
> baserel->max_attr = anum_rowid;
> defel = makeDefElem("anum_rowid",
> (Node *)makeInteger(anum_rowid));
> fdw_private->anum_rowid = defel;
> }
> }
> baserel->fdw_private = fdw_private;
> }
>
>
> I hope that this can be reduced to:
>
>
> static bool
> fileGetForeignRelRowid(PlannerInfo *root,
> RelOptInfo *baserel,
> Oid foreigntableid,
> bool inhparent,
> List *targetList,
> AttrNumber rowidAttr)
> {
> FileFdwPlanState *fdw_private;
> fdw_private = (FileFdwPlanState *)
> palloc0(sizeof(FileFdwPlanState));
>
> defel = makeDefElem("anum_rowid",
> (Node *)makeInteger(rowidAttr));
> fdw_private->anum_rowid = defel;
>
> baserel->fdw_private = fdw_private;
>
> return true; /* we'll use rowid, so please extend baserel->max_attr
> */
> }
>
>
> That wouldn't mean that the FDW cannot define any other
> pseudo-columns in this function, just the case of rowid
> would be simplified.
>
> Does that make any sense?
>
I think "bool" is not suitable data type to inform how many pseudo-columns
are needed for this scan on foreign-table.

Probably, it is helpful to provide a helper function that fetches an attribute-
number of pseudo "rowid" column from the supplied targetlist.
If we have GetPseudoRowidColumn() at foreign/foreign.c, the avove
routine can be rewritten as:

static AttrNumber
fileGetForeignRelWidth(PlannerInfo *root,
RelOptInfo *baserel,
Relation foreignrel,
bool inhparent, List *targetList)
{
FileFdwPlanState *fdw_private;
AttrNumber nattrs = RelationGetNumberOfAttributes(foreignrel);
AttrNumber anum_rowid;

fdw_private = palloc0(sizeof(FileFdwPlanState));
anum_rowid = GetPseudoRowidColumn(..., targetList);
if (anum_rowid > 0)
{
Assert(anum_rowid > nattrs);
fdw_private->anum_rowid
= makeDefElem("anum_rowid", (Node *)makeInteger(anum_rowid));
nattrs = anum_rowid;
}
baserel->fdw_private = fdw_private;

return nattrs;
}

In case when FDW drive wants to push-down other target entry into foreign-
side, thus, it needs multiple pseudo-columns, it is decision of the extension.
In addition, it does not take API change in the future, if some more additional
pseudo-column is required by some other new features.

How about your opinion?

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>