Thanks for re-reviewing! This one I hope is the last version.
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.
Would it be a bit more readable to just duplicate this stanza in the
blocks that assign to rd_partdesc_nodetached and rd_partdesc,
respectively? That's not much code to duplicate and it'd be easier to
see which context is for which partdesc.
Sure .. that's how I first wrote this code. We don't use that style much, so I'm OK with backing out of it.
+ TransactionId rd_partdesc_nodetached_xmin; /* xmin for the above */
Could you please expand this description a bit?
Done.