On Thu, Dec 26, 2019 at 3:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > Amit Langote <amitlangote09@gmail.com> writes:
> >> On Tue, Dec 24, 2019 at 10:59 AM Amit Langote <amitlangote09@gmail.com> wrote:
> >>> Btw, does the memory leakage fix in this patch address any of the
> >>> pending concerns that were discussed on the "hyrax vs.
> >>> RelationBuildPartitionDesc" thread earlier this year[1]?
> >>> [1] https://www.postgresql.org/message-id/flat/3800.1560366716%40sss.pgh.pa.us#092b6b4f6bf75d2f3f90ef6a3b3eab5b
>
> >> I thought about this a little and I think it *does* address the main
> >> complaint in the above thread.
>
> It occurred to me to also recheck the original complaint in that thread,
> which was poor behavior in CLOBBER_CACHE_ALWAYS builds.
Thanks for taking the time to do that.
> I didn't have
> the patience to run a full CCA test, but I did run update.sql, which
> we previously established was sufficient to show the problem. There's
> no apparent memory bloat, either with HEAD or with the patch. I also
> see the runtime (for update.sql on its own) dropping from about
> 474 sec in HEAD to 457 sec with the patch. So that indicates that we're
> actually saving a noticeable amount of work, not just postponing it,
> at least under CCA scenarios where relcache entries get flushed a lot.
Yeah, as long as nothing in between those flushes needs to look at the
partition descriptor.
> I also tried to measure update.sql's runtime in a regular debug build
> (not CCA). I get pretty repeatable results of 279ms on HEAD vs 273ms
> with patch, or about a 2% overall savings. That's at the very limit of
> what I'd consider a reproducible difference, but still it seems to be
> real. So that seems like evidence that forcing the partition data to be
> loaded immediately rather than on-demand is a loser from a performance
> standpoint as well as the recursion concerns that prompted this patch.
Agreed.
> Which naturally leads one to wonder whether forcing other relcache
> substructures (triggers, rules, etc) to be loaded immediately isn't
> a loser as well. I'm still feeling like we're overdue to redesign how
> all of this works and come up with a more uniform, less fragile/ad-hoc
> approach. But I don't have the time or interest to do that right now.
I suppose if on-demand loading of partition descriptors can result in
up to 2% savings, we can perhaps expect slightly more by doing the
same for other substructures. Also, the more different substructures
are accessed similarly the better.
> Anyway, I've run out of reasons not to commit this patch, so I'll
> go do that.
Thank you. I noticed that there are comments suggesting that certain
RelationData members are to be accessed using their RelationGet*
functions, but partitioning members do not have such comments. How
about the attached?
Regards,
Amit