Takamichi-san, On Fri, May 14, 2021 at 11:19 AM osumi.takami...@fujitsu.com <osumi.takami...@fujitsu.com> wrote: > On Thursday, May 13, 2021 7:43 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Apr 20, 2021 at 8:36 AM Amit Langote <amitlangot...@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
