On Tue, Jun 11, 2019 at 1:57 PM Tom Lane <tgl@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