On Wed, Nov 14, 2018 at 2:42 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Sat, Nov 10, 2018 at 9:27 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Mon, Nov 5, 2018 at 5:10 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > > [review for undo record layer (0003-undo-interface-v3)] > > > > I might sound repeating myself, but just to be clear, I was involved > > in the design of this patch as well and I have given a few high-level > > inputs for this patch. I have used this interface in the zheap > > development, but haven't done any sort of detailed review which I am > > doing now. I encourage others also to review this patch. > > Thanks for the review, please find my reply inline. > > > > 1. > > * NOTES: > > + * Handling multilog - > > + * It is possible that the undo record of a transaction can be spread > > across > > + * multiple undo log. And, we need some special handling while inserting > > the > > + * undo for discard and rollback to work sanely. > > > > I think before describing how the undo record is spread across > > multiple logs, you can explain how it is laid out when that is not the > > case. You can also explain how undo record headers are linked. I am > > not sure file header is the best place or it should be mentioned in > > README, but I think for now we can use file header for this purpose > > and later we can move it to README if required. > Added in the header. > > > > > 2. > > +/* > > + * FIXME: Do we want to support undo tuple size which is more than the > > BLCKSZ > > + * if not than undo record can spread across 2 buffers at the max. > > + */ > > > > +#define MAX_BUFFER_PER_UNDO 2 > > > > I think here the right question is what is the possibility of undo > > record to be greater than BLCKSZ? For zheap, as of today, we don' > > have any such requirement as the largest undo record is written for > > update or multi_insert and in both cases we don't exceed the limit of > > BLCKSZ. I guess some user other than zheap could probably have such > > requirement and I don't think it is impossible to enhance this if we > > have any requirement. > > > > If anybody else has an opinion here, please feel to share it. > > Should we remove this FIXME or lets wait for some other opinion. As > of now I have kept it as it is. > >
I think you can keep it with XXX instead of Fixme as there is nothing to fix. Both the patches 0003-undo-interface-v4.patch and 0004-undo-interface-test-v4.patch appears to be same except for the name? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
