On Wed, Feb 3, 2016 at 3:31 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp>
wrote:
> On 2016/01/28 15:20, Rushabh Lathia wrote:
>
>> On Thu, Jan 28, 2016 at 11:33 AM, Etsuro Fujita
>> <fujita.ets...@lab.ntt.co.jp <mailto:fujita.ets...@lab.ntt.co.jp>> wrote:
>>
>> On 2016/01/27 21:23, Rushabh Lathia wrote:
>>
>> If I understood correctly, above documentation means, that if
>> FDW have
>> DMLPushdown APIs that is enough. But in reality thats not the
>> case, we
>> need ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete
>> in case
>> DML is not pushable.
>>
>> And here fact is DMLPushdown APIs are optional for FDW, so that
>> if FDW
>> don't have DMLPushdown APIs they can still very well perform the
>> DML
>> operations using ExecForeignInsert, ExecForeignUpdate, or
>> ExecForeignDelete.
>>
>
> So documentation should be like:
>>
>> 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,
>>
>> If FDW provides DMLPushdown APIs and the DML are pushable to the
>> foreign
>> server, then FDW still needs ExecForeignInsert,
>> ExecForeignUpdate, or
>> ExecForeignDelete for the non-pushable DML operation.
>>
>> What's your opinion ?
>>
>
> I agree that we should add this to the documentation, too.
>>
>
> I added docs to the IsForeignRelUpdatable documentation. Also, a brief
> introductory remark has been added at the beginning of the DML pushdown
> APIs' documentation.
>
> BTW, if I understand correctly, I think we should also modify
>> relation_is_updatabale() accordingly. Am I right?
>>
>
> Yep, we need to modify relation_is_updatable().
>>
>
> I thought I'd modify that function in the same way as
> CheckValidResultRel(), but I noticed that we cannot do that, because we
> don't have any information on whether each update is pushed down to the
> remote server by PlanDMLPushdown, during relation_is_updatabale().
Sorry I didn't get you here. Can't resultRelInfo->ri_FdwPushdown gives
information update whether update is pushed down safe or not ? What my
concern here is, lets say resultRelInfo->ri_FdwPushdown marked as true
(PlanDMLPushdown return true), but later into CheckValidResultRel() it
found out that missing BeginDMLPushdown, IterateDMLPushdown and
EndDMLPushdown APIs and it will end up with error.
What I think CheckValidResultRel() should do is, if
resultRelInfo->ri_FdwPushdown is true then check for the DMLPushdown API
and if it doesn't find those API then check for traditional APIs
(ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete). And when it
doesn't find both then it should return an error.
I changed CheckValidResultRel(), where
1) Don't throw an error if resultRelInfo->ri_FdwPushdown is true and
DMLPushdown APIs are missing as query can still perform operation with
traditional ExecForeign APIs. So in this situation just marked
resultRelInfo->ri_FdwPushdown to false.
(Wondering can we add the checks for DMLPushdown APIs into PlanDMLPushdown
as additional check? Means PlanDMLPushdown should return true only if FDW
provides the BeginDMLPushdown & IterateDMLPushdown & EndDMLPushdown APIs ?
What you say ?)
2) Don't throw an error if resultRelInfo->ri_FdwPushdown is true and
DMLPushdown APIs is present but ExecForeign APIs are missing.
3) Throw an error if resultRelInfo->ri_FdwPushdown is false and ExecForeign
APIs are missing.
Attaching is the WIP patch here, do share your thought.
(need to apply on top of V6 patch)
So, I left that function as-is. relation_is_updatabale() is just used for
> display in the information_schema views, so ISTM that that function is fine
> as-is. (As for CheckValidResultRel(), I revised it so as to check the
> presence of DML pushdown APIs after checking the existing APIs if the given
> command will be pushed down. The reason is because we assume the presence
> of the existing APIs, anyway.)
>
>
I revised other docs and some comments, mostly for consistency.
>
> Attached is an updated version of the patch, which has been created on top
> of the updated version of the bugfix patch posted by Robert in [1]
> (attached).
>
>
> Best regards,
> Etsuro Fujita
>
> [1]
> http://www.postgresql.org/message-id/ca+tgmoz40j2uc5ac1nxu03oj4crvolks15xx+ptfp-1u-8z...@mail.gmail.com
>
--
Rushabh Lathia
diff --git a/src/backend/executor/.execMain.c.swp b/src/backend/executor/.execMain.c.swp
index 6f54fd9..8a29cc6 100644
Binary files a/src/backend/executor/.execMain.c.swp and b/src/backend/executor/.execMain.c.swp differ
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index eb24051..04d90ea 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1082,86 +1082,88 @@ CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation)
RelationGetRelationName(resultRel))));
break;
case RELKIND_FOREIGN_TABLE:
- /* Okay only if the FDW supports it */
- fdwroutine = GetFdwRoutineForRelation(resultRel, false);
- switch (operation)
- {
- case CMD_INSERT:
- if (fdwroutine->ExecForeignInsert == NULL)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot insert into foreign table \"%s\"",
- RelationGetRelationName(resultRel))));
- if (fdwroutine->IsForeignRelUpdatable != NULL &&
- (fdwroutine->IsForeignRelUpdatable(resultRel) & (1 << CMD_INSERT)) == 0)
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("foreign table \"%s\" does not allow inserts",
- RelationGetRelationName(resultRel))));
- break;
- case CMD_UPDATE:
- if (fdwroutine->ExecForeignUpdate == NULL)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot update foreign table \"%s\"",
- RelationGetRelationName(resultRel))));
- if (fdwroutine->IsForeignRelUpdatable != NULL &&
- (fdwroutine->IsForeignRelUpdatable(resultRel) & (1 << CMD_UPDATE)) == 0)
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("foreign table \"%s\" does not allow updates",
- RelationGetRelationName(resultRel))));
- break;
- case CMD_DELETE:
- if (fdwroutine->ExecForeignDelete == NULL)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot delete from foreign table \"%s\"",
- RelationGetRelationName(resultRel))));
- if (fdwroutine->IsForeignRelUpdatable != NULL &&
- (fdwroutine->IsForeignRelUpdatable(resultRel) & (1 << CMD_DELETE)) == 0)
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("foreign table \"%s\" does not allow deletes",
- RelationGetRelationName(resultRel))));
- break;
- default:
- elog(ERROR, "unrecognized CmdType: %d", (int) operation);
- break;
- }
- if (resultRelInfo->ri_FdwPushdown)
{
- if (fdwroutine->BeginDMLPushdown == NULL ||
- fdwroutine->IterateDMLPushdown == NULL ||
- fdwroutine->EndDMLPushdown == NULL)
+ /*
+ * Mark DMLPushdownOk if DML is pushed down safe and FDW provides
+ * the DMLPushdown APIs. Later if ExecForeignInsert or
+ * ExecForeignUpdate or ExecForeignDelete is missing and
+ * DMLPushdownOk is false, then only throw an error.
+ */
+ bool DMLPushdownOk = false;
+ /* Okay only if the FDW supports it */
+ fdwroutine = GetFdwRoutineForRelation(resultRel, false);
+ if (resultRelInfo->ri_FdwPushdown)
{
- switch (operation)
+ if (fdwroutine->BeginDMLPushdown == NULL ||
+ fdwroutine->IterateDMLPushdown == NULL ||
+ fdwroutine->EndDMLPushdown == NULL)
{
- case CMD_INSERT:
+ switch (operation)
+ {
+ case CMD_INSERT:
+ case CMD_UPDATE:
+ case CMD_DELETE:
+ DMLPushdownOk = false;
+ break;
+ default:
+ elog(ERROR, "unrecognized CmdType: %d", (int) operation);
+ break;
+ }
+ }
+ else
+ DMLPushdownOk = true;
+ }
+
+ switch (operation)
+ {
+ case CMD_INSERT:
+ if (fdwroutine->ExecForeignInsert == NULL &&
+ !DMLPushdownOk)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot push down insert on foreign table \"%s\"",
+ errmsg("cannot insert into foreign table \"%s\"",
RelationGetRelationName(resultRel))));
- break;
- case CMD_UPDATE:
+ if (fdwroutine->IsForeignRelUpdatable != NULL &&
+ (fdwroutine->IsForeignRelUpdatable(resultRel) & (1 << CMD_INSERT)) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("foreign table \"%s\" does not allow inserts",
+ RelationGetRelationName(resultRel))));
+ break;
+ case CMD_UPDATE:
+ if (fdwroutine->ExecForeignUpdate == NULL &&
+ !DMLPushdownOk)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot push down update on foreign table \"%s\"",
+ errmsg("cannot update foreign table \"%s\"",
RelationGetRelationName(resultRel))));
- break;
- case CMD_DELETE:
+ if (fdwroutine->IsForeignRelUpdatable != NULL &&
+ (fdwroutine->IsForeignRelUpdatable(resultRel) & (1 << CMD_UPDATE)) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("foreign table \"%s\" does not allow updates",
+ RelationGetRelationName(resultRel))));
+ break;
+ case CMD_DELETE:
+ if (fdwroutine->ExecForeignDelete == NULL &&
+ !DMLPushdownOk)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot push down delete on foreign table \"%s\"",
+ errmsg("cannot delete from foreign table \"%s\"",
RelationGetRelationName(resultRel))));
- break;
- default:
- elog(ERROR, "unrecognized CmdType: %d", (int) operation);
- break;
- }
+ if (fdwroutine->IsForeignRelUpdatable != NULL &&
+ (fdwroutine->IsForeignRelUpdatable(resultRel) & (1 << CMD_DELETE)) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("foreign table \"%s\" does not allow deletes",
+ RelationGetRelationName(resultRel))));
+ break;
+ default:
+ elog(ERROR, "unrecognized CmdType: %d", (int) operation);
+ break;
}
+ break;
}
- break;
default:
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
