Re: [v9.3] writable foreign tables

2012/12/11 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Kohei KaiGai wrote:
>>> Weird, that fails for me.
>
>> Both of the troubles you reported was fixed with attached patch.
>> I also added relevant test cases into regression test, please check it.
>
> It passes the regression tests, and solves the problems I found.
>
> I came up with one more query that causes a problem:
>
> CREATE TABLE test(
> id integer PRIMARY KEY,
> val text NOT NULL
> );
>
> INSERT INTO test(id, val) VALUES (1, 'one');
>
> CREATE FOREIGN TABLE rtest(
> id integer not null,
> val text not null
> ) SERVER loopback OPTIONS (relname 'test');
>
> /* loopback points to the same database */
>
> WITH ch AS (
> UPDATE test
> SET val='changed'
> RETURNING id
> ) UPDATE rtest
> SET val='new'
> FROM ch
> WHERE rtest.id = ch.id;
>
> This causes a deadlock, but one that is not detected;
> the query just keeps hanging.
>
> The UPDATE in the CTE has the rows locked, so the
> SELECT ... FOR UPDATE issued via the FDW connection will hang
> indefinitely.
>
> I wonder if that's just a pathological corner case that we shouldn't
> worry about. Loopback connections for FDWs themselves might not
> be so rare, for example as a substitute for autonomous subtransactions.
>
> I guess it is not easily possible to detect such a situation or
> to do something reasonable about it.
>
It is not avoidable problem due to the nature of distributed database system,
not only loopback scenario.

In my personal opinion, I'd like to wait for someone implements distributed
lock/transaction manager on top of the background worker framework being
recently committed, to intermediate lock request.
However, it will take massive amount of efforts to existing lock/transaction
management layer, not only enhancement of FDW APIs. It is obviously out
of scope in this patch.

So, I'd like to suggest authors of FDW that support writable features to put
mention about possible deadlock scenario in their documentation.
At least, above writable CTE example is a situation that two different sessions
concurrently update the "test" relation, thus, one shall be blocked.

>>> 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.
>
> I tried to overhaul the documentation, see the attached patch.
>
> There was one thing that I was not certain of:
> You say that for writable foreign tables, BeginForeignModify
> and EndForeignModify *must* be implemented.
> I thought that these were optional, and if you can do your work
> with just, say, ExecForeignDelete, you could do that.
>
Yes, that's right. What I wrote was incorrect.
If FDW driver does not require any state during modification of
foreign tables, indeed, these are not mandatory handler.

> I left that paragraph roughly as it is, please change it if
> appropriate.
>
> I also changed the misspelled "resultRelaion" and updated a
> few comments.
>
> May I suggest to split the patch in two parts, one for
> all the parts that affect postgres_fdw and one for the rest?
> That might make the committer's work easier, since
> postgres_fdw is not applied (yet).
>
OK. I split the patch into two portion, part-1 is the APIs relevant
patch, part-2 is relevant to postgres_fdw patch.

In addition, I adjusted the APIs a bit.
Even though the rowid pseudo-column is constructed as cstring
data type, the API delivered it as Datum type. We have no reason
why not to case prior to invoke Update or Delete handler.
At the beginning, I constructed the rowid pseudo-column using
INTERNALOID, but it made troubles when fetched tuples are
materialized prior to table modification.
So, the "rowid" argument of them are re-defined as "const char *".

Also, I found a possible bug when EXPLAIN command prints plan
tree with qualifiers that reference pseudo-columns. In this case,
it raise an error with "invalid attnum %d ...".
So, I adjusted the logic to reference eref alias when get_attname
returns NULL towards foreign-tables. It also required FDW driver
also need to set up rte->eref alias for each pseudo-column.

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