(Thanks for committing the fix.) On Thu, Apr 29, 2021 at 1:11 AM Álvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > On Wed, Apr 28, 2021, at 10:21 AM, Amit Langote wrote: > I noticed that rd_partdesc_nodetached_xmin can sometimes end up with > value 0. While you seem to be already aware of that, because otherwise > you wouldn't have added TransactionIdIsValid(...) in condition in > RelationGetPartitionDesc(), the comments nearby don't mention why such > a thing might happen. Also, I guess it can't be helped that the > partdesc_nodetached will have to be leaked when the xmin is 0, but > that shouldn't be as problematic as the case we discussed earlier. > > > The only case I am aware where that can happen is if the pg_inherits tuple is > frozen. (That's exactly what the affected test case was testing, note the > "VACUUM FREEZE pg_inherits" there). So that test case blew up immediately; > but I think the real-world chances that people are going to be doing that are > pretty low, so I'm not really concerned about the leak.
The case I was looking at is when a partition detach appears as in-progress to a serializable transaction. If the caller wants to omit detached partitions, such a partition ends up in rd_partdesc_nodetached, with the corresponding xmin being set to 0 due to the way find_inheritance_children_extended() sets *detached_xmin. The next query in the transaction that wants to omit detached partitions, seeing rd_partdesc_nodetached_xmin being invalid, rebuilds the partdesc, again including that partition because the snapshot wouldn't have changed, and so on until the transaction ends. Now, this can perhaps be "fixed" by making find_inheritance_children_extended() set the xmin outside the snapshot-checking block, but maybe there's no need to address this on priority. Rather, a point that bothers me a bit is that we're including a detached partition in the partdesc labeled "nodetached" in this particular case. Maybe we should avoid that by considering in this scenario that no detached partitions exist for this transactions and so initialize rd_partdesc, instead of rd_partdesc_nodetached. That will let us avoid the situations where the xmin is left in invalid state. Maybe like the attached (it also fixes a couple of typos/thinkos in the previous commit). Note that we still end up in the same situation as before where each query in the serializable transaction that sees the detach as in-progress to have to rebuild the partition descriptor omitting the detached partitions, even when it's clear that the detach-in-progress partition will be included every time. -- Amit Langote EDB: http://www.enterprisedb.com
partdesc_nodetached-only-if-detached-visible.patch
Description: Binary data
