Robert Haas writes:
> On Tue, Jun 11, 2019 at 1:57 PM Tom Lane wrote:
> 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
On Tue, Jun 11, 2019 at 1:57 PM Tom Lane 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
Robert Haas writes:
> On Thu, Jun 6, 2019 at 2:48 AM Amit Langote 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
On Thu, Jun 6, 2019 at 2:48 AM Amit Langote 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
On Thu, Jun 6, 2019 at 3:47 PM Amit Langote wrote:
> While it's
> problematic that the session in which a new partition is attached to
> the parent relation broadcasts 2 SI messages to invalidate the parent
> relation [1], it's obviously better to fix how RelationClearRelation
> manipulates
On Wed, Jun 5, 2019 at 8:03 PM Amit Langote wrote:
> I noticed a crash with one of the scenarios that I think the patch is
> meant to address. Let me describe the steps:
>
> Attach gdb (or whatever) to session 1 in which we'll run a query that
> will scan at least two partitions and set a
On Tue, Jun 4, 2019 at 9:25 PM Robert Haas wrote:
> On Fri, May 17, 2019 at 4:36 PM Tom Lane wrote:
> > Yeah, I did some additional testing that showed that it's pretty darn
> > hard to get the leak to amount to anything. The test case that I
> > originally posted did many DDLs in a single
On Fri, May 17, 2019 at 4:36 PM Tom Lane wrote:
> Yeah, I did some additional testing that showed that it's pretty darn
> hard to get the leak to amount to anything. The test case that I
> originally posted did many DDLs in a single transaction, and it
> seems that that's actually essential to
Andres Freund writes:
> On 2019-05-01 13:09:07 -0400, Robert Haas wrote:
>> 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.
>>
Hi,
On 2019-05-01 13:09:07 -0400, Robert Haas wrote:
> 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
On Wed, May 1, 2019 at 11:59 AM Andres Freund 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
Hi
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?
Greetings,
Andres Freund
On 2019/04/17 18:58, Amit Langote wrote:
> where index on queries
> column
Oops, I meant "with an index on the queried column".
Thanks,
Amit
On 2019/04/15 4:29, Tom Lane wrote:
> 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
On Sun, Apr 14, 2019 at 3:29 PM Tom Lane wrote:
> What I get for test cases like [1] is
>
> single-partition SELECT, hash partitioning:
>
> N tps, HEAD tps, patch
> 2 11426.24375411448.615193
> 8 11254.83326711374.278861
> 32 11288.32911411371.942425
> 128
Amit Langote writes:
>> I get that we want to get rid of the keep_* kludge in the long term, but
>> is it wrong to think, for example, that having keep_partdesc today allows
>> us today to keep the pointer to rd_partdesc as long as we're holding the
>> relation open or refcnt on the whole
Amit Langote writes:
> On 2019/04/15 2:38, Tom Lane wrote:
>> To my mind there are only two trustworthy solutions to the problem of
>> wanting time-extended usage of a relcache subsidiary data structure: one
>> is to copy it, and the other is to reference-count it. I think that going
>> over to
On 2019/04/15 2:38, Tom Lane wrote:
> Amit Langote writes:
>> On 2019/04/10 15:42, Michael Paquier wrote:
>>> It seems to me that Tom's argument to push in the way relcache
>>> information is handled by copying its contents sounds sensible to me.
>>> That's not perfect, but it is consistent with
On 2019/04/14 0:53, Tom Lane wrote:
> Amit Langote writes:
>> On Sat, Apr 13, 2019 at 4:47 AM Tom Lane wrote:
>>> I concluded that that's not parenthetical but pretty relevant...
>
>> Hmm, do you mean we should grow the same "keep_" logic for
>> rd_partcheck as we have for rd_partkey and
Robert Haas writes:
> On Fri, Mar 15, 2019 at 3:45 PM Tom Lane 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
>
Amit Langote writes:
> On 2019/04/10 15:42, Michael Paquier wrote:
>> It seems to me that Tom's argument to push in the way relcache
>> information is handled by copying its contents sounds sensible to me.
>> That's not perfect, but it is consistent with what exists (note
>> PublicationActions
Amit Langote writes:
> On Sat, Apr 13, 2019 at 4:47 AM Tom Lane wrote:
>> I concluded that that's not parenthetical but pretty relevant...
> Hmm, do you mean we should grow the same "keep_" logic for
> rd_partcheck as we have for rd_partkey and rd_partdesc? I don't see
> it in the updated
Thanks for the updated patch.
On Sat, Apr 13, 2019 at 4:47 AM Tom Lane wrote:
> Robert Haas writes:
> > As a parenthetical note, I observe that relcache.c seems to know
> > almost nothing about rd_partcheck. rd_partkey and rd_partdesc both
> > have handling in RelationClearRelation(), but
Robert Haas writes:
> As a parenthetical note, I observe that relcache.c seems to know
> almost nothing about rd_partcheck. rd_partkey and rd_partdesc both
> have handling in RelationClearRelation(), but rd_partcheck does not,
> and I suspect that's wrong. So the problems are probably not
Michael Paquier 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.
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
Hi,
On 2019/04/10 15:42, Michael Paquier wrote:
> On Mon, Apr 08, 2019 at 10:40:41AM -0400, Robert Haas wrote:
>> On Mon, Apr 8, 2019 at 9:59 AM Tom Lane wrote:
>>> Amit Langote writes:
>>> Yeah, it's an open issue IMO. I think we've been focusing on getting
>>> as many feature patches done as
On Mon, Apr 08, 2019 at 10:40:41AM -0400, Robert Haas wrote:
> On Mon, Apr 8, 2019 at 9:59 AM Tom Lane wrote:
>> Amit Langote writes:
>> Yeah, it's an open issue IMO. I think we've been focusing on getting
>> as many feature patches done as we could during the CF, but now it's
>> time to start
On 2019/03/16 6:41, Robert Haas wrote:
> On Fri, Mar 15, 2019 at 3:45 PM Tom Lane wrote:
>> 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
On 2019/04/08 22:59, Tom Lane wrote:
> Amit Langote writes:
>> Should I add this patch to Older Bugs [1]?
>
> Yeah, it's an open issue IMO. I think we've been focusing on getting
> as many feature patches done as we could during the CF, but now it's
> time to start mopping up problems like this
On Mon, Apr 8, 2019 at 9:59 AM Tom Lane wrote:
> Amit Langote writes:
> > Should I add this patch to Older Bugs [1]?
>
> Yeah, it's an open issue IMO. I think we've been focusing on getting
> as many feature patches done as we could during the CF, but now it's
> time to start mopping up
Amit Langote writes:
> Should I add this patch to Older Bugs [1]?
Yeah, it's an open issue IMO. I think we've been focusing on getting
as many feature patches done as we could during the CF, but now it's
time to start mopping up problems like this one.
regards, tom lane
On 2019/03/14 10:40, Amit Langote wrote:
> On 2019/03/14 5:18, Tom Lane wrote:
>> Robert Haas writes:
>>> On Wed, Mar 13, 2019 at 3:14 PM Tom Lane wrote:
Meanwhile, who's going to take point on cleaning up rd_partcheck?
I don't really understand this code well enough to know whether
On Fri, Mar 15, 2019 at 3:45 PM Tom Lane 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
Robert Haas writes:
> On Fri, Mar 15, 2019 at 2:18 PM Tom Lane wrote:
>> BTW, after closer study of 898e5e329 I have a theory as to why it
>> made things worse for CCA animals: it causes relcache entries to be
>> held open (via incremented refcounts) throughout planning, which
>> they were not
On Fri, Mar 15, 2019 at 2:18 PM Tom Lane wrote:
> BTW, after closer study of 898e5e329 I have a theory as to why it
> made things worse for CCA animals: it causes relcache entries to be
> held open (via incremented refcounts) throughout planning, which
> they were not before. This means that,
Robert Haas writes:
> On Thu, Mar 14, 2019 at 12:36 PM Tom Lane wrote:
>> Hmm, I wonder why not. I suppose the answer is that
>> the leak is worse in HEAD than before, but how come?
> I'd like to know that, too, but right now I don't.
BTW, after closer study of 898e5e329 I have a theory as to
Robert Haas writes:
> On Thu, Mar 14, 2019 at 6:08 PM Tom Lane wrote:
>> And it's
>> still calling get_rel_relkind() in the rd_pdcxt context, potentially
>> leaking a *lot* of stuff into that normally-long-lived context, since
>> that will result in fresh catcache (and thence potentially
Heikki Linnakangas writes:
> On 15/03/2019 00:08, Tom Lane wrote:
>> Possibly it'd make sense to conditionally compile this so that we only
>> do it in a CLOBBER_CACHE_ALWAYS build. I'm not very sure about that,
>> but arguably in a normal build the overhead of making and destroying
>> a context
On Thu, Mar 14, 2019 at 6:08 PM Tom Lane wrote:
> I found that even with 2455ab488, there still seemed to be an
> unreasonable amount of MessageContext bloat during some of the queries,
> on the order of 50MB in some of them. Investigating more closely,
> I found that 2455ab488 has a couple of
On Fri, Mar 15, 2019 at 3:32 AM Heikki Linnakangas wrote:
> We maintain a free list of AllocSetContexts nowadays, so creating a
> short-lived context should be pretty cheap. Or if it's still too
> expensive, we could create one short-lived context as a child of
> TopMemoryContext, and reuse that
On 15/03/2019 00:08, Tom Lane wrote:
What I'm thinking, therefore, is that 2455ab488 had the right idea but
didn't take it far enough. We should remove the temp-context logic it
added to RelationBuildPartitionDesc and instead put that one level up,
in RelationBuildDesc, where the same temp
Robert Haas writes:
> On Thu, Mar 14, 2019 at 12:36 PM Tom Lane wrote:
>> Hmm, I wonder why not. I suppose the answer is that
>> the leak is worse in HEAD than before, but how come?
> I'd like to know that, too, but right now I don't.
I poked at this some more, by dint of adding some memory
On Thu, Mar 14, 2019 at 12:36 PM Tom Lane wrote:
> Hmm, I wonder why not. I suppose the answer is that
> the leak is worse in HEAD than before, but how come?
I'd like to know that, too, but right now I don't.
> I followed your reference to 898e5e329, and I've got to say that
> the hunk it adds
Robert Haas writes:
> I did not back-patch, because the code is in a different file in v11,
> none of the hunks of the patch apply on v11, and v11 is not failing on
> hyrax.
Hmm, I wonder why not. I suppose the answer is that
the leak is worse in HEAD than before, but how come?
I followed your
On Wed, Mar 13, 2019 at 2:59 PM Robert Haas wrote:
> Yeah, that probably makes sense, though it might be polite to wait
> another hour or two to see if anyone wants to argue with that approach
> further.
Hearing nobody, done. If someone wants to argue more we can always
change it later.
I did
On 2019/03/14 5:18, Tom Lane wrote:
> Robert Haas writes:
>> On Wed, Mar 13, 2019 at 3:14 PM Tom Lane wrote:
>>> Meanwhile, who's going to take point on cleaning up rd_partcheck?
>>> I don't really understand this code well enough to know whether that
>>> can share one of the existing
Hi,
On 2019-03-13 17:10:55 -0400, Tom Lane wrote:
> There's already a mechanism in there to suppress child contexts after
> 100 or so, which would almost inevitably kick in on the relcache if we
> did this. So I don't believe we'd have a problem with the context dumps
> getting too long --- more
Andres Freund writes:
> On 2019-03-13 16:50:53 -0400, Robert Haas wrote:
>> On Wed, Mar 13, 2019 at 4:38 PM Robert Haas wrote:
>>> On Wed, Mar 13, 2019 at 4:18 PM Tom Lane wrote:
Off topic for the moment, since this clearly wouldn't be back-patch
material, but I'm starting to wonder
Hi,
On 2019-03-13 16:50:53 -0400, Robert Haas wrote:
> On Wed, Mar 13, 2019 at 4:38 PM Robert Haas wrote:
> > On Wed, Mar 13, 2019 at 4:18 PM Tom Lane wrote:
> > > Off topic for the moment, since this clearly wouldn't be back-patch
> > > material, but I'm starting to wonder if we should just
On Wed, Mar 13, 2019 at 4:38 PM Robert Haas wrote:
> On Wed, Mar 13, 2019 at 4:18 PM Tom Lane wrote:
> > Off topic for the moment, since this clearly wouldn't be back-patch
> > material, but I'm starting to wonder if we should just have a context
> > for each relcache entry and get rid of most
On Wed, Mar 13, 2019 at 4:18 PM Tom Lane wrote:
> Off topic for the moment, since this clearly wouldn't be back-patch
> material, but I'm starting to wonder if we should just have a context
> for each relcache entry and get rid of most or all of the retail
> cleanup logic in
Robert Haas writes:
> On Wed, Mar 13, 2019 at 3:14 PM Tom Lane wrote:
>> Meanwhile, who's going to take point on cleaning up rd_partcheck?
>> I don't really understand this code well enough to know whether that
>> can share one of the existing partitioning-related sub-contexts.
> To your
On Wed, Mar 13, 2019 at 3:14 PM Tom Lane wrote:
> Yeah, but usually there's some comment pointing it out. I also wonder
> if there aren't corner-case bugs; it seems a bit bogus for example that
> rd_pdcxt is created without any thought as to whether it might be set
> already. It's not clear
Robert Haas writes:
> On Wed, Mar 13, 2019 at 2:26 PM Tom Lane wrote:
>> I think that RelationBuildPartitionDesc could use some additional cleanup
>> or at least better commenting. In particular, it's neither documented nor
>> obvious to the naked eye why rel->rd_partdesc mustn't get set till
On Wed, Mar 13, 2019 at 2:26 PM Tom Lane wrote:
> OK, in that case it's definitely all the temporary data that gets created
> that is the problem. I've not examined your patch in great detail but
> it looks plausible for fixing that.
Cool.
> I think that RelationBuildPartitionDesc could use
On Wed, Mar 13, 2019 at 2:21 PM Alvaro Herrera wrote:
> On 2019-Mar-13, Robert Haas wrote:
> > On Wed, Mar 13, 2019 at 1:38 PM Alvaro Herrera
> > wrote:
> > > A bit, yes, but not overly so, and it's less fragile that not having
> > > such a protection. Anything that allocates in
Alvaro Herrera writes:
> You do? Unless I misunderstood, your options are:
> 1. (the patch you attached) create a temporary memory context that is
> used for everything, then at the end copy the good stuff to CacheMemCxt
> (or a sub-context thereof). This still needs to copy.
> 2. create a
Robert Haas writes:
>> Dunno if that's related to hyrax's issue, though.
> It's related in the sense that it's a leak, and any leak will tend to
> run the system out of memory more easily, but what I observed was a
> large leak into MessageContext, and that would be a leak into
>
On 2019-Mar-13, Robert Haas wrote:
> On Wed, Mar 13, 2019 at 1:38 PM Alvaro Herrera
> wrote:
> > A bit, yes, but not overly so, and it's less fragile that not having
> > such a protection. Anything that allocates in CacheMemoryContext needs
> > to be very careful anyway.
>
> True, but I think
On Wed, Mar 13, 2019 at 1:30 PM Tom Lane wrote:
> Ah, here we are: it was rel->rd_partcheck. I'm not sure exactly how
> complicated that structure can be, but I'm pretty sure that this is
> a laughably inadequate method of cleaning it up:
>
> if (relation->rd_partcheck)
>
On Wed, Mar 13, 2019 at 1:38 PM Alvaro Herrera wrote:
> A bit, yes, but not overly so, and it's less fragile that not having
> such a protection. Anything that allocates in CacheMemoryContext needs
> to be very careful anyway.
True, but I think it's more fragile than either of the options I
On 2019-Mar-13, Robert Haas wrote:
> On Wed, Mar 13, 2019 at 12:42 PM Alvaro Herrera
> wrote:
> > I remember going over this code's memory allocation strategy a bit to
> > avoid the copy while not incurring potential leaks CacheMemoryContext;
> > as I recall, my idea was to use two contexts, one
I wrote:
> I recall having noticed someplace where I thought the relcache partition
> support was simply failing to make provisions for cleaning up a cached
> structure at relcache entry drop, but I didn't have time to pursue it
> right then. Let me see if I can reconstruct what I was worried
On Wed, Mar 13, 2019 at 1:15 PM Tom Lane wrote:
> I'm a bit confused as to why there's an issue here at all. The usual
> plan for computed-on-demand relcache sub-structures is that we compute
> a working copy that we're going to return to the caller using the
> caller's context (which is
Robert Haas writes:
> On Wed, Mar 13, 2019 at 12:42 PM Alvaro Herrera
> wrote:
>> I remember going over this code's memory allocation strategy a bit to
>> avoid the copy while not incurring potential leaks CacheMemoryContext;
>> as I recall, my idea was to use two contexts, one of which is
On Wed, Mar 13, 2019 at 12:42 PM Alvaro Herrera
wrote:
> I remember going over this code's memory allocation strategy a bit to
> avoid the copy while not incurring potential leaks CacheMemoryContext;
> as I recall, my idea was to use two contexts, one of which is temporary
> and used for any
On 2019-Mar-13, Robert Haas wrote:
> RelationBuildPartitionDesc() creates basically all of the data
> structures it needs and then copies them into rel->rd_pdcxt, which has
> always seemed a bit inefficient to me. Another way to redesign this
> would be to have the function create a temporary
68 matches
Mail list logo
