v12 "won't fix" item regarding memory leak in "ATTACH PARTITION without AEL"; (or, relcache ref counting)

This links to a long thread, from which I've tried to quote some of the
most important mails, below.
https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items#Won.27t_Fix

I wondered if there's an effort to pursue a resolution for v13 ?

On Fri, Apr 12, 2019 at 11:42:24AM -0400, Tom Lane wrote in <31027.1555083744@sss.pgh.pa.us>:

Michael Paquier <michael@paquier.xyz> writes:

On Wed, Apr 10, 2019 at 05:03:21PM +0900, Amit Langote wrote:

The problem lies in all branches that have partitioning, so it should be
listed under Older Bugs, right? You may have noticed that I posted
patches for all branches down to 10.

I have noticed. The message from Tom upthread outlined that an open
item should be added, but this is not one. That's what I wanted to
emphasize. Thanks for making it clearer.

To clarify a bit: there's more than one problem here. Amit added an
open item about pre-existing leaks related to rd_partcheck. (I'm going
to review and hopefully push his fix for that today.) However, there's
a completely separate leak associated with mismanagement of rd_pdcxt,
as I showed in [1], and it doesn't seem like we have consensus about
what to do about that one. AFAIK that's a new bug in 12 (caused by
898e5e329) and so it ought to be in the main open-items list.

This thread has discussed a bunch of possible future changes like
trying to replace copying of relcache-provided data structures
with reference-counting, but I don't think any such thing could be
v12 material at this point. We do need to fix the newly added
leak though.

regards, tom lane

[1] /messages/by-id/10797.1552679128@sss.pgh.pa.us

On Fri, Mar 15, 2019 at 05:41:47PM -0400, Robert Haas wrote in <CA+Tgmoa5rT+ZR+Vv+q1XLwQtDMCqkNL6B4PjR4V6YAC9K_LBxw@mail.gmail.com>:

On Fri, Mar 15, 2019 at 3:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

More to the point, we turned *one* rebuild = false situation into
a bunch of rebuild = true situations. I haven't studied it closely,
but I think a CCA animal would probably see O(N^2) rebuild = true
invocations in a query with N partitions, since each time
expand_partitioned_rtentry opens another child table, we'll incur
an AcceptInvalidationMessages call which leads to forced rebuilds
of the previously-pinned rels. In non-CCA situations, almost always
nothing happens with the previously-examined relcache entries.

That's rather unfortunate. I start to think that clobbering the cache
"always" is too hard a line.

I agree that copying data isn't great. What I don't agree is that this
solution is better. In particular, copying data out of the relcache
rather than expecting the relcache to hold still over long periods
is the way we've done things everywhere else, cf RelationGetIndexList,
RelationGetStatExtList, RelationGetIndexExpressions,
RelationGetIndexPredicate, RelationGetIndexAttrBitmap,
RelationGetExclusionInfo, GetRelationPublicationActions. I don't care
for a patch randomly deciding to do things differently on the basis of an
unsupported-by-evidence argument that it might cost too much to copy the
data. If we're going to make a push to reduce the amount of copying of
that sort that we do, it should be a separately (and carefully) designed
thing that applies to all the relcache substructures that have the issue,
not one-off hacks that haven't been reviewed thoroughly.

That's not an unreasonable argument. On the other hand, if you never
try new stuff, you lose opportunities to explore things that might
turn out to be better and worth adopting more widely.

I am not very convinced that it makes sense to lump something like
RelationGetIndexAttrBitmap in with something like
RelationGetPartitionDesc. RelationGetIndexAttrBitmap is returning a
single Bitmapset, whereas the data structure RelationGetPartitionDesc
is vastly larger and more complex. You can't say "well, if it's best
to copy 32 bytes of data out of the relcache every time we need it, it
must also be right to copy 10k or 100k of data out of the relcache
every time we need it."

There is another difference as well: there's a good chance that
somebody is going to want to mutate a Bitmapset, whereas they had
BETTER NOT think that they can mutate the PartitionDesc. So returning
an uncopied Bitmapset is kinda risky in a way that returning an
uncopied PartitionDesc is not.

If we want an at-least-somewhat unified solution here, I think we need
to bite the bullet and make the planner hold a reference to the
relcache throughout planning. (The executor already keeps it open, I
believe.) Otherwise, how is the relcache supposed to know when it can
throw stuff away and when it can't? The only alternative seems to be
to have each subsystem hold its own reference count, as I did with the
PartitionDirectory stuff, which is not something we'd want to scale
out.

I especially don't care for the much-less-than-half-baked kluge of
chaining the old rd_pdcxt onto the new one and hoping that it will go away
at a suitable time.

