Обсуждение: Back-branch bugs with fully-prunable UPDATEs

This test script works fine in HEAD:

drop table if exists parttbl cascade;
CREATE TABLE parttbl (a int, b int) PARTITION BY LIST (a);
CREATE TABLE parttbl_1 PARTITION OF parttbl FOR VALUES IN (NULL,500,501,502);
UPDATE parttbl SET a = NULL, b = NULL WHERE a = 1600 AND b = 999;

In v11, it suffers an assertion failure in ExecSetupPartitionTupleRouting.

In v10, it doesn't crash, but we do get

WARNING:  relcache reference leak: relation "parttbl" not closed

which is surely a bug as well.

(This is a boiled-down version of the script I mentioned in
https://www.postgresql.org/message-id/13344.1554578481@sss.pgh.pa.us)

This seems to be related to what Amit Langote complained of in
https://www.postgresql.org/message-id/21e7eaa4-0d4d-20c2-a1f7-c7e96f4ce440@lab.ntt.co.jp
but since there's no foreign tables involved at all, either it's
a different bug or he misdiagnosed what he was seeing.

            regards, tom lane

On Sun, Apr 7, 2019 at 5:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> This test script works fine in HEAD:
>
> drop table if exists parttbl cascade;
> CREATE TABLE parttbl (a int, b int) PARTITION BY LIST (a);
> CREATE TABLE parttbl_1 PARTITION OF parttbl FOR VALUES IN (NULL,500,501,502);
> UPDATE parttbl SET a = NULL, b = NULL WHERE a = 1600 AND b = 999;
>
> In v11, it suffers an assertion failure in ExecSetupPartitionTupleRouting.
>
> In v10, it doesn't crash, but we do get
>
> WARNING:  relcache reference leak: relation "parttbl" not closed
>
> which is surely a bug as well.
>
> (This is a boiled-down version of the script I mentioned in
> https://www.postgresql.org/message-id/13344.1554578481@sss.pgh.pa.us)

What we did in the following commit is behind this:

commit 58947fbd56d1481a86a03087c81f728fdf0be866
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Fri Feb 22 12:23:00 2019 -0500

    Fix plan created for inherited UPDATE/DELETE with all tables excluded.

Before this commit, partitioning related code in the executor could
always rely on the fact that ModifyTableState.resultRelInfo[] only
contains *leaf* partitions.  As of this commit, it may contain the
root partitioned table in some cases, which breaks that assumption.

I've attached fixes for PG 10 and 11, modifying ExecInitModifyTable()
and inheritance_planner(), respectively.

> This seems to be related to what Amit Langote complained of in
> https://www.postgresql.org/message-id/21e7eaa4-0d4d-20c2-a1f7-c7e96f4ce440@lab.ntt.co.jp
> but since there's no foreign tables involved at all, either it's
> a different bug or he misdiagnosed what he was seeing.

I think that one is a different bug, but maybe I haven't looked closely enough.

Thanks,
Amit

Amit Langote <amitlangote09@gmail.com> writes:
> On Sun, Apr 7, 2019 at 5:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This test script works fine in HEAD:
>> In v11, it suffers an assertion failure in ExecSetupPartitionTupleRouting.
>> In v10, it doesn't crash, but we do get
>> WARNING:  relcache reference leak: relation "parttbl" not closed

> What we did in the following commit is behind this:
> commit 58947fbd56d1481a86a03087c81f728fdf0be866
> Before this commit, partitioning related code in the executor could
> always rely on the fact that ModifyTableState.resultRelInfo[] only
> contains *leaf* partitions.  As of this commit, it may contain the
> root partitioned table in some cases, which breaks that assumption.

Ah.  Thanks for the diagnosis and patches; pushed.

I chose to patch HEAD similarly to v11, even though no bug manifests
right now; it seems safer that way.  We should certainly have the
test case in HEAD, now that we realize there wasn't coverage for this.

            regards, tom lane

(2019/04/07 16:54), Amit Langote wrote:
> On Sun, Apr 7, 2019 at 5:28 AM Tom Lane<tgl@sss.pgh.pa.us>  wrote:

>> This seems to be related to what Amit Langote complained of in
>> https://www.postgresql.org/message-id/21e7eaa4-0d4d-20c2-a1f7-c7e96f4ce440@lab.ntt.co.jp
>> but since there's no foreign tables involved at all, either it's
>> a different bug or he misdiagnosed what he was seeing.
>
> I think that one is a different bug, but maybe I haven't looked closely enough.

I started working on that from last Friday (though I didn't work on the 
weekend).  I agree on Amit's reasoning stated in that post, and I think 
that that's my fault.  Sorry for the delay.

Best regards,
Etsuro Fujita

On 2019/04/08 1:57, Tom Lane wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
>> On Sun, Apr 7, 2019 at 5:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> This test script works fine in HEAD:
>>> In v11, it suffers an assertion failure in ExecSetupPartitionTupleRouting.
>>> In v10, it doesn't crash, but we do get
>>> WARNING:  relcache reference leak: relation "parttbl" not closed
> 
>> What we did in the following commit is behind this:
>> commit 58947fbd56d1481a86a03087c81f728fdf0be866
>> Before this commit, partitioning related code in the executor could
>> always rely on the fact that ModifyTableState.resultRelInfo[] only
>> contains *leaf* partitions.  As of this commit, it may contain the
>> root partitioned table in some cases, which breaks that assumption.
> 
> Ah.  Thanks for the diagnosis and patches; pushed.

Thank you.

> I chose to patch HEAD similarly to v11, even though no bug manifests
> right now; it seems safer that way.  We should certainly have the
> test case in HEAD, now that we realize there wasn't coverage for this.

Agreed, thanks for taking care of that.

Regards,
Amit