From: | Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> |
---|---|
To: | "Kohei KaiGai *EXTERN*" <kaigai(at)kaigai(dot)gr(dot)jp> |
Cc: | Atri Sharma <atri(dot)jiit(at)gmail(dot)com>, Alexander Korotkov *EXTERN* <aekorotkov(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>, "Shigeru Hanada" <shigeru(dot)hanada(at)gmail(dot)com> |
Subject: | Re: [v9.3] writable foreign tables |
Date: | 2012-12-07 11:24:56 |
Message-ID: | A737B7A37273E048B164557ADEF4A58B05785FFE@ntex2010i.host.magwien.gv.at |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2012-12-07 12:37:06 | Re: Support for REINDEX CONCURRENTLY |
Previous Message | Simon Riggs | 2012-12-07 09:16:56 | Re: pg_upgrade problem with invalid indexes |