Re: [v9.3] writable foreign tables

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

In response to

Responses

Browse pgsql-hackers by date

  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