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(at)alvh(dot)no-ip(dot)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/)