Re: FailedAssertion("pd_idx == pinfo->nparts", File: "execPartition.c", Line: 1689)

On Tue, Aug 4, 2020 at 1:11 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Aug 03, 2020 at 11:41:37AM -0400, Robert Haas wrote:
> > On Sun, Aug 2, 2020 at 2:11 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > Based on commit logs, I suspect this may be an "older bug", specifically maybe
> > > with:
> > >
> > > |commit 898e5e3290a72d288923260143930fb32036c00c
> > > |Author: Robert Haas <rhaas@postgresql.org>
> > > |Date:   Thu Mar 7 11:13:12 2019 -0500
> > > |
> > > |    Allow ATTACH PARTITION with only ShareUpdateExclusiveLock.
> > >
> > > I don't think it matters, but the process surrounding the table being INSERTed
> > > INTO is more than a little special, involving renames, detaches, creation,
> > > re-attaching within a transaction.  I think that doesn't matter though, and the
> > > issue is surrounding the table being SELECTed *from*, which is actually behind
> > > a view.
> >
> > That's an entirely reasonable guess, but it doesn't seem easy to
> > understand exactly what happened here based on the provided
> > information. The assertion failure probably indicates that
> > pinfo->relid_map[] and partdesc->oids[] differ in some way other than
> > additional elements having been inserted into the latter. For example,
> > some elements might have disappeared, or the order might have changed.
> > This isn't supposed to happen, because DETACH PARTITION requires
> > heavier locking, and the order changing without anything getting
> > detached should be impossible. But evidently it did. If we could dump
> > out the two arrays in question it might shed more light on exactly how
> > things went wrong.

It may be this commit that went into PG 12 that is causing the problem:

commit 428b260f87e8861ba8e58807b69d433db491c4f4
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Sat Mar 30 18:58:55 2019 -0400

    Speed up planning when partitions can be pruned at plan time.

which had this:

-               /* Double-check that list of relations has not changed. */
-               Assert(memcmp(partdesc->oids, pinfo->relid_map,
-                      pinfo->nparts * sizeof(Oid)) == 0);
+               /*
+                * Double-check that the list of unpruned relations has not
+                * changed.  (Pruned partitions are not in relid_map[].)
+                */
+#ifdef USE_ASSERT_CHECKING
+               for (int k = 0; k < pinfo->nparts; k++)
+               {
+                   Assert(partdesc->oids[k] == pinfo->relid_map[k] ||
+                          pinfo->subplan_map[k] == -1);
+               }
+#endif

to account for partitions that were pruned by the planner for which we
decided to put 0 into relid_map, but it only considered the case where
the number of partitions doesn't change since the plan was created.
The crash reported here is in the other case where the concurrently
added partitions cause the execution-time PartitionDesc to have more
partitions than the one that PartitionedRelPruneInfo is based on.

I was able to reproduce such a crash as follows:

Start with these tables in session 1.

create table foo (a int, b int) partition by list (a);
create table foo1 partition of foo for values in (1);
create table foo2 partition of foo for values in (2);
create table foo3 partition of foo for values in (3);

Attach gdb with a breakpoint set in PartitionDirectoryLookup() and run this:

explain analyze select * from foo where a <> 1 and a = (select 2);
<After hitting the breakpoint in PartitionDirectoryLookup() called by
the planner, step to the end of it and leave it there>

In another session:

create table foo4 (like foo)
alter table foo attach partition foo4 for values in (4);

That should finish without waiting for any lock and send an
invalidation message to session 1.  Go back to gdb attached to session
1 and hit continue, resulting in the plan containing runtime pruning
info being executed.  ExecCreatePartitionPruneState() opens foo which
will now have 4 partitions instead of the 3 that the planner would
have seen, of which foo1 is pruned (a <> 1), so the following block is
executed:

            if (partdesc->nparts == pinfo->nparts)
            ...
            else
            {
                int         pd_idx = 0;
                int         pp_idx;

                /*
                 * Some new partitions have appeared since plan time, and
                 * those are reflected in our PartitionDesc but were not
                 * present in the one used to construct subplan_map and
                 * subpart_map.  So we must construct new and longer arrays
                 * where the partitions that were originally present map to
                 * the same place, and any added indexes map to -1, as if the
                 * new partitions had been pruned.
                 */
                pprune->subpart_map = palloc(sizeof(int) * partdesc->nparts);
                for (pp_idx = 0; pp_idx < partdesc->nparts; ++pp_idx)
                {
                    if (pinfo->relid_map[pd_idx] != partdesc->oids[pp_idx])
                    {
                        pprune->subplan_map[pp_idx] = -1;
                        pprune->subpart_map[pp_idx] = -1;
                    }
                    else
                    {
                        pprune->subplan_map[pp_idx] =
                            pinfo->subplan_map[pd_idx];
                        pprune->subpart_map[pp_idx] =
                            pinfo->subpart_map[pd_idx++];
                    }
                }
                Assert(pd_idx == pinfo->nparts);
            }

where it crashes due to having relid_map[] and partdesc->oids[] that
look like this:

(gdb) p *pinfo->relid_map@pinfo->nparts
$3 = {0, 74106, 74109}

(gdb) p *partdesc->oids@partdesc->nparts
$6 = {74103, 74106, 74109, 74112}

The 0 in relid_map matches with nothing in partdesc->oids with the
loop ending without moving forward in the relid_map array, causing the
Assert to fail.

The attached patch should fix that.

-- 
Amit Langote
EnterpriseDB: http://www.enterprisedb.com