Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY

On 2021-Apr-22, Amit Langote wrote:

> On Thu, Apr 22, 2021 at 5:39 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> Reading through the latest one, seeing both include_detached and
> omit_detached being used in different parts of the code makes it a bit
> hard to keep in mind what a given code path is doing wrt detached
> partitions.  How about making it all omit_detached?

Yeah, I hesitated but wanted to do that too.  Done.

>          * Cope with partitions concurrently being detached.  When we see a
> -        * partition marked "detach pending", we only include it in the set of
> -        * visible partitions if caller requested all detached partitions, or
> -        * if its pg_inherits tuple's xmin is still visible to the active
> -        * snapshot.
> +        * partition marked "detach pending", we omit it from the returned
> +        * descriptor if caller requested that and the tuple's xmin does not
> +        * appear in progress to the active snapshot.
> 
> It seems odd for a comment in find_inheritance_children() to talk
> about the "descriptor".   Maybe the earlier "set of visible
> partitions" wording was fine?

Absolutely -- done that way.

> -        * The reason for this check is that we want to avoid seeing the
> +        * The reason for this hack is that we want to avoid seeing the
>          * partition as alive in RI queries during REPEATABLE READ or
> <snip>
> +        * SERIALIZABLE transactions.
> 
> The comment doesn't quite make it clear why it is the RI query case
> that necessitates this hack in the first case.

I added "such queries use a different snapshot than the one used by
regular (user) queries."  I hope that's sufficient.

> Maybe the relation to what's going on with the partdesc
> 
> +   if (likely(rel->rd_partdesc &&
> +              (!rel->rd_partdesc->detached_exist || include_detached)))
> +       return rel->rd_partdesc;
> 
> I think it would help to have a comment about what's going here, in
> addition to the description you already wrote for
> PartitionDescData.detached_exist.  Maybe something along the lines of:
> 
> ===
> Under normal circumstances, we just return the partdesc that was
> already built.  However, if the partdesc was built at a time when
> there existed detach-pending partition(s), which the current caller
> would rather not see (omit_detached), then we build one afresh
> omitting any such partitions and return that one.
> RelationBuildPartitionDesc() makes sure that any such partdescs will
> disappear when the query finishes.
> ===
> 
> That's maybe a bit verbose but I am sure you will find a way to write
> it more succinctly.

I added some text in this spot, and also wrote some more in the comment
above RelationGetPartitionDesc and RelationBuildPartitionDesc.

> BTW, I do feel a bit alarmed by the potential performance impact of
> this.  If detached_exist of a cached partdesc is true, then RI queries
> invoked during a bulk DML operation would have to rebuild one for
> every tuple to be checked, right?  I haven't tried an actual example
> yet though.

Yeah, I was scared about that too (which is why I insisted on trying to
add a cached copy of the partdesc omitting detached partitions).  But
AFAICS what happens is that the plan for the RI query gets cached after
a few tries; so we do build the partdesc a few times at first, but later
we use the cached plan and so we no longer use that one.  So at least in
the normal cases this isn't a serious problem that I can see.

I pushed it now.  Thanks for your help,

-- 
Álvaro Herrera       Valdivia, Chile
"How strange it is to find the words "Perl" and "saner" in such close
proximity, with no apparent sense of irony. I doubt that Larry himself
could have managed it."         (ncm, http://lwn.net/Articles/174769/)