On Tue, Jun 11, 2019 at 1:57 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > I think the existing code is horribly ugly and this is even worse. > It adds cycles to RelationDecrementReferenceCount which is a hotspot > that has no business dealing with this; the invariants are unclear; > and there's no strong reason to think there aren't still cases where > we accumulate lots of copies of old partition descriptors during a > sequence of operations. Basically you're just doubling down on a > wrong design.
I don't understand why a function that decrements a reference count shouldn't be expected to free things if the reference count goes to zero. Under what workload do you think adding this to RelationDecrementReferenceCount would cause a noticeable performance regression? I think the change is responsive to your previous complaint that the timing of stuff getting freed is not very well pinned down. With this change, it's much more tightly pinned down: it happens when the refcount goes to 0. That is definitely not perfect, but I think that it is a lot easier to come up with scenarios where the leak accumulates because no cache flush happens while the relfcount is 0 than it is to come up with scenarios where the refcount never reaches 0. I agree that the latter type of scenario probably exists, but I don't think we've come up with one yet. > As I said upthread, my current inclination is to do nothing in this > area for v12 and then try to replace the whole thing with proper > reference counting in v13. I think the cases where we have a major > leak are corner-case-ish enough that we can leave it as-is for one > release. Is this something you're planning to work on yourself? Do you have a design in mind? Is the idea to reference-count the PartitionDesc? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company