pgsql: Further fix for EvalPlanQual with mix of local and foreign parti

Started by Etsuro Fujitaover 4 years ago4 messagescomitters
Jump to latest
#1Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp

Further fix for EvalPlanQual with mix of local and foreign partitions.

We assume that direct-modify ForeignScan nodes cannot be re-evaluated
during EvalPlanQual processing, but the rework for inherited
UPDATE/DELETE in commit 86dc90056 changed things, without considering
that, so that such ForeignScan nodes get called as part of the
EvalPlanQual subtree during EvalPlanQual processing in the case of an
inherited UPDATE/DELETE where the inheritance set contains foreign
target relations. To avoid re-evaluating such ForeignScan nodes during
EvalPlanQual processing, commit c3928b467 modified nodeForeignscan.c,
but the assumption made there that ExecForeignScan() should never be
called for such ForeignScan nodes during EvalPlanQual processing turned
out to be wrong in some cases, leading to a segmentation fault or a
"cannot re-evaluate a Foreign Update or Delete during EvalPlanQual"
error.

Fix by modifying nodeForeignscan.c further to avoid re-evaluating such
ForeignScan nodes even in ExecForeignScan()/ExecReScanForeignScan()
during EvalPlanQual processing. Since this makes non-reachable the
test-and-elog added to ForeignNext() by commit c3928b467 that produced
the aforesaid error, convert the test-and-elog to an Assert.

Per bug #17355 from Alexander Lakhin. Back-patch to v14 where both
commits came in.

Patch by me, reviewed and tested by Alexander Lakhin and Amit Langote.

Discussion: /messages/by-id/17355-de8e362eb7001a96@postgresql.org

Branch
------
REL_14_STABLE

Details
-------
https://git.postgresql.org/pg/commitdiff/7b0cec2fa05a123ed96163d7ac8485384570b3e0

Modified Files
--------------
src/backend/executor/nodeForeignscan.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)

#2Andres Freund
andres@anarazel.de
In reply to: Etsuro Fujita (#1)
Re: pgsql: Further fix for EvalPlanQual with mix of local and foreign parti

Hi,

On 2022-02-03 06:22:58 +0000, Etsuro Fujita wrote:

Further fix for EvalPlanQual with mix of local and foreign partitions.

My compiler (as well as cfbot) now complains that there is an unused variable,
when building without assertions:

/home/andres/src/postgresql/src/backend/executor/nodeForeignscan.c: In function ‘ForeignNext’:
/home/andres/src/postgresql/src/backend/executor/nodeForeignscan.c:47:21: warning: unused variable ‘estate’ [-Wunused-variable]
47 | EState *estate = node->ss.ps.state;
| ^~~~~~

Seems best to just use node->ss.ps.state instead of the local estate variable?
Rather than using the noisier PG_USED_FOR_ASSERTS_ONLY.

I'll make it so, unless you want to?

Greetings,

Andres Freund

#3Andres Freund
andres@anarazel.de
In reply to: Andres Freund (#2)
Re: pgsql: Further fix for EvalPlanQual with mix of local and foreign parti

On 2022-02-03 10:36:55 -0800, Andres Freund wrote:

I'll make it so, unless you want to?

Did so now, cfbot being all red was a bit annoying. Wonder if we can failures
due to compiler warnings a bit more distinguished from "harder" failures in
the UI?

#4Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Andres Freund (#3)
Re: pgsql: Further fix for EvalPlanQual with mix of local and foreign parti

On Fri, Feb 4, 2022 at 3:50 AM Andres Freund <andres@anarazel.de> wrote:

On 2022-02-03 10:36:55 -0800, Andres Freund wrote:

I'll make it so, unless you want to?

Did so now, cfbot being all red was a bit annoying.

Thanks for the fix!

Wonder if we can failures
due to compiler warnings a bit more distinguished from "harder" failures in
the UI?

Yeah, it would be nice to have such UI.

Best regards,
Etsuro Fujita