Re: Invalid pointer access in logical decoding after error

On Mon, Oct 6, 2025 at 6:55 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> On Mon, Oct 6, 2025, at 5:00 PM, Masahiko Sawada wrote:
> > I agree with your analysis. It seems there is no convenient way to
> > move RelationSyncCache inside PGOutputData. So let's proceed with the
> > proposed approach.
> >
>
> +1. Shouldn't we add a comment mentioning that it is a possibility?

I think the primary reason why we cannot simply move RelationSyncCache
to PGOutputData is there is no way to unregister the invalidation
callback, which is already mentioned in some places:

/*
* We can get here if the plugin was used in SQL interface as the
* RelationSyncCache is destroyed when the decoding finishes, but there is
* no way to unregister the relcache invalidation callback.
*/
if (RelationSyncCache == NULL)

>
> > I've done some minor changes to your v2 patch and updated the commit
> > message. IIUC this patch needs to be backpatched to v15. Please review
> > the attached patch.
> >
>
> I verified that the bug exists since v15 as reported. Despite of the test case
> provided by Vignesh (which I attached a modified version to be used in v15 or
> later), I also added another test case that has a similar problem with
> generated columns. This 2nd test case only works for v18 (where the feature was
> introduced). This patch also fixes this case.

Thank you for the tests!

> I'm curious about other cases related to RelationSyncCache. Is there any other
> cases that this patch doesn't fix?

I think that with the patch we can ensure to clean up
RelationSyncCache alongside with other memory contexts used in
pgoutput, fixing problems stemming from RelationSyncCache referencing
already-freed-memory in the pgoutput memory contexts.

IIUC there is another memory leak issue in pgoutput as I reported on
this thread[1]. It should be fixed in a separate patch.

> This patch looks good to me. Do we really need a new function with the same
> content as pgoutput_shutdown?

Probably we can call pgoutput_shutdown() in rel_sync_cache_reset_cb().

> I don't like mcallback. It seems 'm' stands for
> 'memory' but if we want to use crypt names I would suggest 'mcb' (_m_emory
> _c_all_b_ack) -- that is also a crypt name but a shorter one. Same pattern is
> already used in another place with MemoryContextCallback.

I think it's better to use 'mcallback' since back branches are already
using it (see commit bbe68c13a for example). While considering
backpatching this patch, I noticed that the memory context reset
callback function should be registered to ctx->context but not
ctx->cachectx.

Regards,

[1] https://www.postgresql.org/message-id/CAD21AoCgqZ0BUpXjVY6tD1jSLtVSdWpG%2BLZyZimq4Uu3TymTAA%40mail.gmail.com

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com