From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY
Date: Thu, 22 Apr 2021 15:26:02 -0400
Message-ID: <20210422192602.GA19124@alvherre.pgsql> (raw)
In-Reply-To: <CA+HiwqFgpP1LxJZOBYGt9rpvTjXXkg5qG2+Xch2Z1Q7KrqZR1A@mail.gmail.com>
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/)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, amitlangote09@gmail.com, pryzby@telsasoft.com, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY
In-Reply-To: <20210422192602.GA19124@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox