Re: [v9.3] writable foreign tables

2012/12/7 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Kohei KaiGai wrote:
>> The attached patch is revised version.
>>
>> One most difference from the previous version is, it constructed
>> PoC features on Hanada-san's latest postgres-fdw.v5.patch.
>> Yesh, it looks to me working fine on RDBMS backend also.
>
> Cool.
>
>> Even though the filename of this patch contains "poc" phrase,
>> I think it may be time to consider adoption of the core regarding
>> to the interface portion.
>> (Of course, postgres_fdw is still works in progress.)
>
> Ok, I'll try to review it with regard to that.
>
>> Here is a few operation examples.
> [...]
>> postgres=# INSERT INTO tbl VALUES (7,'ggg'),(8,'hhh');
>> INSERT 0 2
>
> Weird, that fails for me.
>
> CREATE TABLE test(
> id integer PRIMARY KEY,
> val text NOT NULL
> );
>
> CREATE FOREIGN TABLE rtest(
> id integer not null,
> val text not null
> ) SERVER loopback OPTIONS (nspname 'laurenz', relname 'test');
>
> test=> INSERT INTO test(id, val) VALUES (1, 'one');
> INSERT 0 1
> test=> INSERT INTO rtest(id, val) VALUES (2, 'two');
> The connection to the server was lost. Attempting reset: Failed.
> !>
>
> Here is the stack trace:
> 317 RangeTblEntry *rte = root->simple_rte_array[rtindex];
> #0 0x00231cb9 in deparseInsertSql (buf=0xbfffafb0, root=0x9be3614, rtindex=1) at deparse.c:317
> #1 0x0023018c in postgresPlanForeignModify (root=0x9be3614, plan=0x9be3278, resultRelaion=1, subplan=0x9be3bec)
> at postgres_fdw.c:1538
> #2 0x082a3ac2 in make_modifytable (root=0x9be3614, operation=CMD_INSERT, canSetTag=1 '\001', resultRelations=0x9be3c7c,
> subplans=0x9be3c4c, returningLists=0x0, rowMarks=0x0, epqParam=0) at createplan.c:4787
> #3 0x082a7ada in subquery_planner (glob=0x9be357c, parse=0x9be3304, parent_root=0x0, hasRecursion=0 '\0', tuple_fraction=0,
> subroot=0xbfffb0ec) at planner.c:574
> #4 0x082a71dc in standard_planner (parse=0x9be3304, cursorOptions=0, boundParams=0x0) at planner.c:200
> #5 0x082a707b in planner (parse=0x9be3304, cursorOptions=0, boundParams=0x0) at planner.c:129
> #6 0x0832c14c in pg_plan_query (querytree=0x9be3304, cursorOptions=0, boundParams=0x0) at postgres.c:753
> #7 0x0832c1ec in pg_plan_queries (querytrees=0x9be3a98, cursorOptions=0, boundParams=0x0) at postgres.c:812
> #8 0x0832c46e in exec_simple_query (query_string=0x9be267c "INSERT INTO rtest(id, val) VALUES (2, 'two');") at postgres.c:977
>
> (gdb) print root->simple_rte_array
> $1 = (RangeTblEntry **) 0x0
>
The reason why simple_rte_array was not initialized is, invocation of
setup_simple_rel_arrays() was skipped in case of simple INSERT INTO
... VALUES(). So, I put setup_simple_rel_arrays() just after short-cut
code path at query_planner().

> The bug I reported in my last review does not seem to be fixed, either.
> The symptoms are different now (with the definitions from above):
>
> test=> UPDATE rtest SET val='new' FROM rtest t2 WHERE rtest.id=t2.id AND t2.val LIKE '%e';
> TRAP: FailedAssertion("!(baserel->relid == var->varno)", File: "foreign.c", Line: 601)
> The connection to the server was lost. Attempting reset: Failed.
> !>
>
> The same happens for DELETE ... USING.
>
> A different failure happens if I join with a local table:
>
> test=> UPDATE rtest SET val='new' FROM test t2 WHERE rtest.id=t2.id AND t2.val = 'nonexist';
> TRAP: FailedAssertion("!(((((const Node*)(fscan))->type) == T_ForeignScan))", File: "postgres_fdw.c", Line: 1526)
> The connection to the server was lost. Attempting reset: Failed.
> !>
>
> I gave up testing the functionality after that.
>
The reason of this assertion trap is, I incorrectly assumed subplan of
ModifyTable is always ForeignScan. However, it should be picked up
it from the subplan tree using recursive node walking.
I newly added lookup_foreign_scan_plan() not to reinvent same logic
for each extension. It returns ForeignScan node that has supplied
rtindex, even if the top of subplan tree is not ForeignScan.

Both of the troubles you reported was fixed with attached patch.
I also added relevant test cases into regression test, please check it.

>> So, let's back to the main topic of this patch.
>> According to the upthread discussion, I renamed the interface to inform
>> expected width of result set as follows:
>>
>> +typedef AttrNumber (*GetForeignRelWidth_function) (PlannerInfo *root,
>> + RelOptInfo *baserel,
>> + Relation foreignrel,
>> + bool inhparent,
>> + List *targetList);
>>
>> It informs the core how many slots for regular and pseudo columns shall
>> be acquired. If it is identical with number of attributed in foreign table
>> definition, it also means this scan does not use any pseudo columns.
>> A typical use case of pseudo column is "rowid" to move an identifier of
>> remote row from scan stage to modify stage. It is responsibility of FDW
>> driver to ensure "rowid" has uniqueness on the remote side; my
>> enhancement on postgres_fdw uses ctid.
>>
>> get_pseudo_rowid_column() is a utility function that picks up an attribute
>> number of pseudo "rowid" column if query rewriter injected on previous
>> stage. If FDW does not support any other pseudo column features, the
>> value to be returned is just return-value of this function.
>
> Thanks, I think this will make the FDW writer's work easier.
>
I adjusted the code a bit to work this routine correctly, even if a query
contains two or more foreign table references.

>> Other relevant APIs are as follows:
>>
>> +typedef List *(*PlanForeignModify_function) (PlannerInfo *root,
>> + ModifyTable *plan,
>> + Index resultRelation,
>> + Plan *subplan);
>> +
>> I newly added this handler on construction of ModifyTable structure.
>> Because INSERT command does not have scan stage directly connected
>> with table modification, FDW driver has no chance to construct its private
>> stuff relevant to table modification. (In case postgres_fdw, it constructs
>> the second query to modify remote table with/without given ctid.)
>> Its returned List * value is moved to BeginForeignModify handler as
>> third argument.
>
> So, in the case of an INSERT, GetForeignPlan and friends are not
> called? Then such a functionality is indeed desirable.
>
Yes. In case of INSERT, its resultRelation is not appeared in the fromList,
thus, no chance to create relevant scan plan towards the table to be inserted.

>> +typedef void (*BeginForeignModify_function) (ModifyTableState *mtstate,
>> + ResultRelInfo *resultRelInfo,
>> + List *fdw_private,
>> + Plan *subplan,
>> + int eflags);
>> I adjusted some arguments to reference fdw_private being constructed
>> on query plan stage. The role of this handler is not changed. FDW driver
>> should have all the initialization stuff on this handler, like we are doing at
>> BeginForeignScan.
>
> I wonder about the PlanForeignModify/BeginForeignModify pair.
> It is quite different from the "Scan" functions.
>
> Would it make sense to invent a ForeignModifyTableState that has
> the fdw_private information included, similar to ForeignScanState?
> It might make the API more homogenous.
> But maybe that's overkill.
>
The origin of this asymmetry is ModifyTableState may affect multiple
tables if result relation has inheritance, even though foreign table does
not support inheritance feature right now.
Please see the entrypoint to invoke BeginForeignModify handler.
It is in a loop block that initializes executor state being associated with
thie ModifyTableState.

Even if we have symmetric definition, the variable "i" need to be delivered
to identify which executor state is under initialization at least.
The relevant resultRelInfo, fdwprivate, and subplan can be picked up
from the ModifyTableState using this index. But it enforces extensions
to implement same logic with individual code. My preference is to pick
up necessary information at the code side.

> I took a brief look at the documentation; that will need some more
> work.
>
Yep, I believe it should be revised prior to this patch being committed.

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