Re: [v9.3] writable foreign tables

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?

Yours,
Laurenz Albe