de-deduplicate code in DML execution hooks in postgres_fdw
Hi,
While working on a fix related to non-direct DML [1]/messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com, I noticed that
postgresExecForeignInsert(), postgresExecForeignUpdate() and
postgresExecForeignDelete() functions are almost identical except that
postgresExecForeignInsert() doesn't require ctid. The fix that I was
working is applicable to Delete and Update but can be useful for
Insert as well. I had to add the same code to two places at least and
might have missed fixing one of them. Why don't we have a single
function which prepares the statement, extract parameters, executes
the prepared statement and checks for the results, returned rows etc?
It's been a while that these functions are there and haven't produced
code which is a lot different for each of these cases. Here's a patch
to extract that code into a separate function and use it in all the
three hook implementations.
[1]: /messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Attachments:
Refactor-postgres_fdw-s-DML-execution-hook-functions.patchtext/x-patch; charset=US-ASCII; name=Refactor-postgres_fdw-s-DML-execution-hook-functions.patchDownload
From 2b7156230423a9f58f2f8ee27ab24aed1156605d Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Date: Wed, 18 Apr 2018 15:52:32 +0530
Subject: [PATCH 2/2] Refactor postgres_fdw's DML execution hook functions
postgresExecForeignInsert(), postgresExecForeignUpdate() and
postgresExecForeignDelete() functions are almost identical except that
postgresExecForeignInsert() doesn't require ctid. Extract that code
into a separate function and use it in all the three hook
implementations.
Ashutosh Bapat.
---
contrib/postgres_fdw/postgres_fdw.c | 189 ++++++++++-------------------------
1 file changed, 54 insertions(+), 135 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 30e5726..acf72e6 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -25,6 +25,7 @@
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
+#include "nodes/parsenodes.h"
#include "optimizer/cost.h"
#include "optimizer/clauses.h"
#include "optimizer/pathnode.h"
@@ -305,6 +306,10 @@ static void postgresBeginForeignModify(ModifyTableState *mtstate,
List *fdw_private,
int subplan_index,
int eflags);
+static TupleTableSlot *perform_one_foreign_dml(EState *estate,
+ ResultRelInfo *resultRelInfo,
+ TupleTableSlot *slot, TupleTableSlot *planslot,
+ CmdType commandtype);
static TupleTableSlot *postgresExecForeignInsert(EState *estate,
ResultRelInfo *resultRelInfo,
TupleTableSlot *slot,
@@ -1732,26 +1737,50 @@ postgresBeginForeignModify(ModifyTableState *mtstate,
}
/*
- * postgresExecForeignInsert
- * Insert one row into a foreign table
+ * perform_one_foreign_dml
+ * Execute a prepared DML statement once on the foreign server using
+ * parameters from the given plan slot. The execution is expected to
+ * affect only one row on the foreign server.
*/
static TupleTableSlot *
-postgresExecForeignInsert(EState *estate,
- ResultRelInfo *resultRelInfo,
- TupleTableSlot *slot,
- TupleTableSlot *planSlot)
+perform_one_foreign_dml(EState *estate, ResultRelInfo *resultRelInfo,
+ TupleTableSlot *slot, TupleTableSlot *planSlot,
+ CmdType commandtype)
{
PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState;
+ ItemPointer ctid;
const char **p_values;
PGresult *res;
int n_rows;
+ Assert(commandtype == CMD_INSERT ||
+ commandtype == CMD_UPDATE ||
+ commandtype == CMD_DELETE);
+
/* Set up the prepared statement on the remote server, if we didn't yet */
if (!fmstate->p_name)
prepare_foreign_modify(fmstate);
+ if (commandtype == CMD_UPDATE || commandtype == CMD_DELETE)
+ {
+ Datum datum;
+ bool isNull;
+
+ /* Get the ctid that was passed up as a resjunk column */
+ datum = ExecGetJunkAttribute(planSlot,
+ fmstate->ctidAttno,
+ &isNull);
+ /* shouldn't ever get a null result... */
+ if (isNull)
+ elog(ERROR, "ctid is NULL");
+
+ ctid = (ItemPointer) DatumGetPointer(datum);
+ }
+ else
+ ctid = NULL;
+
/* Convert parameters needed by prepared statement to text form */
- p_values = convert_prep_stmt_params(fmstate, NULL, slot);
+ p_values = convert_prep_stmt_params(fmstate, ctid, slot);
/*
* Execute the prepared statement.
@@ -1796,6 +1825,20 @@ postgresExecForeignInsert(EState *estate,
}
/*
+ * postgresExecForeignInsert
+ * Insert one row into a foreign table
+ */
+static TupleTableSlot *
+postgresExecForeignInsert(EState *estate,
+ ResultRelInfo *resultRelInfo,
+ TupleTableSlot *slot,
+ TupleTableSlot *planSlot)
+{
+ return perform_one_foreign_dml(estate, resultRelInfo, slot, planSlot,
+ CMD_INSERT);
+}
+
+/*
* postgresExecForeignUpdate
* Update one row in a foreign table
*/
@@ -1805,70 +1848,8 @@ postgresExecForeignUpdate(EState *estate,
TupleTableSlot *slot,
TupleTableSlot *planSlot)
{
- PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState;
- Datum datum;
- bool isNull;
- const char **p_values;
- PGresult *res;
- int n_rows;
-
- /* Set up the prepared statement on the remote server, if we didn't yet */
- if (!fmstate->p_name)
- prepare_foreign_modify(fmstate);
-
- /* Get the ctid that was passed up as a resjunk column */
- datum = ExecGetJunkAttribute(planSlot,
- fmstate->ctidAttno,
- &isNull);
- /* shouldn't ever get a null result... */
- if (isNull)
- elog(ERROR, "ctid is NULL");
-
- /* Convert parameters needed by prepared statement to text form */
- p_values = convert_prep_stmt_params(fmstate,
- (ItemPointer) DatumGetPointer(datum),
- slot);
-
- /*
- * Execute the prepared statement.
- */
- if (!PQsendQueryPrepared(fmstate->conn,
- fmstate->p_name,
- fmstate->p_nums,
- p_values,
- NULL,
- NULL,
- 0))
- pgfdw_report_error(ERROR, NULL, fmstate->conn, false, fmstate->query);
-
- /*
- * Get the result, and check for success.
- *
- * We don't use a PG_TRY block here, so be careful not to throw error
- * without releasing the PGresult.
- */
- res = pgfdw_get_result(fmstate->conn, fmstate->query);
- if (PQresultStatus(res) !=
- (fmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
- pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
-
- /* Check number of rows affected, and fetch RETURNING tuple if any */
- if (fmstate->has_returning)
- {
- n_rows = PQntuples(res);
- if (n_rows > 0)
- store_returning_result(fmstate, slot, res);
- }
- else
- n_rows = atoi(PQcmdTuples(res));
-
- /* And clean up */
- PQclear(res);
-
- MemoryContextReset(fmstate->temp_cxt);
-
- /* Return NULL if nothing was updated on the remote end */
- return (n_rows > 0) ? slot : NULL;
+ return perform_one_foreign_dml(estate, resultRelInfo, slot, planSlot,
+ CMD_UPDATE);
}
/*
@@ -1881,70 +1862,8 @@ postgresExecForeignDelete(EState *estate,
TupleTableSlot *slot,
TupleTableSlot *planSlot)
{
- PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState;
- Datum datum;
- bool isNull;
- const char **p_values;
- PGresult *res;
- int n_rows;
-
- /* Set up the prepared statement on the remote server, if we didn't yet */
- if (!fmstate->p_name)
- prepare_foreign_modify(fmstate);
-
- /* Get the ctid that was passed up as a resjunk column */
- datum = ExecGetJunkAttribute(planSlot,
- fmstate->ctidAttno,
- &isNull);
- /* shouldn't ever get a null result... */
- if (isNull)
- elog(ERROR, "ctid is NULL");
-
- /* Convert parameters needed by prepared statement to text form */
- p_values = convert_prep_stmt_params(fmstate,
- (ItemPointer) DatumGetPointer(datum),
- NULL);
-
- /*
- * Execute the prepared statement.
- */
- if (!PQsendQueryPrepared(fmstate->conn,
- fmstate->p_name,
- fmstate->p_nums,
- p_values,
- NULL,
- NULL,
- 0))
- pgfdw_report_error(ERROR, NULL, fmstate->conn, false, fmstate->query);
-
- /*
- * Get the result, and check for success.
- *
- * We don't use a PG_TRY block here, so be careful not to throw error
- * without releasing the PGresult.
- */
- res = pgfdw_get_result(fmstate->conn, fmstate->query);
- if (PQresultStatus(res) !=
- (fmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
- pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
-
- /* Check number of rows affected, and fetch RETURNING tuple if any */
- if (fmstate->has_returning)
- {
- n_rows = PQntuples(res);
- if (n_rows > 0)
- store_returning_result(fmstate, slot, res);
- }
- else
- n_rows = atoi(PQcmdTuples(res));
-
- /* And clean up */
- PQclear(res);
-
- MemoryContextReset(fmstate->temp_cxt);
-
- /* Return NULL if nothing was deleted on the remote end */
- return (n_rows > 0) ? slot : NULL;
+ return perform_one_foreign_dml(estate, resultRelInfo, slot, planSlot,
+ CMD_DELETE);
}
/*
--
1.7.9.5
(2018/04/18 19:34), Ashutosh Bapat wrote:
Hi,
While working on a fix related to non-direct DML [1], I noticed that
postgresExecForeignInsert(), postgresExecForeignUpdate() and
postgresExecForeignDelete() functions are almost identical except that
postgresExecForeignInsert() doesn't require ctid. The fix that I was
working is applicable to Delete and Update but can be useful for
Insert as well. I had to add the same code to two places at least and
might have missed fixing one of them. Why don't we have a single
function which prepares the statement, extract parameters, executes
the prepared statement and checks for the results, returned rows etc?
It's been a while that these functions are there and haven't produced
code which is a lot different for each of these cases. Here's a patch
to extract that code into a separate function and use it in all the
three hook implementations.[1] /messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com
+1 for the general idea. (Actually, I also thought the same thing
before.) But since this is definitely a matter of PG12, ISTM that it's
wise to work on this after addressing the issue in [1]. My concern is:
if we do this refactoring now, we might need two patches for fixing the
issue in case of backpatching as the fix might need to change those
executor functions.
Best regards,
Etsuro Fujita
On Thu, Jul 19, 2018 at 2:05 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
(2018/04/18 19:34), Ashutosh Bapat wrote:
Hi,
While working on a fix related to non-direct DML [1], I noticed that
postgresExecForeignInsert(), postgresExecForeignUpdate() and
postgresExecForeignDelete() functions are almost identical except that
postgresExecForeignInsert() doesn't require ctid. The fix that I was
working is applicable to Delete and Update but can be useful for
Insert as well. I had to add the same code to two places at least and
might have missed fixing one of them. Why don't we have a single
function which prepares the statement, extract parameters, executes
the prepared statement and checks for the results, returned rows etc?
It's been a while that these functions are there and haven't produced
code which is a lot different for each of these cases. Here's a patch
to extract that code into a separate function and use it in all the
three hook implementations.[1]
/messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com+1 for the general idea. (Actually, I also thought the same thing before.)
But since this is definitely a matter of PG12, ISTM that it's wise to work
on this after addressing the issue in [1]. My concern is: if we do this
refactoring now, we might need two patches for fixing the issue in case of
backpatching as the fix might need to change those executor functions.
The only thing in [1]/messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com that would conflict with this patch is the 0002
(and possibly 0001) patch in [2]/messages/by-id/CAFjFpRfK69ptCTNChBBk+LYMXFzJ92SW6NmG4HLn_1y7xFk=kw@mail.gmail.com. We haven't yet decided anything
about whether those patches can be back-patched or not. I think it's
going to take much longer time for the actual solution to arise. But
we don't have to wait for it to commit this patch as well as 0001 and
0002 patches in [2]/messages/by-id/CAFjFpRfK69ptCTNChBBk+LYMXFzJ92SW6NmG4HLn_1y7xFk=kw@mail.gmail.com because a. the larger solution is not likely to be
back-patchable b. it's going to take much longer time. We don't have
any estimate about how much longer it's going to take. We don't want
this small piece to get stuck for that larger piece worst get stuck
indefinitely. So, I will vote for getting it committed ASAP, if we
agree that it's good change. If the solution to [1]/messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com happens to require
this refactoring we we can back-patch that since back-patching it
won't hurt.
[1]: /messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com
[2]: /messages/by-id/CAFjFpRfK69ptCTNChBBk+LYMXFzJ92SW6NmG4HLn_1y7xFk=kw@mail.gmail.com
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
(2018/07/19 17:52), Ashutosh Bapat wrote:
On Thu, Jul 19, 2018 at 2:05 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:+1 for the general idea. (Actually, I also thought the same thing before.)
But since this is definitely a matter of PG12, ISTM that it's wise to work
on this after addressing the issue in [1]. My concern is: if we do this
refactoring now, we might need two patches for fixing the issue in case of
backpatching as the fix might need to change those executor functions.The only thing in [1] that would conflict with this patch is the 0002
(and possibly 0001) patch in [2]. We haven't yet decided anything
about whether those patches can be back-patched or not. I think it's
going to take much longer time for the actual solution to arise. But
we don't have to wait for it to commit this patch as well as 0001 and
0002 patches in [2]
I've just started catching up the discussions in [1], so I don't think I
understand those fully, but it appears that we haven't yet reached a
consensus on what to do for that issue.
because a. the larger solution is not likely to be
back-patchable b. it's going to take much longer time. We don't have
any estimate about how much longer it's going to take.
I don't understand the solution yet, so I'll study about that.
Best regards,
Etsuro Fujita
On Thu, Jul 19, 2018 at 05:35:11PM +0900, Etsuro Fujita wrote:
+1 for the general idea. (Actually, I also thought the same thing before.)
But since this is definitely a matter of PG12, ISTM that it's wise to work
on this after addressing the issue in [1]. My concern is: if we do this
refactoring now, we might need two patches for fixing the issue in case of
backpatching as the fix might need to change those executor functions.
FWIW, I would think that if some cleanup of the code is obvious, we
should make it without waiting for the other issues to settle down
because there is no way to know when those are done, and this patch
could be forgotten. This indeed makes back-patching a bit harder but it
also reduces the code chunk for HEAD with the extra fixes.
Looking at the proposed patch, moving the new routine closer to
execute_dml_stmt and renaming it execute_dml_single_row would be nicer.
--
Michael
(2018/07/20 13:49), Michael Paquier wrote:
On Thu, Jul 19, 2018 at 05:35:11PM +0900, Etsuro Fujita wrote:
+1 for the general idea. (Actually, I also thought the same thing before.)
But since this is definitely a matter of PG12, ISTM that it's wise to work
on this after addressing the issue in [1]. My concern is: if we do this
refactoring now, we might need two patches for fixing the issue in case of
backpatching as the fix might need to change those executor functions.FWIW, I would think that if some cleanup of the code is obvious, we
should make it without waiting for the other issues to settle down
because there is no way to know when those are done,
I would agree to that if we were late in the development cycle for PG12.
and this patch
could be forgotten.
I won't forget this patch. :)
Looking at the proposed patch, moving the new routine closer to
execute_dml_stmt and renaming it execute_dml_single_row would be nicer.
Or execute_parameterized_dml_stmt?
Best regards,
Etsuro Fujita
On Fri, Jul 20, 2018 at 2:27 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
(2018/07/20 13:49), Michael Paquier wrote:
On Thu, Jul 19, 2018 at 05:35:11PM +0900, Etsuro Fujita wrote:
+1 for the general idea. (Actually, I also thought the same thing
before.)
But since this is definitely a matter of PG12, ISTM that it's wise to
work
on this after addressing the issue in [1]. My concern is: if we do this
refactoring now, we might need two patches for fixing the issue in case
of
backpatching as the fix might need to change those executor functions.FWIW, I would think that if some cleanup of the code is obvious, we
should make it without waiting for the other issues to settle down
because there is no way to know when those are done,I would agree to that if we were late in the development cycle for PG12.
and this patch
could be forgotten.I won't forget this patch. :)
Looking at the proposed patch, moving the new routine closer to
execute_dml_stmt and renaming it execute_dml_single_row would be nicer.Or execute_parameterized_dml_stmt?
I have placed it closer to its caller, I think that's better than
moving it closer to execute_dml_stmt, unless there is any other strong
advantage.
I used perform instead of execute since the later is usually
associated with local operation. I added "foreign" in the name of the
function to indicate that it's executed on foreign server. I am happy
with "remote" as well. I don't think "one" and "single" make any
difference. I don't like "parameterized" since that gets too tied to
the method we are using rather than what's actually being done. In
short I don't find any of the suggestions to be significantly better
or worse than the name I have chosen. Said that, I am not wedded to
any of those. A committer is free to choose anything s/he likes.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On Mon, Jul 23, 2018 at 02:17:38PM +0530, Ashutosh Bapat wrote:
I used perform instead of execute since the later is usually
associated with local operation. I added "foreign" in the name of the
function to indicate that it's executed on foreign server. I am happy
with "remote" as well. I don't think "one" and "single" make any
difference. I don't like "parameterized" since that gets too tied to
the method we are using rather than what's actually being done. In
short I don't find any of the suggestions to be significantly better
or worse than the name I have chosen. Said that, I am not wedded to
any of those. A committer is free to choose anything s/he likes.
Fujita-san, you are registered as a reviewer of this patch. Are you
planning to look at it soon? It looks useful to me to rework this code
anyway to help with future maintenance, so in the worst case I could
always look at it. For now I have moved it to the next commit fest.
--
Michael
(2018/10/01 19:42), Michael Paquier wrote:
On Mon, Jul 23, 2018 at 02:17:38PM +0530, Ashutosh Bapat wrote:
I used perform instead of execute since the later is usually
associated with local operation. I added "foreign" in the name of the
function to indicate that it's executed on foreign server. I am happy
with "remote" as well. I don't think "one" and "single" make any
difference. I don't like "parameterized" since that gets too tied to
the method we are using rather than what's actually being done. In
short I don't find any of the suggestions to be significantly better
or worse than the name I have chosen. Said that, I am not wedded to
any of those. A committer is free to choose anything s/he likes.Fujita-san, you are registered as a reviewer of this patch. Are you
planning to look at it soon?
Yeah, I'm planning to work on this immediately after fixing the issue
[1]: , because it still seems to me wise to work on it after addressing that issue. (I'll post an updated version of the patch for that tomorrow.)
that issue. (I'll post an updated version of the patch for that tomorrow.)
It looks useful to me to rework this code
anyway to help with future maintenance,
Agreed.
so in the worst case I could
always look at it. For now I have moved it to the next commit fest.
Thanks for taking care of this!
Best regards,
Etsuro Fujita
(2018/10/01 21:54), Etsuro Fujita wrote:
(2018/10/01 19:42), Michael Paquier wrote:
On Mon, Jul 23, 2018 at 02:17:38PM +0530, Ashutosh Bapat wrote:
I used perform instead of execute since the later is usually
associated with local operation. I added "foreign" in the name of the
function to indicate that it's executed on foreign server. I am happy
with "remote" as well. I don't think "one" and "single" make any
difference. I don't like "parameterized" since that gets too tied to
the method we are using rather than what's actually being done. In
short I don't find any of the suggestions to be significantly better
or worse than the name I have chosen. Said that, I am not wedded to
any of those. A committer is free to choose anything s/he likes.Fujita-san, you are registered as a reviewer of this patch. Are you
planning to look at it soon?Yeah, I'm planning to work on this immediately after fixing the issue
[1], because it still seems to me wise to work on it after addressing
that issue. (I'll post an updated version of the patch for that tomorrow.)
Sorry, I forgot to add the pointer for [1]:
/messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com
Best regards,
Etsuro Fujita
On Tue, Oct 02, 2018 at 01:50:46PM +0900, Etsuro Fujita wrote:
Sorry, I forgot to add the pointer for [1]:
/messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com
OK, thanks for your update!
--
Michael
On Mon, Oct 1, 2018 at 2:55 PM Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
(2018/10/01 19:42), Michael Paquier wrote:
On Mon, Jul 23, 2018 at 02:17:38PM +0530, Ashutosh Bapat wrote:
Fujita-san, you are registered as a reviewer of this patch. Are you
planning to look at it soon?Yeah, I'm planning to work on this immediately after fixing the issue
[1], because it still seems to me wise to work on it after addressing
that issue. (I'll post an updated version of the patch for that tomorrow.)
Yep, it would be nice to have a full review here. But as far as I see the issue
you were talking about is still in progress, right? Also looks like someone
needs to take over this rather small patch.
(2018/11/30 2:58), Dmitry Dolgov wrote:
On Mon, Oct 1, 2018 at 2:55 PM Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp> wrote:
(2018/10/01 19:42), Michael Paquier wrote:
On Mon, Jul 23, 2018 at 02:17:38PM +0530, Ashutosh Bapat wrote:
Fujita-san, you are registered as a reviewer of this patch. Are you
planning to look at it soon?Yeah, I'm planning to work on this immediately after fixing the issue
[1], because it still seems to me wise to work on it after addressing
that issue. (I'll post an updated version of the patch for that tomorrow.)Yep, it would be nice to have a full review here. But as far as I see the issue
you were talking about is still in progress, right?
That's right.
Also looks like someone
needs to take over this rather small patch.
I changed my mind; I thought it would be better to work on that issue
before this, but Tom raised concerns about the patch I proposed for that
issue, and I think it'll take much longer time to address that, so I'd
like to get this done first.
One thing we would need to discuss more about this is the name of a new
function added by the patch. IIRC, we have three options so far [1]/messages/by-id/CAFjFpRc+MdhD2W=ofc06n3NriqPCy6ztNBehXTc_g_y9=P5k1w@mail.gmail.com:
1) perform_one_foreign_dml proposed by Ashutosh
2) execute_dml_single_row proposed by Michael
3) execute_parameterized_dml_stmt proposed by me
I'll withdraw my proposal; I think #1 and #2 would describe the
functionality better than #3. My vote would go to #2 or
execute_dml_stmt_single_row, which moves the function much more closer
to execute_dml_stmt. I'd like to hear the opinions of others.
Thanks!
Best regards,
Etsuro Fujita
[1]: /messages/by-id/CAFjFpRc+MdhD2W=ofc06n3NriqPCy6ztNBehXTc_g_y9=P5k1w@mail.gmail.com
/messages/by-id/CAFjFpRc+MdhD2W=ofc06n3NriqPCy6ztNBehXTc_g_y9=P5k1w@mail.gmail.com
(2018/11/30 19:55), Etsuro Fujita wrote:
One thing we would need to discuss more about this is the name of a new
function added by the patch. IIRC, we have three options so far [1]:1) perform_one_foreign_dml proposed by Ashutosh
2) execute_dml_single_row proposed by Michael
3) execute_parameterized_dml_stmt proposed by meI'll withdraw my proposal; I think #1 and #2 would describe the
functionality better than #3. My vote would go to #2 or
execute_dml_stmt_single_row, which moves the function much more closer
to execute_dml_stmt. I'd like to hear the opinions of others.
On second thought I'd like to propose another option:
execute_foreign_modify because I think this would match the existing
helper functions for updating foreign tables in postgres_fdw.c better,
such as create_foreign_modify, prepare_foreign_modify and
finish_foreign_modify. I don't really think the function name should
contain "one" or "single_row". Like the names of the calling APIs (ie,
ExecForeignInsert, ExecForeignUpdate and ExecForeignDelete), I think
it's OK to omit such words from the function name. Here is an updated
version of the patch. In addition to the naming, I tweaked the function
a little bit to match other functions (mainly variable names), moved it
to the place where the helper functions are defined, fiddled with some
comments, and removed an extra include file that the original patch added.
Best regards,
Etsuro Fujita
Attachments:
Refactor-postgres_fdw-s-DML-execution-hook-functions-v2.patchtext/x-patch; name=Refactor-postgres_fdw-s-DML-execution-hook-functions-v2.patchDownload
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 390,395 **** static PgFdwModifyState *create_foreign_modify(EState *estate,
--- 390,400 ----
List *target_attrs,
bool has_returning,
List *retrieved_attrs);
+ static TupleTableSlot *execute_foreign_modify(EState *estate,
+ ResultRelInfo *resultRelInfo,
+ CmdType operation,
+ TupleTableSlot *slot,
+ TupleTableSlot *planSlot);
static void prepare_foreign_modify(PgFdwModifyState *fmstate);
static const char **convert_prep_stmt_params(PgFdwModifyState *fmstate,
ItemPointer tupleid,
***************
*** 1775,1832 **** postgresExecForeignInsert(EState *estate,
TupleTableSlot *slot,
TupleTableSlot *planSlot)
{
! PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState;
! const char **p_values;
! PGresult *res;
! int n_rows;
!
! /* Set up the prepared statement on the remote server, if we didn't yet */
! if (!fmstate->p_name)
! prepare_foreign_modify(fmstate);
!
! /* Convert parameters needed by prepared statement to text form */
! p_values = convert_prep_stmt_params(fmstate, NULL, slot);
!
! /*
! * Execute the prepared statement.
! */
! if (!PQsendQueryPrepared(fmstate->conn,
! fmstate->p_name,
! fmstate->p_nums,
! p_values,
! NULL,
! NULL,
! 0))
! pgfdw_report_error(ERROR, NULL, fmstate->conn, false, fmstate->query);
!
! /*
! * Get the result, and check for success.
! *
! * We don't use a PG_TRY block here, so be careful not to throw error
! * without releasing the PGresult.
! */
! res = pgfdw_get_result(fmstate->conn, fmstate->query);
! if (PQresultStatus(res) !=
! (fmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
! pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
!
! /* Check number of rows affected, and fetch RETURNING tuple if any */
! if (fmstate->has_returning)
! {
! n_rows = PQntuples(res);
! if (n_rows > 0)
! store_returning_result(fmstate, slot, res);
! }
! else
! n_rows = atoi(PQcmdTuples(res));
!
! /* And clean up */
! PQclear(res);
!
! MemoryContextReset(fmstate->temp_cxt);
!
! /* Return NULL if nothing was inserted on the remote end */
! return (n_rows > 0) ? slot : NULL;
}
/*
--- 1780,1787 ----
TupleTableSlot *slot,
TupleTableSlot *planSlot)
{
! return execute_foreign_modify(estate, resultRelInfo, CMD_INSERT,
! slot, planSlot);
}
/*
***************
*** 1839,1908 **** postgresExecForeignUpdate(EState *estate,
TupleTableSlot *slot,
TupleTableSlot *planSlot)
{
! PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState;
! Datum datum;
! bool isNull;
! const char **p_values;
! PGresult *res;
! int n_rows;
!
! /* Set up the prepared statement on the remote server, if we didn't yet */
! if (!fmstate->p_name)
! prepare_foreign_modify(fmstate);
!
! /* Get the ctid that was passed up as a resjunk column */
! datum = ExecGetJunkAttribute(planSlot,
! fmstate->ctidAttno,
! &isNull);
! /* shouldn't ever get a null result... */
! if (isNull)
! elog(ERROR, "ctid is NULL");
!
! /* Convert parameters needed by prepared statement to text form */
! p_values = convert_prep_stmt_params(fmstate,
! (ItemPointer) DatumGetPointer(datum),
! slot);
!
! /*
! * Execute the prepared statement.
! */
! if (!PQsendQueryPrepared(fmstate->conn,
! fmstate->p_name,
! fmstate->p_nums,
! p_values,
! NULL,
! NULL,
! 0))
! pgfdw_report_error(ERROR, NULL, fmstate->conn, false, fmstate->query);
!
! /*
! * Get the result, and check for success.
! *
! * We don't use a PG_TRY block here, so be careful not to throw error
! * without releasing the PGresult.
! */
! res = pgfdw_get_result(fmstate->conn, fmstate->query);
! if (PQresultStatus(res) !=
! (fmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
! pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
!
! /* Check number of rows affected, and fetch RETURNING tuple if any */
! if (fmstate->has_returning)
! {
! n_rows = PQntuples(res);
! if (n_rows > 0)
! store_returning_result(fmstate, slot, res);
! }
! else
! n_rows = atoi(PQcmdTuples(res));
!
! /* And clean up */
! PQclear(res);
!
! MemoryContextReset(fmstate->temp_cxt);
!
! /* Return NULL if nothing was updated on the remote end */
! return (n_rows > 0) ? slot : NULL;
}
/*
--- 1794,1801 ----
TupleTableSlot *slot,
TupleTableSlot *planSlot)
{
! return execute_foreign_modify(estate, resultRelInfo, CMD_UPDATE,
! slot, planSlot);
}
/*
***************
*** 1915,1984 **** postgresExecForeignDelete(EState *estate,
TupleTableSlot *slot,
TupleTableSlot *planSlot)
{
! PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState;
! Datum datum;
! bool isNull;
! const char **p_values;
! PGresult *res;
! int n_rows;
!
! /* Set up the prepared statement on the remote server, if we didn't yet */
! if (!fmstate->p_name)
! prepare_foreign_modify(fmstate);
!
! /* Get the ctid that was passed up as a resjunk column */
! datum = ExecGetJunkAttribute(planSlot,
! fmstate->ctidAttno,
! &isNull);
! /* shouldn't ever get a null result... */
! if (isNull)
! elog(ERROR, "ctid is NULL");
!
! /* Convert parameters needed by prepared statement to text form */
! p_values = convert_prep_stmt_params(fmstate,
! (ItemPointer) DatumGetPointer(datum),
! NULL);
!
! /*
! * Execute the prepared statement.
! */
! if (!PQsendQueryPrepared(fmstate->conn,
! fmstate->p_name,
! fmstate->p_nums,
! p_values,
! NULL,
! NULL,
! 0))
! pgfdw_report_error(ERROR, NULL, fmstate->conn, false, fmstate->query);
!
! /*
! * Get the result, and check for success.
! *
! * We don't use a PG_TRY block here, so be careful not to throw error
! * without releasing the PGresult.
! */
! res = pgfdw_get_result(fmstate->conn, fmstate->query);
! if (PQresultStatus(res) !=
! (fmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
! pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
!
! /* Check number of rows affected, and fetch RETURNING tuple if any */
! if (fmstate->has_returning)
! {
! n_rows = PQntuples(res);
! if (n_rows > 0)
! store_returning_result(fmstate, slot, res);
! }
! else
! n_rows = atoi(PQcmdTuples(res));
!
! /* And clean up */
! PQclear(res);
!
! MemoryContextReset(fmstate->temp_cxt);
!
! /* Return NULL if nothing was deleted on the remote end */
! return (n_rows > 0) ? slot : NULL;
}
/*
--- 1808,1815 ----
TupleTableSlot *slot,
TupleTableSlot *planSlot)
{
! return execute_foreign_modify(estate, resultRelInfo, CMD_DELETE,
! slot, planSlot);
}
/*
***************
*** 3425,3430 **** create_foreign_modify(EState *estate,
--- 3256,3353 ----
}
/*
+ * execute_foreign_modify
+ * Perform foreign-table modification as required, and fetch RETURNING
+ * result if any. (This is the shared guts of postgresExecForeignInsert,
+ * postgresExecForeignUpdate, and postgresExecForeignDelete.)
+ */
+ static TupleTableSlot *
+ execute_foreign_modify(EState *estate,
+ ResultRelInfo *resultRelInfo,
+ CmdType operation,
+ TupleTableSlot *slot,
+ TupleTableSlot *planSlot)
+ {
+ PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState;
+ ItemPointer ctid = NULL;
+ const char **p_values;
+ PGresult *res;
+ int n_rows;
+
+ /* The operation should be INSERT, UPDATE, or DELETE */
+ Assert(operation == CMD_INSERT ||
+ operation == CMD_UPDATE ||
+ operation == CMD_DELETE);
+
+ /* Set up the prepared statement on the remote server, if we didn't yet */
+ if (!fmstate->p_name)
+ prepare_foreign_modify(fmstate);
+
+ /*
+ * For UPDATE/DELETE, get the ctid that was passed up as a resjunk column
+ */
+ if (operation == CMD_UPDATE || operation == CMD_DELETE)
+ {
+ Datum datum;
+ bool isNull;
+
+ datum = ExecGetJunkAttribute(planSlot,
+ fmstate->ctidAttno,
+ &isNull);
+ /* shouldn't ever get a null result... */
+ if (isNull)
+ elog(ERROR, "ctid is NULL");
+ ctid = (ItemPointer) DatumGetPointer(datum);
+ }
+
+ /* Convert parameters needed by prepared statement to text form */
+ p_values = convert_prep_stmt_params(fmstate, ctid, slot);
+
+ /*
+ * Execute the prepared statement.
+ */
+ if (!PQsendQueryPrepared(fmstate->conn,
+ fmstate->p_name,
+ fmstate->p_nums,
+ p_values,
+ NULL,
+ NULL,
+ 0))
+ pgfdw_report_error(ERROR, NULL, fmstate->conn, false, fmstate->query);
+
+ /*
+ * Get the result, and check for success.
+ *
+ * We don't use a PG_TRY block here, so be careful not to throw error
+ * without releasing the PGresult.
+ */
+ res = pgfdw_get_result(fmstate->conn, fmstate->query);
+ if (PQresultStatus(res) !=
+ (fmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
+ pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
+
+ /* Check number of rows affected, and fetch RETURNING tuple if any */
+ if (fmstate->has_returning)
+ {
+ n_rows = PQntuples(res);
+ if (n_rows > 0)
+ store_returning_result(fmstate, slot, res);
+ }
+ else
+ n_rows = atoi(PQcmdTuples(res));
+
+ /* And clean up */
+ PQclear(res);
+
+ MemoryContextReset(fmstate->temp_cxt);
+
+ /*
+ * Return NULL if nothing was inserted/updated/deleted on the remote end
+ */
+ return (n_rows > 0) ? slot : NULL;
+ }
+
+ /*
* prepare_foreign_modify
* Establish a prepared statement for execution of INSERT/UPDATE/DELETE
*/
(2019/01/07 20:26), Etsuro Fujita wrote:
(2018/11/30 19:55), Etsuro Fujita wrote:
One thing we would need to discuss more about this is the name of a new
function added by the patch. IIRC, we have three options so far [1]:1) perform_one_foreign_dml proposed by Ashutosh
2) execute_dml_single_row proposed by Michael
3) execute_parameterized_dml_stmt proposed by meI'll withdraw my proposal; I think #1 and #2 would describe the
functionality better than #3. My vote would go to #2 or
execute_dml_stmt_single_row, which moves the function much more closer
to execute_dml_stmt. I'd like to hear the opinions of others.On second thought I'd like to propose another option:
execute_foreign_modify because I think this would match the existing
helper functions for updating foreign tables in postgres_fdw.c better,
such as create_foreign_modify, prepare_foreign_modify and
finish_foreign_modify. I don't really think the function name should
contain "one" or "single_row". Like the names of the calling APIs (ie,
ExecForeignInsert, ExecForeignUpdate and ExecForeignDelete), I think
it's OK to omit such words from the function name. Here is an updated
version of the patch. In addition to the naming, I tweaked the function
a little bit to match other functions (mainly variable names), moved it
to the place where the helper functions are defined, fiddled with some
comments, and removed an extra include file that the original patch added.
If there are no objections, I'll commit that version of the patch.
Best regards,
Etsuro Fujita
On Wed, Jan 16, 2019 at 02:59:15PM +0900, Etsuro Fujita wrote:
(2019/01/07 20:26), Etsuro Fujita wrote:
On second thought I'd like to propose another option:
execute_foreign_modify because I think this would match the existing
helper functions for updating foreign tables in postgres_fdw.c better,
such as create_foreign_modify, prepare_foreign_modify and
finish_foreign_modify. I don't really think the function name should
contain "one" or "single_row". Like the names of the calling APIs (ie,
ExecForeignInsert, ExecForeignUpdate and ExecForeignDelete), I think
it's OK to omit such words from the function name. Here is an updated
version of the patch. In addition to the naming, I tweaked the function
a little bit to match other functions (mainly variable names), moved it
to the place where the helper functions are defined, fiddled with some
comments, and removed an extra include file that the original patch added.If there are no objections, I'll commit that version of the patch.
I think that you could use PgFdwModifyState for the second argument of
execute_foreign_modify instead of ResultRelInfo. Except of that nit,
it looks fine to me, thanks for taking care of it.
--
Michael
Michael-san,
(2019/01/16 15:54), Michael Paquier wrote:
On Wed, Jan 16, 2019 at 02:59:15PM +0900, Etsuro Fujita wrote:
(2019/01/07 20:26), Etsuro Fujita wrote:
On second thought I'd like to propose another option:
execute_foreign_modify because I think this would match the existing
helper functions for updating foreign tables in postgres_fdw.c better,
such as create_foreign_modify, prepare_foreign_modify and
finish_foreign_modify. I don't really think the function name should
contain "one" or "single_row". Like the names of the calling APIs (ie,
ExecForeignInsert, ExecForeignUpdate and ExecForeignDelete), I think
it's OK to omit such words from the function name. Here is an updated
version of the patch. In addition to the naming, I tweaked the function
a little bit to match other functions (mainly variable names), moved it
to the place where the helper functions are defined, fiddled with some
comments, and removed an extra include file that the original patch added.If there are no objections, I'll commit that version of the patch.
I think that you could use PgFdwModifyState for the second argument of
execute_foreign_modify instead of ResultRelInfo.
Yeah, that is another option, but my favorite would be to use
ResultRelInfo, as in the original patch by Ashutosh, because that makes
it possible to write postgresExecForeignInsert,
postgresExecForeignUpdate, and postgresExecForeignDelete as a single line.
Except of that nit,
it looks fine to me, thanks for taking care of it.
Great! Thanks for the review!
Best regards,
Etsuro Fujita
(2019/01/16 20:30), Etsuro Fujita wrote:
(2019/01/16 15:54), Michael Paquier wrote:
On Wed, Jan 16, 2019 at 02:59:15PM +0900, Etsuro Fujita wrote:
If there are no objections, I'll commit that version of the patch.
I think that you could use PgFdwModifyState for the second argument of
execute_foreign_modify instead of ResultRelInfo.Yeah, that is another option, but my favorite would be to use
ResultRelInfo, as in the original patch by Ashutosh, because that makes
it possible to write postgresExecForeignInsert,
postgresExecForeignUpdate, and postgresExecForeignDelete as a single line.Except of that nit,
it looks fine to me, thanks for taking care of it.Great! Thanks for the review!
Pushed. I left that argument alone. I think we can change it later, if
necessary :).
Best regards,
Etsuro Fujita