Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Column defaults for foreign tables (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

Re: Column defaults for foreign tables (was Re: [v9.3] writable foreign tables)

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: Column defaults for foreign tables (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

Kaigai-san,

On Thu, Aug 23, 2012 at 2:10 PM, Kohei KaiGai  wrote:
> The patched portion at contrib/file_fdw.c does not make sense
> actually. It just prints messages for each invocation.
> It is just a proof-of-concept to show possibility of implementation
> based on real RDBMS.

Attached is a tar ball of pgsql_fdw.  It's WIP and contains no
document, but it would be enough for your PoC purpose.  Usage and
features are same as the last version posted for 9.2 cycle.
# I'll post finished patch in the CF-Sep.

Here are random comments for your PoC patch:

+ As Robert says, using CTID as virtual tuple identifier doesn't seem
nice when considering various FDWs for NoSQL or RDBMS.  Having abstract
layer between FDWs and tuple sounds better, but implementing it by each
FDW seems useless effort.  Do yo have any idea of generic mechanism for
tuple mapping?

+ Do you have any plan about deparsing local qualifiers into remote
query to avoid repeated query submission?  This would improve
performance of big UPDATE, but its use case might be limited to
statements which consist of one foreign table.  For this case, we can
consider pass-through mode as second way.

+ I have not read your patch closely yet, but I wonder how we can know
which column is actually updated.  If we have only updated image of
tuple, we have to update all remote columns by "new" values?

-- 
Shigeru Hanada

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: Column defaults for foreign tables (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

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.

>> 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.

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).

Yours,
Laurenz Albe

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Kohei KaiGai wrote:
>> I came up with one more query that causes a problem:
[...]
>> 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.

Fair enough.

>> 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 have updated the documentation, that was all I changed in the
attached patches.

> OK. I split the patch into two portion, part-1 is the APIs relevant
> patch, part-2 is relevant to postgres_fdw patch.

Great.

I'll mark the patch as "ready for committer".

Yours,
Laurenz Albe

Re: Column defaults for foreign tables (was Re: [v9.3] writable foreign tables)

Re: Column defaults for foreign tables (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: Column defaults for foreign tables (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

On Thu, Mar 14, 2013 at 8:07 PM, Tom Lane  wrote:
> Daniel Farina  writes:
>> On Tue, Mar 12, 2013 at 11:51 AM, Tom Lane  wrote:
>>> Yeah, watching the remote side's datestyle and intervalstyle and
>>> matching them (for both input and output) would probably work.
>
>> Alright, so I've been whacking at this and there's one interesting
>> thing to ask about: saving and restoring the local GUCs.  There are a
>> bunch of things about GUCs besides their value that are maintained,
>> such as their 'source', so writing a little ad-hoc save/restore is not
>> going to do the right thing.
>
> Right, you should use NewGUCNestLevel/AtEOXact_GUC.  Look at the fixes
> I committed in postgres_fdw a day or two ago for an example.

Thanks.  Here's a patch.  Here is the comments on top of the patch file inlined:

Similar in purpose to cc3f281ffb0a5d9b187e7a7b7de4a045809ff683, but
taking into account that a dblink caller may choose to cause arbitrary
changes to DateStyle and IntervalStyle.  To handle this, it is
necessary to use PQparameterStatus before parsing any input, every
time.  This is avoided in cases that do not include the two
problematic types treated -- interval and timestamptz -- by scanning
the TupleDesc's types first.

Although it has been suggested that extra_float_digits would need
similar treatment as IntervalStyle and DateStyle (as it's seen in
pg_dump), extra_float_digits is not an GUC_REPORT-ed GUC and is not
able to be checked in PQparameterStatus, and furthermore, the float4
and float8 parsers are not sensitive to the GUC anyway: both accept as
much precision as is provided in an unambiguous way.

>> So, I can add one more such use of AtEOXact_GUC for the dblink fix,
>> but would it also be appropriate to find some new names for the
>> concepts (instead of AtEOXact_GUC and isCommit) here to more
>> accurately express what's going on?
>
> Meh.  I guess we could invent an "EndGUCNestLevel" that's a wrapper
> around AtEOXact_GUC, but I'm not that excited about it ...

Per your sentiment, I won't pursue that then.

Overall, the patch I think has reasonably thorough regression testing
(I tried to hit the several code paths whereby one would have to scan
TupleDescs, as well as a few other edge cases) and has some of the
obvious optimizations in place: it doesn't call PQparameterStatus more
than once per vulnerable type per TupleDesc scan, and avoids using the
 parameter status procedure at all if there are no affected types.

The mechanisms may be overwrought though, since it was originally
intended to generalize to three types before I realized that
extra_float_digits is another kind of problem entirely, leaving just
IntervalStyle and DateStyle remaining, which perhaps could have been
treated even more specially to make the patch more terse.

I'll add it to the commitfest.

--
fdr

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

On Tue, Mar 19, 2013 at 3:06 PM, Daniel Farina  wrote:
> On Tue, Mar 19, 2013 at 2:41 PM, Tom Lane  wrote:
>> Daniel Farina  writes:
>>> On Tue, Mar 19, 2013 at 1:16 PM, Tom Lane  wrote:
>>>> I'd be inclined to eat the cost of calling PQparameterStatus every time
>>>> (which won't be that much) and instead try to optimize by avoiding the
>>>> GUC-setting overhead if the current value matches the local setting.
>>>> But even that might be premature optimization.  Did you do any
>>>> performance testing to see if there was a problem worth avoiding?
>>
>>> Nope; should I invent a new way to do that, or would it be up to
>>> commit standard based on inspection alone?  I'm okay either way, let
>>> me know.
>>
>> Doesn't seem that hard to test: run a dblink query that pulls back a
>> bunch of data under best-case conditions (ie, local not remote server),
>> and time it with and without the proposed fix.  If the difference
>> is marginal then it's not worth working hard to optimize.
>
> Okay, will do, and here's the shorter and less mechanically intensive
> naive version that I think is the baseline: it doesn't try to optimize
> out any GUC settings and sets up the GUCs before the two
> materialization paths in dblink.

The results.  Summary: seems like grabbing the GUC and strcmp-ing is
worthwhile, but the amount of ping-ponging between processes adds some
noise to the timing results: utilization is far short of 100% on
either processor involved.  Attached is a cumulative diff of the new
version, and also reproduced below are the changes to v2 that make up
v3.

## Benchmark

SELECT dblink_connect('benchconn','dbname=contrib_regression');

CREATE FUNCTION bench() RETURNS integer AS $$
    DECLARE
    iterations integer;
    BEGIN
iterations := 0;

        WHILE iterations < 300000 LOOP
          PERFORM * FROM dblink('benchconn', 'SELECT 1') AS t(a int);
          iterations := iterations + 1;
        END LOOP;

        RETURN iterations;
    END;
$$ LANGUAGE plpgsql;

SELECT clock_timestamp() INTO TEMP beginning;
SELECT bench();
SELECT clock_timestamp() INTO TEMP ending;

SELECT 'dblink-benchmark-lines';
SELECT ending.clock_timestamp - beginning.clock_timestamp
FROM beginning, ending;

## Data Setup

CREATE TEMP TABLE bench_results (version text, span interval);

COPY bench_results FROM STDIN WITH CSV;
no-patch,@ 41.308122 secs
no-patch,@ 36.63597 secs
no-patch,@ 34.264119 secs
no-patch,@ 34.760179 secs
no-patch,@ 32.991257 secs
no-patch,@ 34.538258 secs
no-patch,@ 42.576354 secs
no-patch,@ 39.335557 secs
no-patch,@ 37.493206 secs
no-patch,@ 37.812205 secs
v2-applied,@ 36.550553 secs
v2-applied,@ 38.608723 secs
v2-applied,@ 39.415744 secs
v2-applied,@ 46.091052 secs
v2-applied,@ 45.425438 secs
v2-applied,@ 48.219506 secs
v2-applied,@ 43.514878 secs
v2-applied,@ 45.892302 secs
v2-applied,@ 48.479335 secs
v2-applied,@ 47.632041 secs
v3-strcmp,@ 32.524385 secs
v3-strcmp,@ 34.982098 secs
v3-strcmp,@ 34.487222 secs
v3-strcmp,@ 44.394681 secs
v3-strcmp,@ 44.638309 secs
v3-strcmp,@ 44.113088 secs
v3-strcmp,@ 45.497769 secs
v3-strcmp,@ 33.530176 secs
v3-strcmp,@ 32.9704 secs
v3-strcmp,@ 40.84764 secs
\.

=> SELECT version, avg(extract(epoch from span)), stddev(extract(epoch
from span))
   FROM bench_results
   GROUP BY version;
  version   |    avg     |      stddev
------------+------------+------------------
 no-patch   | 37.1715227 | 3.17076487912923
 v2-applied | 43.9829572 | 4.30572672565711
 v3-strcmp  | 38.7985768 | 5.54760393720725

## Changes to v2:

--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2981,9 +2981,11 @@ initRemoteGucs(remoteGucs *rgs, PGconn *conn)
 static void
 applyRemoteGucs(remoteGucs *rgs)
 {
- int i;
  const int numGucs = sizeof parseAffectingGucs / sizeof *parseAffectingGucs;

+ int i;
+ int addedGucNesting = false;
+
  /*
  * Affected types require local GUC manipulations.  Create a new
  * GUC NestLevel to overlay the remote settings.
@@ -2992,14 +2994,27 @@ applyRemoteGucs(remoteGucs *rgs)
  * structure, so expect it to come with an invalid NestLevel.
  */
  Assert(rgs->localGUCNestLevel == -1);
- rgs->localGUCNestLevel = NewGUCNestLevel();

  for (i = 0; i < numGucs; i += 1)
  {
+ int gucApplyStatus;
+
  const char *gucName   = parseAffectingGucs[i];
  const char *remoteVal = PQparameterStatus(rgs->conn, gucName);
+ const char *localVal  = GetConfigOption(gucName, true, true);

- int gucApplyStatus;
+ /*
+ * Attempt to avoid GUC setting if the remote and local GUCs
+ * already have the same value.
+ */
+ if (strcmp(remoteVal, localVal) == 0)
+ continue;
+
+ if (!addedGucNesting)
+ {
+ rgs->localGUCNestLevel = NewGUCNestLevel();
+ addedGucNesting = true;
+ }

  gucApplyStatus = set_config_option(gucName, remoteVal,
    PGC_USERSET, PGC_S_SESSION,

--
fdr

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

On Tue, Mar 19, 2013 at 6:10 PM, Daniel Farina  wrote:
> On Tue, Mar 19, 2013 at 3:06 PM, Daniel Farina  wrote:
>> On Tue, Mar 19, 2013 at 2:41 PM, Tom Lane  wrote:
>>> Daniel Farina  writes:
>>>> On Tue, Mar 19, 2013 at 1:16 PM, Tom Lane  wrote:
>>>>> I'd be inclined to eat the cost of calling PQparameterStatus every time
>>>>> (which won't be that much) and instead try to optimize by avoiding the
>>>>> GUC-setting overhead if the current value matches the local setting.
>>>>> But even that might be premature optimization.  Did you do any
>>>>> performance testing to see if there was a problem worth avoiding?
>>>
>>>> Nope; should I invent a new way to do that, or would it be up to
>>>> commit standard based on inspection alone?  I'm okay either way, let
>>>> me know.
>>>
>>> Doesn't seem that hard to test: run a dblink query that pulls back a
>>> bunch of data under best-case conditions (ie, local not remote server),
>>> and time it with and without the proposed fix.  If the difference
>>> is marginal then it's not worth working hard to optimize.
>>
>> Okay, will do, and here's the shorter and less mechanically intensive
>> naive version that I think is the baseline: it doesn't try to optimize
>> out any GUC settings and sets up the GUCs before the two
>> materialization paths in dblink.
>
> The results.  Summary: seems like grabbing the GUC and strcmp-ing is
> worthwhile, but the amount of ping-ponging between processes adds some
> noise to the timing results: utilization is far short of 100% on
> either processor involved.  Attached is a cumulative diff of the new
> version, and also reproduced below are the changes to v2 that make up
> v3.

I added programming around various NULL returns reading GUCs in this
revision, v4.

The non-cumulative changes:

--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -3005,8 +3005,22 @@ applyRemoteGucs(remoteGucs *rgs)
  /*
  * Attempt to avoid GUC setting if the remote and local GUCs
  * already have the same value.
+ *
+ * NB: Must error if the GUC is not found.
  */
- localVal = GetConfigOption(gucName, true, true);
+ localVal = GetConfigOption(gucName, false, true);
+
+ if (remoteVal == NULL)
+ ereport(ERROR,
+ (errmsg("could not load parameter status of %s",
+ gucName)));
+
+ /*
+ * An error must have been raised by now if GUC values could
+ * not be loaded for any reason.
+ */
+ Assert(localVal != NULL);
+ Assert(remoteVal != NULL);

  if (strcmp(remoteVal, localVal) == 0)
  continue;

--
fdr

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

On Wed, Mar 20, 2013 at 7:43 AM, Tom Lane  wrote:
> Daniel Farina  writes:
>> Okay, one more of those fridge-logic bugs.  Sorry for the noise. v5.
>
>> A missing PG_RETHROW to get the properly finally-esque semantics:
>
>> --- a/contrib/dblink/dblink.c
>> +++ b/contrib/dblink/dblink.c
>> @@ -642,7 +642,10 @@ dblink_fetch(PG_FUNCTION_ARGS)
>>   }
>>   PG_CATCH();
>>   {
>> + /* Pop any set GUCs, if necessary */
>>   restoreLocalGucs(&rgs);
>> +
>> + PG_RE_THROW();
>>   }
>>   PG_END_TRY();
>
> Um ... you shouldn't need a PG_TRY for that at all.  guc.c will take
> care of popping the values on transaction abort --- that's really rather
> the whole point of having that mechanism.

Hmm, well, merely raising the error doesn't reset the GUCs, so I was
rather thinking that this was a good idea to compose more neatly in
the case of nested exception processing, e.g.:

    PG_TRY();
    {
        PG_TRY();
        {
            elog(NOTICE, "pre: %s",
                 GetConfigOption("DateStyle", false, true));
            materializeResult(fcinfo, res);
        }
        PG_CATCH();
        {
            elog(NOTICE, "inner catch: %s",
                 GetConfigOption("DateStyle", false, true));
            PG_RE_THROW();
        }
        PG_END_TRY();
    }
    PG_CATCH();
    {
        elog(NOTICE, "outer catch: %s",
             GetConfigOption("DateStyle", false, true));
        restoreLocalGucs(&rgs);
        elog(NOTICE, "restored: %s",
             GetConfigOption("DateStyle", false, true));
        PG_RE_THROW();
    }
    PG_END_TRY();

I don't think this paranoia is made in other call sites for AtEOXact,
so included is a version that takes the same stance. This one shows up
 best with the whitespace-insensitive option for git:

--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -634,21 +634,8 @@ dblink_fetch(PG_FUNCTION_ARGS)
         * affect parsing and then un-set them afterwards.
         */
        initRemoteGucs(&rgs, conn);
-
-       PG_TRY();
-       {
        applyRemoteGucs(&rgs);
        materializeResult(fcinfo, res);
-       }
-       PG_CATCH();
-       {
-               /* Pop any set GUCs, if necessary */
-               restoreLocalGucs(&rgs);
-
-               PG_RE_THROW();
-       }
-       PG_END_TRY();
-
        restoreLocalGucs(&rgs);

        return (Datum) 0;
@@ -823,9 +810,6 @@ dblink_record_internal(FunctionCallInfo fcinfo,
bool is_async)
                if (freeconn)
                        PQfinish(conn);

-               /* Pop any set GUCs, if necessary */
-               restoreLocalGucs(&rgs);
-
                PG_RE_THROW();
        }
        PG_END_TRY();

--
fdr

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

On Tue, Mar 19, 2013 at 2:41 PM, Tom Lane  wrote:
> Daniel Farina  writes:
>> On Tue, Mar 19, 2013 at 1:16 PM, Tom Lane  wrote:
>>> I'd be inclined to eat the cost of calling PQparameterStatus every time
>>> (which won't be that much) and instead try to optimize by avoiding the
>>> GUC-setting overhead if the current value matches the local setting.
>>> But even that might be premature optimization.  Did you do any
>>> performance testing to see if there was a problem worth avoiding?
>
>> Nope; should I invent a new way to do that, or would it be up to
>> commit standard based on inspection alone?  I'm okay either way, let
>> me know.
>
> Doesn't seem that hard to test: run a dblink query that pulls back a
> bunch of data under best-case conditions (ie, local not remote server),
> and time it with and without the proposed fix.  If the difference
> is marginal then it's not worth working hard to optimize.

Okay, will do, and here's the shorter and less mechanically intensive
naive version that I think is the baseline: it doesn't try to optimize
out any GUC settings and sets up the GUCs before the two
materialization paths in dblink.

Something I forgot to ask about is about if an strangely-minded type
input function could whack around the GUC as records are being
remitted, which would necessitate per-tuple polling of
pqParameterStatus (e.g. in the inner loop of a materialization) .
That seemed pretty "out there," but I'm broaching it for completeness.

I'll see how much of a penalty it is vs. not applying any patch at all next.

--
fdr

Re: [v9.3] writable foreign tables

On Fri, Feb 1, 2013 at 2:22 AM, Daniel Farina  wrote:
> On Tue, Jan 29, 2013 at 1:19 AM, Kohei KaiGai  wrote:
>> I noticed the v10 patch cannot be applied on the latest master branch
>> cleanly. The attached ones were rebased.
>
> Anyway, I'm looking at the first patch, which applies neatly. Thanks.

Hello,

My review is incomplete, but to the benefit of pipelining I wanted to
ask a couple of things and submit some changes for your consideration
while continuing to look at this.

So far, I've been trying to understand in very close detail how your
changes affect non-FDW related paths first, before delving into the
actual writable FDW functionality.  There's not that much in this
category, but there's one thing that gave me pause: the fact that the
target list (by convention, tlist) has to be pushed down from
planmain.c/query_planner all the way to a
fdwroutine->GetForeignRelWidth callsite in plancat.c/get_relation_info
(so even in the end, it is just pushed down -- never inspected or
modified).  In relative terms, it's a significant widening of
currently fairly short parameter lists in these procedures:

add_base_rels_to_query(PlannerInfo *root, List *tlist, Node *jtnode)
build_simple_rel(PlannerInfo *root, int relid, List *tlist, RelOptKind
reloptkind)
get_relation_info(PlannerInfo *root, Oid relationObjectId, bool
inhparent, List *tlist, RelOptInfo *rel)

In the case of all the other parameters except tlist, each is more
intimately related with the inner workings and mechanisms of the
procedure.  I wonder if there's a nice way that can train the reader's
eye that the tlist is simply pushed down rather than actively managed
at each level.  However, I can't immediately produce a way to both
achieve that that doesn't seem overwrought.  Perhaps a comment will
suffice.

Related to this, I found that make_modifytable grew a dependency on
PlannerInfo *root, and it seemed like a bunch of the arguments are
functionally related to that, so the attached patch that should be
able to be applied to yours tries to utilize that as often as
possible.  The weirdest thing in there is how make_modifytable has
been taught to call SS_assign_special_param, which has a side effect
on the PlannerInfo, but there exist exactly zero cases where one
*doesn't* want to do that prior to the (exactly two) call sites to
make_modifytable, so I pushed it in.  The second weirdest thing is
pushing in the handling of rowMarks (e.g. SELECT FOR UPDATE et al)

Let me know as to your thoughts, otherwise I'll keep moving on...

--
fdr

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

This contains some edits to comments that referred to the obsolete and
bogus TupleDesc scanning.  No mechanical alterations.

--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2961,9 +2961,8 @@ initRemoteGucs(remoteGucs *rgs, PGconn *conn)
 }

 /*
- * Scan a TupleDesc and, should it contain types that are sensitive to
- * GUCs, acquire remote GUCs and set them in a new GUC nesting level.
- * This is undone with restoreLocalGucs.
+ * Acquire remote GUCs that may affect type parsing and set them in a
+ * new GUC nesting level.
  */
 static void
 applyRemoteGucs(remoteGucs *rgs)
@@ -2974,11 +2973,8 @@ applyRemoteGucs(remoteGucs *rgs)
        int addedGucNesting = false;

        /*
-        * Affected types require local GUC manipulations.  Create a new
-        * GUC NestLevel to overlay the remote settings.
-        *
-        * Also, this nesting is done exactly once per remoteGucInfo
-        * structure, so expect it to come with an invalid NestLevel.
+        * This nesting is done exactly once per remoteGucInfo structure,
+        * so expect it to come with an invalid NestLevel.
         */
        Assert(rgs->localGUCNestLevel == -1);

diff --git a/contrib/dblink/expected/dblink.out
b/contrib/dblink/expected/dblink.out
index 3946485..579664e 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -930,9 +930,8 @@ SELECT dblink_exec('myconn', 'SET datestyle =
GERMAN, DMY;');
  SET
 (1 row)

--- The following attempt test various paths at which TupleDescs are
--- formed and inspected for containment of types requiring local GUC
--- setting.
+-- The following attempt test various paths at which tuples are formed
+-- and inspected for containment of types requiring local GUC setting.
 -- single row synchronous case
 SELECT *
 FROM dblink('myconn',
diff --git a/contrib/dblink/sql/dblink.sql b/contrib/dblink/sql/dblink.sql
index de925eb..7ff43fd 100644
--- a/contrib/dblink/sql/dblink.sql
+++ b/contrib/dblink/sql/dblink.sql
@@ -435,9 +435,8 @@ SET timezone = UTC;
 SELECT dblink_connect('myconn','dbname=contrib_regression');
 SELECT dblink_exec('myconn', 'SET datestyle = GERMAN, DMY;');

--- The following attempt test various paths at which TupleDescs are
--- formed and inspected for containment of types requiring local GUC
--- setting.
+-- The following attempt test various paths at which tuples are formed
+-- and inspected for containment of types requiring local GUC setting.

 -- single row synchronous case
 SELECT *

--
fdr

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

On Tue, Mar 19, 2013 at 10:37 PM, Daniel Farina  wrote:
> I added programming around various NULL returns reading GUCs in this
> revision, v4.

Okay, one more of those fridge-logic bugs.  Sorry for the noise. v5.

A missing PG_RETHROW to get the properly finally-esque semantics:

--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -642,7 +642,10 @@ dblink_fetch(PG_FUNCTION_ARGS)
  }
  PG_CATCH();
  {
+ /* Pop any set GUCs, if necessary */
  restoreLocalGucs(&rgs);
+
+ PG_RE_THROW();
  }
  PG_END_TRY();

This was easy to add a regression test to exercise, and so I did (not
displayed here).

--
fdr

Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

2012/12/7 Albe Laurenz :
> 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 reason why simple_rte_array was not initialized is, invocation of
setup_simple_rel_arrays() was skipped in case of simple INSERT INTO
... VALUES(). So, I put setup_simple_rel_arrays() just after short-cut
code path at query_planner().

> 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.
>
The reason of this assertion trap is, I incorrectly assumed subplan of
ModifyTable is always ForeignScan. However, it should be picked up
it from the subplan tree using recursive node walking.
I newly added lookup_foreign_scan_plan() not to reinvent same logic
for each extension. It returns ForeignScan node that has supplied
rtindex, even if the top of subplan tree is not ForeignScan.

Both of the troubles you reported was fixed with attached patch.
I also added relevant test cases into regression test, please check it.

>> 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.
>
I adjusted the code a bit to work this routine correctly, even if a query
contains two or more foreign table references.

>> 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.
>
Yes. In case of INSERT, its resultRelation is not appeared in the fromList,
thus, no chance to create relevant scan plan towards the table to be inserted.

>> +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.
>
The origin of this asymmetry is ModifyTableState may affect multiple
tables if result relation has inheritance, even though foreign table does
not support inheritance feature right now.
Please see the entrypoint to invoke BeginForeignModify handler.
It is in a loop block that initializes executor state being associated with
thie ModifyTableState.

Even if we have symmetric definition, the variable "i" need to be delivered
to identify which executor state is under initialization at least.
The relevant resultRelInfo, fdwprivate, and subplan can be picked up
from the ModifyTableState using this index. But it enforces extensions
to implement same logic with individual code. My preference is to pick
up necessary information at the code side.

> 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.

Thanks,
-- 
KaiGai Kohei 

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

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.

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.)

Here is a few operation examples.

postgres=# CREATE FOREIGN TABLE tbl (a int, b text, c date) SERVER loopback;
CREATE FOREIGN TABLE
postgres=# SELECT * FROM tbl;
 a |  b  |     c
---+-----+------------
 1 | aaa | 2012-12-01
 2 | bbb | 2012-12-02
 3 | ccc | 2012-12-03
 4 | ddd | 2012-12-04
 5 | eee | 2012-12-05
 6 | fff | 2012-12-06
(6 rows)

postgres=# UPDATE tbl SET b = b || b WHERE a % 2 = 1;
UPDATE 3
postgres=# SELECT * FROM tbl ORDER BY a;
 a |   b    |     c
---+--------+------------
 1 | aaaaaa | 2012-12-01
 2 | bbb    | 2012-12-02
 3 | cccccc | 2012-12-03
 4 | ddd    | 2012-12-04
 5 | eeeeee | 2012-12-05
 6 | fff    | 2012-12-06
(6 rows)

postgres=# INSERT INTO tbl VALUES (7,'ggg'),(8,'hhh');
INSERT 0 2
postgres=# SELECT * FROM tbl ORDER BY a;
 a |   b    |     c
---+--------+------------
 1 | aaaaaa | 2012-12-01
 2 | bbb    | 2012-12-02
 3 | cccccc | 2012-12-03
 4 | ddd    | 2012-12-04
 5 | eeeeee | 2012-12-05
 6 | fff    | 2012-12-06
 7 | ggg    |
 8 | hhh    |
(8 rows)

postgres=# DELETE FROM tbl WHERE a % 2 = 0;
DELETE 4
postgres=# SELECT * FROM tbl ORDER BY a;
 a |   b    |     c
---+--------+------------
 1 | aaaaaa | 2012-12-01
 3 | cccccc | 2012-12-03
 5 | eeeeee | 2012-12-05
 7 | ggg    |
(4 rows)

Even though it still has restriction of transaction control on remote side,
I believe Hanada-san will provide more graceful implementation next to
the first read-only version getting committed.

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.

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.

+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.

+typedef int    (*ExecForeignInsert_function) (ResultRelInfo *resultRelInfo,
+                                          HeapTuple tuple);
+typedef int    (*ExecForeignDelete_function) (ResultRelInfo *resultRelInfo,
+                                          Datum rowid);
+typedef int    (*ExecForeignUpdate_function) (ResultRelInfo *resultRelInfo,
+                                          Datum rowid,
+                                          HeapTuple tuple);
These are not changed.

+typedef void (*EndForeignModify_function) (ResultRelInfo *resultRelInfo);

Also, it is not changed.

Thanks,
-- 
KaiGai Kohei 

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

2012/8/29 Kohei KaiGai :
> 2012/8/28 Kohei KaiGai :
>> 2012/8/28 Tom Lane :
>>> Kohei KaiGai  writes:
>>>>> Would it be too invasive to introduce a new pointer in TupleTableSlot
>>>>> that is NULL for anything but virtual tuples from foreign tables?
>>>
>>>> I'm not certain whether the duration of TupleTableSlot is enough to
>>>> carry a private datum between scan and modify stage.
>>>
>>> It's not.
>>>
>>>> Is it possible to utilize ctid field to move a private pointer?
>>>
>>> UPDATEs and DELETEs do not rely on the ctid field of tuples to carry the
>>> TID from scan to modify --- in fact, most of the time what the modify
>>> step is going to get is a "virtual" TupleTableSlot that hasn't even
>>> *got* a physical CTID field.
>>>
>>> Instead, the planner arranges for the TID to be carried up as an
>>> explicit resjunk column named ctid.  (Currently this is done in
>>> rewriteTargetListUD(), but see also preptlist.c which does some related
>>> things for SELECT FOR UPDATE.)
>>>
>>> I'm inclined to think that what we need here is for FDWs to be able to
>>> modify the details of that behavior, at least to the extent of being
>>> able to specify a different data type than TID for the row
>>> identification column.
>>>
>> Hmm. It seems to me a straight-forward solution rather than ab-use
>> of ctid system column. Probably, cstring data type is more suitable
>> to carry a private datum between scan and modify stage.
>>
>> One problem I noticed is how FDW driver returns an extra field that
>> is in neither system nor regular column.
>> Number of columns and its data type are defined with TupleDesc of
>> the target foreign-table, so we also need a feature to extend it on
>> run-time. For example, FDW driver may have to be able to extend
>> a "virtual" column with cstring data type, even though the target
>> foreign table does not have such a column.
>>
> I tried to investigate the related routines.
>
> TupleDesc of TupleTableSlot associated with ForeignScanState
> is initialized at ExecInitForeignScan as literal.
> ExecAssignScanType assigns TupleDesc of the target foreign-
> table on tts_tupleDescriptor, "as-is".
> It is the reason why IterateForeignScan cannot return a private
> datum except for the columns being declared as regular ones.
>
The attached patch improved its design according to the upthread
discussion. It now got away from ab-use of "ctid" field, and adopts
a concept of pseudo-column to hold row-id with opaque data type
instead.

Pseudo-column is Var reference towards attribute-number larger
than number of attributes on the target relation; thus, it is not
a substantial object. It is normally unavailable to reference such
a larger attribute number because TupleDesc of each ScanState
associated with a particular relation is initialized at ExecInitNode.

The patched ExecInitForeignScan was extended to generate its
own TupleDesc including pseudo-column definitions on the fly,
instead of relation's one, when scan-plan of foreign-table requires
to have pseudo-columns.

Right now, the only possible pseudo-column is "rowid" being
injected at rewriteTargetListUD(). It has no data format
restriction like "ctid" because of VOID data type.
FDW extension can set an appropriate value on the "rowid"
field in addition to contents of regular columns at
IterateForeignScan method, to track which remote row should
be updated or deleted.

Another possible usage of this pseudo-column is push-down
of target-list including complex calculation. It may enable to
move complex mathematical formula into remote devices
(such as GPU device?) instead of just a reference of Var node.

This patch adds a new interface: GetForeignRelInfo being invoked
from get_relation_info() to adjust width of RelOptInfo->attr_needed
according to the target-list which may contain "rowid" pseudo-column.
Some FDW extension may use this interface to push-down a part of
target list into remote side, even though I didn't implement this
feature on file_fdw.

RelOptInfo->max_attr is a good marker whether the plan shall have
pseudo-column reference. Then, ExecInitForeignScan determines
whether it should generate a TupleDesc, or not.

The "rowid" is fetched using ExecGetJunkAttribute as we are currently
doing on regular tables using "ctid", then it shall be delivered to
ExecUpdate or ExecDelete. We can never expect the fist argument of
them now, so "ItemPointer tupleid" redefined to "Datum rowid", and
argument of BR-trigger routines redefined also.

[kaigai@iwashi sepgsql]$ cat ~/testfile.csv
10      aaa
11      bbb
12      ccc
13      ddd
14      eee
15      fff
[kaigai@iwashi sepgsql]$ psql postgres
psql (9.3devel)
Type "help" for help.

postgres=# UPDATE ftbl SET b = md5(b) WHERE a > 12 RETURNING *;
INFO:  ftbl is the target relation of UPDATE
INFO:  fdw_file: BeginForeignModify method
INFO:  fdw_file: UPDATE (lineno = 4)
INFO:  fdw_file: UPDATE (lineno = 5)
INFO:  fdw_file: UPDATE (lineno = 6)
INFO:  fdw_file: EndForeignModify method
 a  |                b
----+----------------------------------
 13 | 77963b7a931377ad4ab5ad6a9cd718aa
 14 | d2f2297d6e829cd3493aa7de4416a18f
 15 | 343d9040a671c45832ee5381860e2996
(3 rows)

UPDATE 3
postgres=# DELETE FROM ftbl WHERE a % 2 = 1 RETURNING *;
INFO:  ftbl is the target relation of DELETE
INFO:  fdw_file: BeginForeignModify method
INFO:  fdw_file: DELETE (lineno = 2)
INFO:  fdw_file: DELETE (lineno = 4)
INFO:  fdw_file: DELETE (lineno = 6)
INFO:  fdw_file: EndForeignModify method
 a  |  b
----+-----
 11 | bbb
 13 | ddd
 15 | fff
(3 rows)

DELETE 3

In addition, there is a small improvement. ExecForeignInsert,
ExecForeignUpdate and ExecForeignDelete get being able
to return number of processed rows; that allows to push-down
whole the statement into remote-side, if it is enough simple
(e.g, delete statement without any condition).

Even though it does not make matter right now, pseudo-columns
should be adjusted when foreign-table is referenced with table
inheritance feature, because an attribute number being enough
large in parent table is not enough large in child table.
We need to fix up them until foreign table feature got inheritance
capability.

I didn't update the documentation stuff because I positioned
the state of this patch as proof-of-concept now. Please note that.

Thanks,
-- 
KaiGai Kohei 

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

The attached patch is just a refreshed version for clean applying to
the latest tree.

As previous version doing, it makes pseudo enhancement on file_fdw
to print something about the supplied tuple on INSERT, UPDATE and
DELETE statement.
Here is one other idea. My GPU acceleration module (PG-Strom)
implements column-oriented data store underlying foreign table.
It might make sense to cut out this portion for proof-of-concept of
writable foreign tables.

Any ideas?

2012/11/8 Atri Sharma :
>
>
> On 08-Nov-2012, at 13:35, "Albe Laurenz"  wrote:
>
>> Alexander Korotkov wrote:
>>> 2) You wrote that FDW can support or don't support write depending on
>> having corresponding functions.
>>> However it's likely some tables of same FDW could be writable while
>> another are not. I think we should
>>> have some mechanism for FDW telling whether particular table is
>> writable.
>>
>> I think that this would best be handled by a table option,
>> if necessary.
>> That allows maximum flexibility for the design of the FDW.
>> In many cases it might be enough if the foreign data source
>> raises an error on a write request.
>>
>> Yours,
>> Laurenz Albe
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>
> +1
>
> I agree, we should have a system where if the foreign data source raises an error on write, FDW can raise corresponding error on PostgreSQL side.exposing this as a table option is IMHO a bit risky, and the user may not know whether the foreign data source will accept writes or not.
>
> Atri



-- 
KaiGai Kohei 

Re: [v9.3] writable foreign tables

2012/12/11 Albe Laurenz :
> 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 

Re: [v9.3] writable foreign tables

2013/2/5 Daniel Farina :
> On Fri, Feb 1, 2013 at 2:22 AM, Daniel Farina  wrote:
>> On Tue, Jan 29, 2013 at 1:19 AM, Kohei KaiGai  wrote:
>>> I noticed the v10 patch cannot be applied on the latest master branch
>>> cleanly. The attached ones were rebased.
>>
>> Anyway, I'm looking at the first patch, which applies neatly. Thanks.
>
> Hello,
>
> My review is incomplete, but to the benefit of pipelining I wanted to
> ask a couple of things and submit some changes for your consideration
> while continuing to look at this.
>
> So far, I've been trying to understand in very close detail how your
> changes affect non-FDW related paths first, before delving into the
> actual writable FDW functionality.  There's not that much in this
> category, but there's one thing that gave me pause: the fact that the
> target list (by convention, tlist) has to be pushed down from
> planmain.c/query_planner all the way to a
> fdwroutine->GetForeignRelWidth callsite in plancat.c/get_relation_info
> (so even in the end, it is just pushed down -- never inspected or
> modified).  In relative terms, it's a significant widening of
> currently fairly short parameter lists in these procedures:
>
> add_base_rels_to_query(PlannerInfo *root, List *tlist, Node *jtnode)
> build_simple_rel(PlannerInfo *root, int relid, List *tlist, RelOptKind
> reloptkind)
> get_relation_info(PlannerInfo *root, Oid relationObjectId, bool
> inhparent, List *tlist, RelOptInfo *rel)
>
> In the case of all the other parameters except tlist, each is more
> intimately related with the inner workings and mechanisms of the
> procedure.  I wonder if there's a nice way that can train the reader's
> eye that the tlist is simply pushed down rather than actively managed
> at each level.  However, I can't immediately produce a way to both
> achieve that that doesn't seem overwrought.  Perhaps a comment will
> suffice.
>
Thanks for your checks.

add_base_rels_to_query() originally has two arguments; PlannerInfo
and Node, but these are wide-spreading in use. I don't think it is a good
idea to add a special field to reduce number of arguments only.
Instead, I tried to add a short comment on top of add_base_rels_to_query()
as follows.

 * XXX - Also note that tlist needs to be pushed down into deeper level,
 * for construction of RelOptInfo relevant to foreign-tables with pseudo-
 * columns.
 */

I'm not 100% sure whether it is a good comment here, because internal
will be revised patch-by-patch. So, if future version also modifies argument
list here, this comment may talks a lie.

> Related to this, I found that make_modifytable grew a dependency on
> PlannerInfo *root, and it seemed like a bunch of the arguments are
> functionally related to that, so the attached patch that should be
> able to be applied to yours tries to utilize that as often as
> possible.  The weirdest thing in there is how make_modifytable has
> been taught to call SS_assign_special_param, which has a side effect
> on the PlannerInfo, but there exist exactly zero cases where one
> *doesn't* want to do that prior to the (exactly two) call sites to
> make_modifytable, so I pushed it in.  The second weirdest thing is
> pushing in the handling of rowMarks (e.g. SELECT FOR UPDATE et al)
>
Good suggestion. I added the PlannerInfo *root with spinal-reflexes.
Indeed, I'd like to agree with your optimization.

The attached patch adds Daniel's reworks on make_modifytable
invocation, and add a short comment on add_base_rels_to_query().
Rest of portion has not been changed from the previous version.

Thanks,
-- 
KaiGai Kohei 

Re: [v9.3] writable foreign tables

2012/9/23 Kohei KaiGai :
> 2012/8/29 Kohei KaiGai :
>> 2012/8/28 Kohei KaiGai :
>>> 2012/8/28 Tom Lane :
>>>> Kohei KaiGai  writes:
>>>>>> Would it be too invasive to introduce a new pointer in TupleTableSlot
>>>>>> that is NULL for anything but virtual tuples from foreign tables?
>>>>
>>>>> I'm not certain whether the duration of TupleTableSlot is enough to
>>>>> carry a private datum between scan and modify stage.
>>>>
>>>> It's not.
>>>>
>>>>> Is it possible to utilize ctid field to move a private pointer?
>>>>
>>>> UPDATEs and DELETEs do not rely on the ctid field of tuples to carry the
>>>> TID from scan to modify --- in fact, most of the time what the modify
>>>> step is going to get is a "virtual" TupleTableSlot that hasn't even
>>>> *got* a physical CTID field.
>>>>
>>>> Instead, the planner arranges for the TID to be carried up as an
>>>> explicit resjunk column named ctid.  (Currently this is done in
>>>> rewriteTargetListUD(), but see also preptlist.c which does some related
>>>> things for SELECT FOR UPDATE.)
>>>>
>>>> I'm inclined to think that what we need here is for FDWs to be able to
>>>> modify the details of that behavior, at least to the extent of being
>>>> able to specify a different data type than TID for the row
>>>> identification column.
>>>>
>>> Hmm. It seems to me a straight-forward solution rather than ab-use
>>> of ctid system column. Probably, cstring data type is more suitable
>>> to carry a private datum between scan and modify stage.
>>>
>>> One problem I noticed is how FDW driver returns an extra field that
>>> is in neither system nor regular column.
>>> Number of columns and its data type are defined with TupleDesc of
>>> the target foreign-table, so we also need a feature to extend it on
>>> run-time. For example, FDW driver may have to be able to extend
>>> a "virtual" column with cstring data type, even though the target
>>> foreign table does not have such a column.
>>>
>> I tried to investigate the related routines.
>>
>> TupleDesc of TupleTableSlot associated with ForeignScanState
>> is initialized at ExecInitForeignScan as literal.
>> ExecAssignScanType assigns TupleDesc of the target foreign-
>> table on tts_tupleDescriptor, "as-is".
>> It is the reason why IterateForeignScan cannot return a private
>> datum except for the columns being declared as regular ones.
>>
> The attached patch improved its design according to the upthread
> discussion. It now got away from ab-use of "ctid" field, and adopts
> a concept of pseudo-column to hold row-id with opaque data type
> instead.
>
> Pseudo-column is Var reference towards attribute-number larger
> than number of attributes on the target relation; thus, it is not
> a substantial object. It is normally unavailable to reference such
> a larger attribute number because TupleDesc of each ScanState
> associated with a particular relation is initialized at ExecInitNode.
>
> The patched ExecInitForeignScan was extended to generate its
> own TupleDesc including pseudo-column definitions on the fly,
> instead of relation's one, when scan-plan of foreign-table requires
> to have pseudo-columns.
>
> Right now, the only possible pseudo-column is "rowid" being
> injected at rewriteTargetListUD(). It has no data format
> restriction like "ctid" because of VOID data type.
> FDW extension can set an appropriate value on the "rowid"
> field in addition to contents of regular columns at
> IterateForeignScan method, to track which remote row should
> be updated or deleted.
>
> Another possible usage of this pseudo-column is push-down
> of target-list including complex calculation. It may enable to
> move complex mathematical formula into remote devices
> (such as GPU device?) instead of just a reference of Var node.
>
> This patch adds a new interface: GetForeignRelInfo being invoked
> from get_relation_info() to adjust width of RelOptInfo->attr_needed
> according to the target-list which may contain "rowid" pseudo-column.
> Some FDW extension may use this interface to push-down a part of
> target list into remote side, even though I didn't implement this
> feature on file_fdw.
>
> RelOptInfo->max_attr is a good marker whether the plan shall have
> pseudo-column reference. Then, ExecInitForeignScan determines
> whether it should generate a TupleDesc, or not.
>
> The "rowid" is fetched using ExecGetJunkAttribute as we are currently
> doing on regular tables using "ctid", then it shall be delivered to
> ExecUpdate or ExecDelete. We can never expect the fist argument of
> them now, so "ItemPointer tupleid" redefined to "Datum rowid", and
> argument of BR-trigger routines redefined also.
>
> [kaigai@iwashi sepgsql]$ cat ~/testfile.csv
> 10      aaa
> 11      bbb
> 12      ccc
> 13      ddd
> 14      eee
> 15      fff
> [kaigai@iwashi sepgsql]$ psql postgres
> psql (9.3devel)
> Type "help" for help.
>
> postgres=# UPDATE ftbl SET b = md5(b) WHERE a > 12 RETURNING *;
> INFO:  ftbl is the target relation of UPDATE
> INFO:  fdw_file: BeginForeignModify method
> INFO:  fdw_file: UPDATE (lineno = 4)
> INFO:  fdw_file: UPDATE (lineno = 5)
> INFO:  fdw_file: UPDATE (lineno = 6)
> INFO:  fdw_file: EndForeignModify method
>  a  |                b
> ----+----------------------------------
>  13 | 77963b7a931377ad4ab5ad6a9cd718aa
>  14 | d2f2297d6e829cd3493aa7de4416a18f
>  15 | 343d9040a671c45832ee5381860e2996
> (3 rows)
>
> UPDATE 3
> postgres=# DELETE FROM ftbl WHERE a % 2 = 1 RETURNING *;
> INFO:  ftbl is the target relation of DELETE
> INFO:  fdw_file: BeginForeignModify method
> INFO:  fdw_file: DELETE (lineno = 2)
> INFO:  fdw_file: DELETE (lineno = 4)
> INFO:  fdw_file: DELETE (lineno = 6)
> INFO:  fdw_file: EndForeignModify method
>  a  |  b
> ----+-----
>  11 | bbb
>  13 | ddd
>  15 | fff
> (3 rows)
>
> DELETE 3
>
> In addition, there is a small improvement. ExecForeignInsert,
> ExecForeignUpdate and ExecForeignDelete get being able
> to return number of processed rows; that allows to push-down
> whole the statement into remote-side, if it is enough simple
> (e.g, delete statement without any condition).
>
> Even though it does not make matter right now, pseudo-columns
> should be adjusted when foreign-table is referenced with table
> inheritance feature, because an attribute number being enough
> large in parent table is not enough large in child table.
> We need to fix up them until foreign table feature got inheritance
> capability.
>
> I didn't update the documentation stuff because I positioned
> the state of this patch as proof-of-concept now. Please note that.
>
A tiny bit of this patch was updated. I noticed INTERNAL data type
is more suitable to move a private datum from scan-stage to
modify-stage, because its type-length is declared as SIZEOF_POINTER.

Also, I fixed up some obvious compiler warnings.

Thanks,
-- 
KaiGai Kohei 

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

[v9.3] writable foreign tables

Hello,

The attached patch is just a proof-of-concept of writable foreign table
feature; thus, I don't expect it getting merged at the upcoming commit
fest. The purpose of this patch is to find out the best way to support
"write stuff" in FDW.

Basic idea of this patch is to utilize "ctid" field to track an identifier of
a particular foreign-row; to be updated or deleted. It shall be moved
to the modify-stage from the scan-stage as regular table doing.
Then, newly added methods being invoked at ExecUpdate or
ExecDelete with the "pseudo ctid", so FDW shall be able to modify
the target foreign-row.
It is a responsibility of FDW extension (and DBA) to ensure each
foreign-row has a unique identifier that has 48-bits width integer
data type in maximum. In case of pgsql_fdw, "ctid" of remote table
can perform as "pseudo ctid", as is. For other RDBMS, DBA will
need to indicate which column should perform.
INSERT is simple enough; all we need to do it to carry every field
of new tuple into the remote side.

This patch adds five new methods of FdwRoutine structure.
The BeginForeignModify and EndForeignModify give a chance
to initialize / destruct a private state that can be allocated on
ResultRelInfo. As literal, ExecForeignInsert, ExecForeignDelete
and ExecForeignUpdate are invoked for each new tuple, instead
of heap_*() for regular tables. If NULL was set on them, it means
this FDW does not support these operations.

I intend FDW to set up a prepared statement that modifies
a particular remote-row being identified with pseudo-ctid,
at the BeginForeignModify(). Then, ExecForeign*() kicks
the prepared statement with given pseudo-ctid.

The patched portion at contrib/file_fdw.c does not make sense
actually. It just prints messages for each invocation.
It is just a proof-of-concept to show possibility of implementation
based on real RDBMS.

In case when file_fdw performs behalf on "ftbl".
--------------------------------
postgres=# SELECT ctid, * FROM ftbl;
  ctid  |  a  |  b
--------+-----+-----
 (0,1)  | 101 | aaa
 (0,2)  | 102 | bbb
 (0,3)  | 103 | ccc
 (0,4)  | 104 | ddd
 (0,5)  | 105 | eee
 (0,6)  | 106 | fff
 (0,7)  | 107 | ggg
 (0,8)  | 108 | hhh
 (0,9)  | 109 | iii
 (0,10) | 110 | jjj
(10 rows)
 ==> ctid is used to carray identifier of row; line number in this example.

postgres=# UPDATE ftbl SET a = a + 1 WHERE a > 107;
INFO:  ftbl is the target relation of UPDATE
INFO:  fdw_file: BeginForeignModify method
INFO:  fdw_file: UPDATE (lineno = 8)
INFO:  fdw_file: UPDATE (lineno = 9)
INFO:  fdw_file: UPDATE (lineno = 10)
INFO:  fdw_file: EndForeignModify method
UPDATE 3
postgres=# DELETE FROM ftbl WHERE a BETWEEN 103 AND 106;
INFO:  ftbl is the target relation of DELETE
INFO:  fdw_file: BeginForeignModify method
INFO:  fdw_file: DELETE (lineno = 3)
INFO:  fdw_file: DELETE (lineno = 4)
INFO:  fdw_file: DELETE (lineno = 5)
INFO:  fdw_file: DELETE (lineno = 6)
INFO:  fdw_file: EndForeignModify method
DELETE 4
--------------------------------

This patch does not care about transaction control anyway.
According to the discussion in developer meeting at Ottawa,
I didn't include such a complex stuff in the first step.
(Probably, we can implement using XactCallback...)

Thanks,
-- 
KaiGai Kohei 

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

The attached patches are revised version for before-row triggers or
default setting.

I adjusted APIs to allow FDW drivers to modify the supplied slot
according to the row
being actually inserted / updated on the remote side.
Now, relevant handlers are declared as follows:

TupleTableSlot *
ExecForeignInsert (ResultRelInfo *resultRelInfo,
                   TupleTableSlot *slot);

TupleTableSlot *
ExecForeignUpdate (ResultRelInfo *resultRelInfo,
                   const char *rowid,
                   TupleTableSlot *slot);

bool
ExecForeignDelete (ResultRelInfo *resultRelInfo,
                   const char *rowid);

If nothing were modified, insert or update handler usually returns the supplied
slot as is. Elsewhere, it can return a modified image of tuple according to the
result of remote query execution, including default setting or
before-row trigger.

Let's see the following examples with enhanced postgres_fdw.

postgres=# CREATE FOREIGN TABLE ft1 (a int, b text, c date) SERVER
loopback OPTIONS(relname 't1');
CREATE FOREIGN TABLE
postgres=# SELECT * FROM ft1;
 a |  b  |     c
---+-----+------------
 1 | aaa | 2012-12-10
 2 | bbb | 2012-12-15
 3 | ccc | 2012-12-20
(3 rows)

==> ft1 is a foreign table behalf of t1 on loopback server.

postgres=# ALTER TABLE t1 ALTER c SET DEFAULT current_date;
ALTER TABLE
postgres=# CREATE OR REPLACE FUNCTION t1_trig_func() RETURNS trigger AS $$
postgres$# BEGIN
postgres$#   NEW.b = NEW.b || '_trig_update';
postgres$#   RETURN NEW;
postgres$# END;
postgres$# $$ LANGUAGE plpgsql;
CREATE FUNCTION
postgres=# CREATE TRIGGER t1_br_trig BEFORE INSERT OR UPDATE ON t1 FOR
EACH ROW EXECUTE PROCEDURE t1_trig_func();
CREATE TRIGGER

==> The "t1" table has default setting and before-row triggers.

postgres=# INSERT INTO ft1 VALUES (4,'ddd','2012-12-05'),
(5,'eee',null) RETURNING *;
 a |        b        |     c
---+-----------------+------------
 4 | ddd_trig_update | 2012-12-05
 5 | eee_trig_update |
(2 rows)

INSERT 0 2
postgres=# INSERT INTO ft1 VALUES (6, 'fff') RETURNING *;
 a |        b        |     c
---+-----------------+------------
 6 | fff_trig_update | 2012-12-23
(1 row)

INSERT 0 1

==> RETURNING clause can back modified image on the remote side.
       Please note that the results are affected by default-setting and
       before-row procedure on remote side.

postgres=# UPDATE ft1 SET a = a + 100 RETURNING *;
  a  |              b              |     c
-----+-----------------------------+------------
 101 | aaa_trig_update             | 2012-12-10
 102 | bbb_trig_update             | 2012-12-15
 103 | ccc_trig_update             | 2012-12-20
 104 | ddd_trig_update_trig_update | 2012-12-05
 105 | eee_trig_update_trig_update |
 106 | fff_trig_update_trig_update | 2012-12-23
(6 rows)

UPDATE 6

==> update command also

The modified API lost a feature to return the number of rows being affected
that might be useful in case when underlying query multiple rows on the
remote side. However, I rethought it does not really make sense.
If FDW driver support a feature to push down whole UPDATE or DELETE
command towards simple queries, it is not so difficult to cache the result
of the query that affect multiple rows, then the relevant routine can pop
the cached result for each invocation without remote query execution on
demand.

Thanks,

2012/12/18 Kohei KaiGai :
> Hi,
>
> 2012/12/18 Ronan Dunklau :
>> Hello.
>>
>> I've tried to implement this API for our Multicorn FDW, and I have a few
>> questions about the API.
>>
>> First, I don't understand what are the requirements on the "rowid"
>> pseudo-column values.
>>
>> In particular, this sentence from a previous mail makes it ambiguous to me:
>>
>>
>>> 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 *".
>>
>> Does that mean that one should only store a cstring in the rowid
>> pseudo-column ? Or can one store an arbitrary pointer ? Currently, I'm
>> building a text value using cstring_to_text_with_len. Could there be a
>> problem with that ?
>>
> All what we require on the rowid pseudo-column is it has capability to
> identify a particular row on the remote side. In case of postgres_fdw,
> ctid of the relevant table is sufficient for the purpose.
> I don't recommend to set the rowid field an arbitrary pointer, because
> scanned tuple may be materialized between scanning and modifying
> foreign table, thus, the arbitrary pointer shall be dealt under the
> assumption of cstring data type.
>
>> Secondly, how does one use a returning clause ?
>> I've tried to look at the postgres_fdw code, but it does not seem to handle
>> that.
>>
>> For what its worth, knowing that the postgres_fdw is still in WIP status,
>> here is a couple result of my experimentation with it:
>>
>> - Insert into a foreign table mapped to a table with a "before" trigger,
>> using a returning clause, will return the values as they were before being
>> modified by the trigger.
>> - Same thing,  but if the trigger prevents insertion (returning NULL), the
>> "would-have-been" inserted row is still returned, although the insert
>> statement reports zero rows.
>>
> Hmm. Do you want to see the "real" final contents of the rows being inserted,
> don't you. Yep, the proposed interface does not have capability to modify
> the supplied tuple on ExecForeignInsert or other methods.
>
> Probably, it needs to adjust interface to allow FDW drivers to return
> a modified HeapTuple or TupleTableSlot for RETURNING calculation.
> (As trigger doing, it can return the given one as-is if no change)
>
> Let me investigate the code around this topic.
>
>> - Inserting into a table with default values does not work as intended,
>> since missing values are replaced by a null value in the remote statement.
>>
> It might be a bug of my proof-of-concept portion at postgres_fdw.
> The prepared INSERT statement should list up columns being actually
> used only, not all ones unconditionally.
>
> Thanks,
>
>> What can be done to make the behaviour more consistent ?
>>
>> I'm very excited about this feature, thank you for making this possible.
>>
>> Regards,
>> --
>> Ronan Dunklau
>>
>> 2012/12/14 Albe Laurenz 
>>>
>>> Kohei KaiGai wrote:
>>> >> I came up with one more query that causes a problem:
>>> [...]
>>> >> 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.
>>>
>>> Fair enough.
>>>
>>> >> 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 have updated the documentation, that was all I changed in the
>>> attached patches.
>>>
>>> > OK. I split the patch into two portion, part-1 is the APIs relevant
>>> > patch, part-2 is relevant to postgres_fdw patch.
>>>
>>> Great.
>>>
>>> I'll mark the patch as "ready for committer".
>>>
>>> Yours,
>>> Laurenz Albe
>>>
>>>
>>> --
>>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>>> To make changes to your subscription:
>>> http://www.postgresql.org/mailpref/pgsql-hackers
>>>
>>
>
>
>
> --
> KaiGai Kohei 



-- 
KaiGai Kohei 

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

I noticed the v10 patch cannot be applied on the latest master branch
cleanly. The attached ones were rebased.

I also noticed that deparse_expression() assumes relation never has
columns with attribute number equal or larger than number of columns
of the relation, thus it makes a problem when EXPLAIN shows a plan
tree including Var node towards pseudo-column.
So, I adjusted the code to construct column-name array being used
for EXPLAIN output.

I don't touch the part-2 portion that extends Hanada-san's postgres_fdw
v5 patch.

Thanks,

2012/12/23 Kohei KaiGai :
> The attached patches are revised version for before-row triggers or
> default setting.
>
> I adjusted APIs to allow FDW drivers to modify the supplied slot
> according to the row
> being actually inserted / updated on the remote side.
> Now, relevant handlers are declared as follows:
>
> TupleTableSlot *
> ExecForeignInsert (ResultRelInfo *resultRelInfo,
>                    TupleTableSlot *slot);
>
> TupleTableSlot *
> ExecForeignUpdate (ResultRelInfo *resultRelInfo,
>                    const char *rowid,
>                    TupleTableSlot *slot);
>
> bool
> ExecForeignDelete (ResultRelInfo *resultRelInfo,
>                    const char *rowid);
>
> If nothing were modified, insert or update handler usually returns the supplied
> slot as is. Elsewhere, it can return a modified image of tuple according to the
> result of remote query execution, including default setting or
> before-row trigger.
>
> Let's see the following examples with enhanced postgres_fdw.
>
> postgres=# CREATE FOREIGN TABLE ft1 (a int, b text, c date) SERVER
> loopback OPTIONS(relname 't1');
> CREATE FOREIGN TABLE
> postgres=# SELECT * FROM ft1;
>  a |  b  |     c
> ---+-----+------------
>  1 | aaa | 2012-12-10
>  2 | bbb | 2012-12-15
>  3 | ccc | 2012-12-20
> (3 rows)
>
> ==> ft1 is a foreign table behalf of t1 on loopback server.
>
> postgres=# ALTER TABLE t1 ALTER c SET DEFAULT current_date;
> ALTER TABLE
> postgres=# CREATE OR REPLACE FUNCTION t1_trig_func() RETURNS trigger AS $$
> postgres$# BEGIN
> postgres$#   NEW.b = NEW.b || '_trig_update';
> postgres$#   RETURN NEW;
> postgres$# END;
> postgres$# $$ LANGUAGE plpgsql;
> CREATE FUNCTION
> postgres=# CREATE TRIGGER t1_br_trig BEFORE INSERT OR UPDATE ON t1 FOR
> EACH ROW EXECUTE PROCEDURE t1_trig_func();
> CREATE TRIGGER
>
> ==> The "t1" table has default setting and before-row triggers.
>
> postgres=# INSERT INTO ft1 VALUES (4,'ddd','2012-12-05'),
> (5,'eee',null) RETURNING *;
>  a |        b        |     c
> ---+-----------------+------------
>  4 | ddd_trig_update | 2012-12-05
>  5 | eee_trig_update |
> (2 rows)
>
> INSERT 0 2
> postgres=# INSERT INTO ft1 VALUES (6, 'fff') RETURNING *;
>  a |        b        |     c
> ---+-----------------+------------
>  6 | fff_trig_update | 2012-12-23
> (1 row)
>
> INSERT 0 1
>
> ==> RETURNING clause can back modified image on the remote side.
>        Please note that the results are affected by default-setting and
>        before-row procedure on remote side.
>
> postgres=# UPDATE ft1 SET a = a + 100 RETURNING *;
>   a  |              b              |     c
> -----+-----------------------------+------------
>  101 | aaa_trig_update             | 2012-12-10
>  102 | bbb_trig_update             | 2012-12-15
>  103 | ccc_trig_update             | 2012-12-20
>  104 | ddd_trig_update_trig_update | 2012-12-05
>  105 | eee_trig_update_trig_update |
>  106 | fff_trig_update_trig_update | 2012-12-23
> (6 rows)
>
> UPDATE 6
>
> ==> update command also
>
> The modified API lost a feature to return the number of rows being affected
> that might be useful in case when underlying query multiple rows on the
> remote side. However, I rethought it does not really make sense.
> If FDW driver support a feature to push down whole UPDATE or DELETE
> command towards simple queries, it is not so difficult to cache the result
> of the query that affect multiple rows, then the relevant routine can pop
> the cached result for each invocation without remote query execution on
> demand.
>
> Thanks,
>
> 2012/12/18 Kohei KaiGai :
>> Hi,
>>
>> 2012/12/18 Ronan Dunklau :
>>> Hello.
>>>
>>> I've tried to implement this API for our Multicorn FDW, and I have a few
>>> questions about the API.
>>>
>>> First, I don't understand what are the requirements on the "rowid"
>>> pseudo-column values.
>>>
>>> In particular, this sentence from a previous mail makes it ambiguous to me:
>>>
>>>
>>>> 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 *".
>>>
>>> Does that mean that one should only store a cstring in the rowid
>>> pseudo-column ? Or can one store an arbitrary pointer ? Currently, I'm
>>> building a text value using cstring_to_text_with_len. Could there be a
>>> problem with that ?
>>>
>> All what we require on the rowid pseudo-column is it has capability to
>> identify a particular row on the remote side. In case of postgres_fdw,
>> ctid of the relevant table is sufficient for the purpose.
>> I don't recommend to set the rowid field an arbitrary pointer, because
>> scanned tuple may be materialized between scanning and modifying
>> foreign table, thus, the arbitrary pointer shall be dealt under the
>> assumption of cstring data type.
>>
>>> Secondly, how does one use a returning clause ?
>>> I've tried to look at the postgres_fdw code, but it does not seem to handle
>>> that.
>>>
>>> For what its worth, knowing that the postgres_fdw is still in WIP status,
>>> here is a couple result of my experimentation with it:
>>>
>>> - Insert into a foreign table mapped to a table with a "before" trigger,
>>> using a returning clause, will return the values as they were before being
>>> modified by the trigger.
>>> - Same thing,  but if the trigger prevents insertion (returning NULL), the
>>> "would-have-been" inserted row is still returned, although the insert
>>> statement reports zero rows.
>>>
>> Hmm. Do you want to see the "real" final contents of the rows being inserted,
>> don't you. Yep, the proposed interface does not have capability to modify
>> the supplied tuple on ExecForeignInsert or other methods.
>>
>> Probably, it needs to adjust interface to allow FDW drivers to return
>> a modified HeapTuple or TupleTableSlot for RETURNING calculation.
>> (As trigger doing, it can return the given one as-is if no change)
>>
>> Let me investigate the code around this topic.
>>
>>> - Inserting into a table with default values does not work as intended,
>>> since missing values are replaced by a null value in the remote statement.
>>>
>> It might be a bug of my proof-of-concept portion at postgres_fdw.
>> The prepared INSERT statement should list up columns being actually
>> used only, not all ones unconditionally.
>>
>> Thanks,
>>
>>> What can be done to make the behaviour more consistent ?
>>>
>>> I'm very excited about this feature, thank you for making this possible.
>>>
>>> Regards,
>>> --
>>> Ronan Dunklau
>>>
>>> 2012/12/14 Albe Laurenz 
>>>>
>>>> Kohei KaiGai wrote:
>>>> >> I came up with one more query that causes a problem:
>>>> [...]
>>>> >> 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.
>>>>
>>>> Fair enough.
>>>>
>>>> >> 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 have updated the documentation, that was all I changed in the
>>>> attached patches.
>>>>
>>>> > OK. I split the patch into two portion, part-1 is the APIs relevant
>>>> > patch, part-2 is relevant to postgres_fdw patch.
>>>>
>>>> Great.
>>>>
>>>> I'll mark the patch as "ready for committer".
>>>>
>>>> Yours,
>>>> Laurenz Albe
>>>>
>>>>
>>>> --
>>>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>>>> To make changes to your subscription:
>>>> http://www.postgresql.org/mailpref/pgsql-hackers
>>>>
>>>
>>
>>
>>
>> --
>> KaiGai Kohei 
>
>
>
> --
> KaiGai Kohei 



-- 
KaiGai Kohei 

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables

Re: [v9.3] writable foreign tables