On Wed, Nov 20, 2019 at 8:22 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Wed, Nov 20, 2019 at 11:15 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Tue, Nov 19, 2019 at 5:23 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Mon, Nov 18, 2019 at 5:02 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > > > > > 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. > > > > > > > > > > Hmm, this is also used to build cid hash map (see > > > ReorderBufferBuildTupleCidHash) which we need to use while streaming > > > changes for the in-progress transactions. So, I think that it would > > > be required earlier (before commit) as well. > > > > > Oh right, I guess I missed that part. > > Attached a new rebased version of the patch set. I have fixed all > the issues discussed up-thread and agreed upon. > > Pending Issues: > 1. The default value of the logical_decoding_work_mem is set to 64kb > in test_decoding/logical.conf. So we need to change the expected > output files for the test decoding module. > 2. Need to complete the patch for concurrent abort handling of the > (sub)transaction. There are some pending issues with the existing > patch[1]. > [1] > https://www.postgresql.org/message-id/CAFiTN-ud98kWHCo2YKS55H8rGw3_A7ESyssHwU0xPU6KJsoy6A%40mail.gmail.com Apart from these there is one more issue reported upthread[2] [2] https://www.postgresql.org/message-id/CAFiTN-vrSNkAfRVrWKe2R1dqFBTubjt%3DDYS%3DjhH%2BjiCoBODdaw%40mail.gmail.com
-- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
