Back-branch bugs with fully-prunable UPDATEs

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
/messages/by-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
/messages/by-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
/messages/by-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