On Thu, Dec 26, 2019 at 3:21 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> I wrote:
> > Amit Langote <amitlangot...@gmail.com> writes:
> >> On Tue, Dec 24, 2019 at 10:59 AM Amit Langote <amitlangot...@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
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 2239f791e8..5ce9d8a086 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -95,10 +95,15 @@ typedef struct RelationData
List *rd_fkeylist; /* list of ForeignKeyCacheInfo (see
below) */
bool rd_fkeyvalid; /* true if list has been computed */
+ /* data managed by RelationGetPartitionKey: */
PartitionKey rd_partkey; /* partition key, or NULL */
MemoryContext rd_partkeycxt; /* private context for rd_partkey, if
any */
+
+ /* data managed by RelationGetPartitionDesc: */
PartitionDesc rd_partdesc; /* partition descriptor, or NULL */
MemoryContext rd_pdcxt; /* private context for rd_partdesc, if
any */
+
+ /* data managed by RelationGetPartitionQual: */
List *rd_partcheck; /* partition CHECK quals */
bool rd_partcheckvalid; /* true if list has been
computed */
MemoryContext rd_partcheckcxt; /* private cxt for rd_partcheck, if any
*/
