Subject: Re: SEGFAULT on a concurrent UPDATE of mix of local and foreign
 partitions
To: Andrey Lepikhov <a.lepikhov@postgrespro.ru>,
 PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>,
 Amit Langote <amitlangote09@gmail.com>
References: <cb2b808d-cbaa-4772-76ee-c8809bafcf3d@postgrespro.ru>
From: Heikki Linnakangas <hlinnaka@iki.fi>
Message-ID: <1a6c8eab-87e5-2dce-590f-e4aef85d6e27@iki.fi>
Date: Mon, 9 Aug 2021 23:29:47 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <cb2b808d-cbaa-4772-76ee-c8809bafcf3d@postgrespro.ru>
Content-Type: multipart/mixed;
 boundary="------------CDE573369EDE9BBEEE65B90B"
Content-Language: en-US
Archived-At: 
 <https://www.postgresql.org/message-id/1a6c8eab-87e5-2dce-590f-e4aef85d6e27%40iki.fi>
Precedence: bulk

This is a multi-part message in MIME format.
--------------CDE573369EDE9BBEEE65B90B
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

On 06/08/2021 08:34, Andrey Lepikhov wrote:
> Hi,
> Postgres SEGFAULT'ed on the UPDATE of mix of local and foreign partitions.
> Initialization - see t.sql
> For replaying this segfault just execute in parallel:
> UPDATE test SET x = x - 1;
> 
> The problem was introduced by commit 1375422.
> ExecUpdate has found a concurrently updated tuples and starts subplan
> evaluation. This operation creates new EState for EPQState and sets
> es_result_relations in NULL value. Next, ExecInitNode(subplan) is
> launched and underlying ExecInitForeignScan tries to access to an
> element of es_result_relations. This causes SEGFAULT.

Thanks for the report!

> I studied this problem shortly. I think, EPQState can use
> es_result_relations of a parent EState. Patch in attachment fixes this.
> check-world passed clearly.

Hmm, that seems to work, but I have a few questions:

Can we be sure that the es_result_relations entry of the result relation 
has been initialized when the EPQState is constructed? The general model 
now is that you initialize them lazily, when needed.

Does it ever make sense to re-evaluate a direct Foreign Update as part 
of EvalPlanQual? It doesn't seem sane to me, you cannot just run an 
UPDATE statement again and expect it to be idempotent. So I think it 
would make sense skip this during EvalPlanQual processing altogether. 
And no need to call the FDW's Begin/EndDirectModify functions either.

It's not clear to me what guarantee there is that a direct-modify 
Foreign Update/Delete node is never re-evaluated as part of 
EvalPlanQual. Is it because the a ModifyTable node is always at the top 
of the plan tree? Or is it because the FDW never uses direct-modify in 
queries where EvalPlanQual might be needed? Or because we use 
ROW_MARK_COPY in those cases?

In any case, re-evaluating a direct-modify statement doesn't seem sane, 
so I propose the attached patch to add some runtime checks for that and 
to avoid the original segfault.

- Heikki

--------------CDE573369EDE9BBEEE65B90B
Content-Type: text/x-patch; charset=UTF-8;
 name="0001-Fix-segfault-during-EvalPlanQual-with-mix-of-local-a.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="0001-Fix-segfault-during-EvalPlanQual-with-mix-of-local-a.pa";
 filename*1="tch"