From: | Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, Thom Brown <thom(at)linux(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Optimization for updating foreign tables in Postgres FDW |
Date: | 2016-01-26 10:45:18 |
Message-ID: | 56A74E3E.5080504@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2016/01/25 17:03, Rushabh Lathia wrote:
> Here are couple of comments:
> 1)
>
> int
> IsForeignRelUpdatable (Relation rel);
> Documentation for IsForeignUpdatable() need to change as it says:
>
> If the IsForeignRelUpdatable pointer is set to NULL, foreign tables are
> assumed
> to be insertable, updatable, or deletable if the FDW provides
> ExecForeignInsert,
> ExecForeignUpdate or ExecForeignDelete respectively.
>
> With introduce of DMLPushdown API now this is no more correct, as even if
> FDW don't provide ExecForeignInsert, ExecForeignUpdate or
> ExecForeignDelete API
> still foreign tables are assumed to be updatable or deletable with
> DMLPushdown
> API's, right ?
That's what I'd like to discuss.
I intentionally leave that as-is, because I think we should determine
the updatability of a foreign table in the current manner. As you
pointed out, even if the FDW doesn't provide eg, ExecForeignUpdate, an
UPDATE on a foreign table could be done using the DML pushdown APIs if
the UPDATE is *pushdown-safe*. However, since all UPDATEs on the
foreign table are not necessarily pushdown-safe, I'm not sure it's a
good idea to assume the table-level updatability if the FDW provides the
DML pushdown callback routines. To keep the existing updatability
decision, I think the FDW should provide the DML pushdown callback
routines together with ExecForeignInsert, ExecForeignUpdate, or
ExecForeignDelete. What do you think about that?
> 2)
>
> + /* SQL statement to execute remotely (as a String node) */
> + FdwDmlPushdownPrivateUpdateSql,
>
> FdwDmlPushdownPrivateUpdateSql holds the UPDATE/DELETE query, so name should
> be something like FdwDmlPushdownPrivateQuery os FdwDmlPushdownPrivateSql ?
> Later I realized that for FdwModifyPrivateIndex too the index name is
> FdwModifyPrivateUpdateSql even though its holding any DML query. Not sure
> whether we should consider to change this or not ?
To tell the truth, I imitated FdwModifyPrivateIndex when adding
FdwDmlPushdownPrivateIndex, because I think "UpdateSql" means INSERT,
UPDATE, or DELETE, not just UPDATE. (IsForeignRelUpdatable discussed
above reports not only the updatability but the insertability and
deletability of a foreign table!). So, +1 for leaving that as-is.
> Apart from this perform sanity testing on the new patch and things working
> as expected.
Thanks for the review!
Best regards,
Etsuro Fujita
From | Date | Subject | |
---|---|---|---|
Next Message | Sebastien Diemer | 2016-01-26 11:18:31 | Re: pglogical most basic setup for logical replication |
Previous Message | Marko Tiikkaja | 2016-01-26 10:42:33 | Re: count_nulls(VARIADIC "any") |