On Friday, March 17, 2023 11:49 AM Peter Smith <smithpb2...@gmail.com> wrote: > > On Wed, Mar 15, 2023 at 7:30 PM houzj.f...@fujitsu.com > <houzj.f...@fujitsu.com> wrote: > > > > Hi, > > > > I noticed that there are some duplicated codes in pgoutput_change() > > function which can be simplified, and here is an attempt to do that. > > Hi Hou-san. > > I had a quick look at the 0001 patch. > > Here are some first comments.
Thanks for the comments. > > ====== > > 1. > + if (relentry->attrmap) > + old_slot = execute_attr_map_slot(relentry->attrmap, old_slot, > + MakeTupleTableSlot(RelationGetDescr(targetrel), > + &TTSOpsVirtual)); > > 1a. > IMO maybe it was more readable before when there was a separate 'tupdesc' > variable, instead of trying to squeeze too much into one statement. > > 1b. > Should you retain the old comments that said "/* Convert tuple if needed. */" Added. > ~~~ > > 2. > - if (old_slot) > - old_slot = execute_attr_map_slot(relentry->attrmap, > - old_slot, > - MakeTupleTableSlot(tupdesc, &TTSOpsVirtual)); > > The original code for REORDER_BUFFER_CHANGE_UPDATE was checking "if > (old_slot)" but that check seems no longer present. Is it OK? I think the logic is the same. > > ~~~ > > 3. > - /* > - * Send BEGIN if we haven't yet. > - * > - * We send the BEGIN message after ensuring that we will actually > - * send the change. This avoids sending a pair of BEGIN/COMMIT > - * messages for empty transactions. > - */ > > That original longer comment has been replaced with just "/* Send BEGIN if we > haven't yet */". Won't it be better to retain the more informative longer > comment? Added. > ~~~ > > 4. > + > +cleanup: > if (RelationIsValid(ancestor)) > { > RelationClose(ancestor); > > ~ > > Since you've introduced a new label 'cleanup:' then IMO you can remove that > old comment "/* Cleanup */". > Removed. Best Regards, Hou zj