From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> |
Cc: | Noah Misch <noah(at)leadboat(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Rushabh Lathia <rushabh(dot)lathia(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-04-19 04:59:03 |
Message-ID: | CAB7nPqRP9MWCL6aCyKsudfmpTBt8q_s0j_Ft995CpGaK-AqC9g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Apr 19, 2016 at 1:14 PM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> The comment "We don't use a PG_TRY block here ..." seems to be wrongly
> placed, so I moved that comment. Also, I think it'd be better to call
> pgfdw_report_error() with the clear argument set to false, not true, since
> we don't need to clear the PGresult. Same for postgresExecForeignUpdate,
> postgresExecForeignUpdate, and prepare_foreign_modify.
No objection to moving those comment blocks where pgfdw_get_result is called.
> What do you think about that?
+ /* Wait for the result */
+ res = pgfdw_get_result(conn, query);
+ if (res == NULL)
+ pgfdw_report_error(ERROR, NULL, conn, false, query);
+ last_res = res;
+
+ /*
+ * Verify that there are no more results
+ *
+ * We don't use a PG_TRY block here, so be careful not to throw error
+ * without releasing the PGresult.
+ */
+ res = pgfdw_get_result(conn, query);
+ if (res != NULL)
+ {
+ PQclear(last_res);
+ pgfdw_report_error(ERROR, res, conn, true, query);
+ }
But huge objection to that because this fragilizes the current logic
postgres_fdw is based on: PQexec returns the last result to caller,
I'd rather not break that logic for 9.6 stability's sake.
A even better proof of that is the following, which just emulates what
your version of pgfdw_get_result is doing when consuming the results.
+ /* Verify that there are no more results */
+ res = pgfdw_get_result(fmstate->conn, fmstate->query);
+ if (res != NULL)
+ pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
This could even lead to incorrect errors in the future if multiple
queries are combined with those DMLs for a reason or another.
--
Michael
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2016-04-19 05:23:39 | Re: snapshot too old, configured by time |
Previous Message | Noah Misch | 2016-04-19 04:16:31 | Re: [COMMITTERS] pgsql: Add trigonometric functions that work in degrees. |