On Fri, Nov 15, 2019 at 4:19 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Nov 15, 2019 at 4:01 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Fri, Nov 15, 2019 at 3:50 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > > > > Few other comments on this patch: > > > 1. > > > + case REORDER_BUFFER_CHANGE_INVALIDATION: > > > + > > > + /* > > > + * Execute the invalidation message locally. > > > + * > > > + * XXX Do we need to care about relcacheInitFileInval and > > > + * the other fields added to ReorderBufferChange, or just > > > + * about the message itself? > > > + */ > > > + LocalExecuteInvalidationMessage(&change->data.inval.msg); > > > + break; > > > > > > Here, why are we executing messages individually? Can't we just > > > follow what we do in DecodeCommit which is to record the invalidations > > > in ReorderBufferTXN as we encounter them and then allow them to > > > execute on each REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID. Is there a > > > reason why we don't do ReorderBufferXidSetCatalogChanges when we > > > receive any invalidation message?
I think it's fine to call ReorderBufferXidSetCatalogChanges, only on commit. Because this is required to add any committed transaction to the snapshot if it has done any catalog changes. So I think there is no point in setting that flag every time we get an invalidation message. > > IMHO, the reason is that in DecodeCommit, we get all the invalidation > > at one time so, at REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID, we don't > > know which invalidation message to execute so for being safe we have > > to execute all. But, since we are logging all invalidation > > individually, we exactly know at this stage which cache to invalidate. > > So it is better to only invalidate required cache not all. > > > > In that case, invalidations can be processed multiple times, the first > time when these individual WAL logs for invalidation are processed and > then later at commit time when we accumulate all invalidation messages > and then execute them for REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID. > Can we avoid to execute invalidations from other places after this > patch which also includes executing them as part of XLOG_INVALIDATIONS > processing? I think we can avoid invalidation which is done as part of REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID. I need to further investigate the invalidation which is done as part of XLOG_INVALIDATIONS. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com