Date: Tue, 13 Apr 2021 12:10:30 -0400
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY
Message-ID: <20210413161030.GA10776@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+HiwqFxY2HbbKV0B-v4qKLe7KFmotHRJWXo-eN4bX=pdqu_rQ@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: 
 <https://www.postgresql.org/message-id/20210413161030.GA10776%40alvherre.pgsql>
Precedence: bulk

On 2021-Apr-13, Amit Langote wrote:

> Actually it occurred to me this morning that CLOBBER_CACHE_ALWAYS is
> what exposed this problem on this animal (not sure if other such
> animals did too though).  With CLOBBER_CACHE_ALWAYS, a PartitionDesc
> will be built afresh on most uses.  In this particular case, the RI
> query executed by the insert has to build a new one (for d4_primary),
> correctly excluding the detach-pending partition (d4_primary1) per the
> snapshot with which it is run.  In normal builds, it would reuse the
> one built by an earlier query in the transaction, which does include
> the detach-pending partition, thus allowing the insert trying to
> insert a row referencing that partition to succeed.  There is a
> provision in RelationGetPartitionDesc() to rebuild if any
> detach-pending partitions in the existing copy of PartitionDesc are
> not to be seen by the current query, but a bug mentioned in my earlier
> reply prevents that from kicking in.

Right -- that explanation makes perfect sense: the problem stems from
the fact that the cached copy of the partition descriptor is not valid
depending on the visibility of detached partitions for the operation
that requests the descriptor.  I think your patch is a critical part to
a complete solution, but one thing is missing: we don't actually know
that the detached partitions we see now are the same detached partitions
we saw a moment ago.  After all, a partitioned table can have several
partitions in the process of being detached; so if you just go with the
boolean "does it have any detached or not" bit, you could end up with a
descriptor that doesn't include/ignore *all* detached partitions, just
the older one(s).

I think you could fix this aspect easily by decreeing that you can only
have only one partition-being-detached at one point.  So if you try to
DETACH CONCURRENTLY and there's another one in that state, raise an
error.  Maybe for simplicity we should do that anyway.

But I think there's another hidden assumption in your patch, which is
that the descriptor is rebuilt every now and then *anyway* because the
flag for detached flips between parser and executor, and because we send
invalidation messages for each detach.  I don't think we would ever
change things that would break this flipping (it sounds like planner and
executor necessarily have to be doing things differently all the time),
but it seems fragile as heck.  I would feel much safer if we just
avoided caching the wrong thing ... or perhaps keep a separate cache
entry (one descriptor including detached, another one not), to avoid
pointless rebuilds.

-- 
Álvaro Herrera       Valdivia, Chile