Re: Forget close an open relation in ReorderBufferProcessTXN()

Takamichi-san,

On Fri, May 14, 2021 at 11:19 AM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
> On Thursday, May 13, 2021 7:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Tue, Apr 20, 2021 at 8:36 AM Amit Langote <amitlangote09@gmail.com>
> > wrote:
> > > Back in:
> > https://www.postgresql.org/message-id/CA%2BHiwqEeU19iQgjN6HF1HTP
> > U0L5%2
> > > BJxyS5CmxaOVGNXBAfUY06Q%40mail.gmail.com
> > >
> > > I had proposed to move the map creation from maybe_send_schema() to
> > > get_rel_sync_entry(), mainly because the latter is where I realized it
> > > belongs, though a bit too late.
> >
> > It seems in get_rel_sync_entry, it will only build the map again when there is
> > any invalidation in publication_rel. Don't we need to build it after any DDL on
> > the relation itself? I haven't tried this with a test so I might be missing
> > something.
> Yeah, the patch not only tries to address the memory leak
> but also changes the timing (condition) to call convert_tuples_by_name.
> This is because the patch placed the function within a condition of !entry->replicate_valid in get_rel_sync_entry.
> OTOH, OSS HEAD calls it based on RelationSyncEntry's schema_sent in maybe_send_schema.
>
> The two flags (replicate_valid and schema_sent) are reset at different timing somehow.
> InvalidateSystemCaches resets both flags but schema_send is also reset by LocalExecuteInvalidationMessage
> while replicate_valid is reset by CallSyscacheCallbacks.
>
> IIUC, InvalidateSystemCaches, which applies to both flags, is called
> when a transaction starts, via AtStart_Cache and when a table lock is taken via LockRelationOid, etc.
> Accordingly, I think we can notice changes after any DDL on the relation.
>
> But, as for the different timing, we need to know the impact of the change accurately.
> LocalExecuteInvalidationMessage is called from functions in reorderbuffer
> (e.g. ReorderBufferImmediateInvalidation, ReorderBufferExecuteInvalidations).
> This seems to me that changing the condition by the patch
> reduces the chance of the reorderbuffer's proactive reset of
> the flag which leads to rebuild the map in the end.
>
> Langote-san, could you please explain this perspective ?

Please check the reply I just sent.  In summary, moving map-creation
into get_rel_sync_entry() was not correct.

-- 
Amit Langote
EDB: http://www.enterprisedb.com