It will go away at a suitable time, but maybe not at the soonest
suitable time. It wouldn't be hard to improve that, though. The
easiest thing to do, I think, would be to have an rd_oldpdcxt and
stuff the old context there. If there already is one there, parent
the new one under it. When RelationDecrementReferenceCount reduces
the reference count to zero, destroy anything found in rd_oldpdcxt.
With that change, things get destroyed at the earliest time at which
we know the old things aren't referenced, instead of the earliest time
at which they are not referenced + an invalidation arrives.

regression=# create table parent (a text, b int) partition by list (a);
CREATE TABLE
regression=# create table child (a text, b int);
CREATE TABLE
regression=# do $$
regression$# begin
regression$# for i in 1..10000000 loop
regression$# alter table parent attach partition child for values in ('x');
regression$# alter table parent detach partition child;
regression$# end loop;
regression$# end $$;

Interesting example.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, Apr 14, 2019 at 03:29:26PM -0400, Tom Lane wrote in <27380.1555270166@sss.pgh.pa.us>:

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Mar 15, 2019 at 3:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I agree that copying data isn't great. What I don't agree is that this
solution is better.

I am not very convinced that it makes sense to lump something like
RelationGetIndexAttrBitmap in with something like
RelationGetPartitionDesc. RelationGetIndexAttrBitmap is returning a
single Bitmapset, whereas the data structure RelationGetPartitionDesc
is vastly larger and more complex. You can't say "well, if it's best
to copy 32 bytes of data out of the relcache every time we need it, it
must also be right to copy 10k or 100k of data out of the relcache
every time we need it."

I did not say that. What I did say is that they're both correct
solutions. Copying large data structures is clearly a potential
performance problem, but that doesn't mean we can take correctness
shortcuts.

If we want an at-least-somewhat unified solution here, I think we need
to bite the bullet and make the planner hold a reference to the
relcache throughout planning. (The executor already keeps it open, I
believe.) Otherwise, how is the relcache supposed to know when it can
throw stuff away and when it can't?

The real problem with that is that we *still* won't know whether it's
okay to throw stuff away or not. The issue with these subsidiary
data structures is exactly that we're trying to reduce the lock strength
required for changing one of them, and as soon as you make that lock
strength less than AEL, you have the problem that that value may need
to change in relcache entries despite them being pinned. The code I'm
complaining about is trying to devise a shortcut solution to exactly
that situation ... and it's not a good shortcut.

The only alternative seems to be to have each subsystem hold its own
reference count, as I did with the PartitionDirectory stuff, which is
not something we'd want to scale out.

Well, we clearly don't want to devise a separate solution for every
subsystem, but it doesn't seem out of the question to me that we could
build a "reference counted cache sub-structure" mechanism and apply it
to each of these relcache fields. Maybe it could be unified with the
existing code in the typcache that does a similar thing. Sure, this'd
be a fair amount of work, but we've done it before. Syscache entries
didn't use to have reference counts, for example.

BTW, the problem I have with the PartitionDirectory stuff is exactly
that it *isn't* a reference-counted solution. If it were, we'd not
have this problem of not knowing when we could free rd_pdcxt.

I especially don't care for the much-less-than-half-baked kluge of
chaining the old rd_pdcxt onto the new one and hoping that it will go away
at a suitable time.

It will go away at a suitable time, but maybe not at the soonest
suitable time. It wouldn't be hard to improve that, though. The
easiest thing to do, I think, would be to have an rd_oldpdcxt and
stuff the old context there. If there already is one there, parent
the new one under it. When RelationDecrementReferenceCount reduces
the reference count to zero, destroy anything found in rd_oldpdcxt.

Meh. While it seems likely that that would mask most practical problems,
it still seems like covering up a wart with a dirty bandage. In
particular, that fix doesn't fix anything unless relcache reference counts
go to zero pretty quickly --- which I'll just note is directly contrary
to your enthusiasm for holding relcache pins longer.

I think that what we ought to do for v12 is have PartitionDirectory
copy the data, and then in v13 work on creating real reference-count
infrastructure that would allow eliminating the copy steps with full
safety. The $64 question is whether that really would cause unacceptable
performance problems. To look into that, I made the attached WIP patches.
(These are functionally complete, but I didn't bother for instance with
removing the hunk that 898e5e329 added to relcache.c, and the comments
need work, etc.) The first one just changes the PartitionDirectory
code to do that, and then the second one micro-optimizes
partition_bounds_copy() to make it somewhat less expensive, mostly by
collapsing lots of small palloc's into one big one.

What I get for test cases like [1] is

single-partition SELECT, hash partitioning:

N tps, HEAD tps, patch
2 11426.243754 11448.615193
8 11254.833267 11374.278861
32 11288.329114 11371.942425
128 11222.329256 11185.845258
512 11001.177137 10572.917288
1024 10612.456470 9834.172965
4096 8819.110195 7021.864625
8192 7372.611355 5276.130161

