On 2021-Apr-22, Amit Langote wrote: > On Thu, Apr 22, 2021 at 5:39 AM Alvaro Herrera <alvhe...@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/)
