On Tue, Dec 03, 2024 at 09:50:42AM +0530, Amit Kapila wrote: > But that suits the current design more. We allocate PGOutputData and > other contexts in that structure in a "Logical decoding context". A > few of its members (publications, publication_names) residing in > totally unrelated contexts sounds odd. In the first place, we don't > need to allocate publications under CacheMemoryContext, they should be > allocated in PGOutputData->cachectx. However, because we need to free > those entirely at one-shot during invalidation processing, we could > use a new context as a child context of PGOutputData->cachectx. Unless > I am missing something, the current memory context usage appears more > like a coding convenience than a thoughtful design decision.
PGOutputData->cachectx has been introduced in 2022 in commit 52e4f0cd4, while the decision to have RelationSyncEntry and the publication list in CacheMemoryContext gets down to v10 where this logical replication has been introduced. This was a carefully-thought choice back then because this is data that belongs to the process cache, so yes, this choice makes sense to me. Even today this choice makes sense: this is still data cached in the process, and it's stored in the memory context for cached data. The problem is that we have two locations where the cached data is stored, and I'd agree to make the whole leaner by relying on one or the other entirely, but not both. If you want to move all that to the single memory context in PGOutputData, perhaps that makes sense in the long-run and the code gets simpler. Perhaps also it could be simpler to get everything under CacheMemoryContext and remove PGOutputData->cachectx. However, if you do so, I'd suggest to use the same parent context for RelationSyncData as well rather than have two concepts, not only the publication list. That's digressing from the original subject a bit, btw.. -- Michael
signature.asc
Description: PGP signature
