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 ?
Best Regards,
Takamichi Osumi