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:

Previous
From: Simon Riggs
Date:
Subject: Re: pg_upgrade problem with invalid indexes
Next
From: Michael Paquier
Date:
Subject: Re: Support for REINDEX CONCURRENTLY