Subject: Re: SEGFAULT on a concurrent UPDATE of mix of local and foreign
 partitions
To: Heikki Linnakangas <hlinnaka@iki.fi>,
 PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>,
 Amit Langote <amitlangote09@gmail.com>
References: <cb2b808d-cbaa-4772-76ee-c8809bafcf3d@postgrespro.ru>
 <1a6c8eab-87e5-2dce-590f-e4aef85d6e27@iki.fi>
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Organization: Postgres Professional
Message-ID: <706b5033-d2cd-92b1-e44e-3349ff3053fb@postgrespro.ru>
Date: Tue, 10 Aug 2021 14:18:52 +0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <1a6c8eab-87e5-2dce-590f-e4aef85d6e27@iki.fi>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/706b5033-d2cd-92b1-e44e-3349ff3053fb%40postgrespro.ru>
Precedence: bulk

On 8/10/21 1:29 AM, Heikki Linnakangas wrote:
> 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.
As I understand the code correctly - yes, because a ModifyTable node 
higher and will initialize this field earlier. But in many cases it is 
not 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.
Agree. It is my first dive into the EPQ code. After studying i think 
this makes no sense.
> 
> 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?
ModifyTable node may be found in an underlying CTE, for example. And, 
right now, it's not clear for me too.
> 
> 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 segfaultYour patch looks much better.

-- 
regards,
Andrey Lepikhov
Postgres Professional