single-partition SELECT, range partitioning:

N tps, HEAD tps, patch
2 11037.855338 11153.595860
8 11085.218022 11019.132341
32 10994.348207 10935.719951
128 10884.417324 10532.685237
512 10635.583411 9578.108915
1024 10407.286414 8689.585136
4096 8361.463829 5139.084405
8192 7075.880701 3442.542768

Now certainly these numbers suggest that avoiding the copy could be worth
our trouble, but these results are still several orders of magnitude
better than where we were two weeks ago [2]. Plus, this is an extreme
case that's not really representative of real-world usage, since the test
tables have neither indexes nor any data. In practical situations the
baseline would be lower and the dropoff less bad. So I don't feel bad
about shipping v12 with these sorts of numbers and hoping for more
improvement later.

regards, tom lane

[1] /messages/by-id/3529.1554051867@sss.pgh.pa.us

[2] /messages/by-id/0F97FA9ABBDBE54F91744A9B37151A512BAC60@g01jpexmbkw24

On Wed, May 01, 2019 at 01:09:07PM -0400, Robert Haas wrote in <CA+Tgmob-cska+-WUC7T-G4zkSJp7yum6M_bzYd4YFzwQ51qiow@mail.gmail.com>:

On Wed, May 1, 2019 at 11:59 AM Andres Freund <andres@anarazel.de> wrote:

The message I'm replying to is marked as an open item. Robert, what do
you think needs to be done here before release? Could you summarize,
so we then can see what others think?

The original issue on this thread was that hyrax started running out
of memory when it hadn't been doing so previously. That happened
because, for complicated reasons, commit
898e5e3290a72d288923260143930fb32036c00c resulted in the leak being
hit lots of times in CLOBBER_CACHE_ALWAYS builds instead of just once.
Commits 2455ab48844c90419714e27eafd235a85de23232 and
d3f48dfae42f9655425d1f58f396e495c7fb7812 fixed that problem.

In the email at issue, Tom complains about two things. First, he
complains about the fact that I try to arrange things so that relcache
data remains valid for as long as required instead of just copying it.
Second, he complains about the fact repeated ATTACH and DETACH
PARTITION operations can produce a slow session-lifespan memory leak.

I think it's reasonable to fix the second issue for v12. I am not
sure how important it is, because (1) the leak is small, (2) it seems
unlikely that anyone would execute enough ATTACH/DETACH PARTITION
operations in one backend for the leak to amount to anything
significant, and (3) if a relcache flush ever happens when you don't
have the relation open, all of the "leaked" memory will be un-leaked.
However, I believe that we could fix it as follows. First, invent
rd_pdoldcxt and put the first old context there; if that pointer is
already in use, then parent the new context under the old one.
Second, in RelationDecrementReferenceCount, if the refcount hits 0,
nuke rd_pdoldcxt and set the pointer back to NULL. With that change,
you would only keep the old PartitionDesc around until the ref count
hits 0, whereas at present, you keep the old PartitionDesc around
until an invalidation happens while the ref count is 0.

I think the first issue is not v12 material. Tom proposed to fix it
by copying all the relevant data out of the relcache, but his own
performance results show a pretty significant hit, and AFAICS he
hasn't pointed to anything that's actually broken by the current
approach. What I think should be done is actually generalize the
approach I took in this patch, so that instead of the partition
directory holding a reference count, the planner or executor holds
one. Then not only could people who want information about the
PartitionDesc avoid copying stuff from the relcache, but maybe other
things as well. I think that would be significantly better than
continuing to double-down on the current copy-everything approach,
which really only works well in a world where a table can't have all
that much metadata, which is clearly not true for PostgreSQL any more.
I'm not sure that Tom is actually opposed to this approach -- although
I may have misunderstood his position -- but where we disagree, I
think, is that I see what I did in this commit as a stepping-stone
toward a better world, and he sees it as something that should be
killed with fire until that better world has fully arrived.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jun 11, 2019 at 01:57:16PM -0400, Tom Lane wrote in <18286.1560275836@sss.pgh.pa.us>:

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Jun 6, 2019 at 2:48 AM Amit Langote <amitlangote09@gmail.com> wrote:

Attached is a patch that applies on top of Robert's pdoldcxt-v1.patch,
which seems to fix this issue for me.

Yeah, that looks right. I think my patch was full of fuzzy thinking
and inadequate testing; thanks for checking it over and coming up with
the right solution.

Anyone else want to look/comment?

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.

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.

regards, tom lane

On Wed, Jun 12, 2019 at 03:11:56PM -0400, Tom Lane wrote in <3800.1560366716@sss.pgh.pa.us>: