Re: [v9.3] writable foreign tables - Mailing list pgsql-hackers
From | Albe Laurenz |
---|---|
Subject | Re: [v9.3] writable foreign tables |
Date | |
Msg-id | A737B7A37273E048B164557ADEF4A58B05785FFE@ntex2010i.host.magwien.gv.at Whole thread Raw |
In response to | Re: [v9.3] writable foreign tables (Kohei KaiGai <kaigai@kaigai.gr.jp>) |
Responses |
Re: [v9.3] writable foreign tables
|
List | pgsql-hackers |
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 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. > 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. > 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. > +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. I took a brief look at the documentation; that will need some more work. Yours, Laurenz Albe
pgsql-hackers by date: