Re: memory leak in logical WAL sender with pgoutput's cachectx

Hi,

On Wed, Aug 20, 2025 at 8:44 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Sun, Aug 17, 2025 at 11:30 PM Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> > Dear Sawada-san,
> >
> > > I've not verified, but even if that's true, IIUC only one relation's
> > > cache entry can set in_use to true at a time.
> >
> > I also think so.
> >
> > > If my understanding is
> > > correct, when the walsender accepts invalidation messages in
> > > logicalrep_write_tuple() as you mentioned, it doesn't release the
> > > cache of the relation whose change is being sent but does for other
> > > relations. It seems to me too aggressive to release caches compared to
> > > the current behavior.
> >
> > Valid point. In some cases, publications can be altered then same relsync entries
> > would be added again.
> >
> > So... first approach may be better from the perspective. Attached patch counts
> > the number of invalidated entries. There is a chance to cleanup them only at
> > COMMIT/ABORT/PREPARE time - hence in_use flag is not needed. Thought?
> >
>
> In the proposed patch, we have the following change:
>
> @@ -2060,6 +2081,11 @@ get_rel_sync_entry(PGOutputData *data, Relation relation)
> entry->columns = NULL;
> entry->attrmap = NULL;
> }
> + else
> + {
> + /* The entry would be re-used, decrement the counter */
> + RelationSyncCacheInvalidateCounter--;
> + }
>
> It decrements the counter whenever we successfully find the entry from
> the cache but I'm not sure this is the right approach. What if no
> cache invalidation happens at all but we retrieve entries from the
> cache many times?
>

This may not be ideal. It decrements on every lookup of an existing
entry, not just when consuming an invalidation, which could make the
counter go
negative. Do we need decrementing logic? Not perfect 1:1 tracking
seems ok in here; though it might make the clean-up a bit more
aggressive.

Best,
Xuneng