On Friday, March 17, 2023 11:49 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Mar 15, 2023 at 7:30 PM houzj.fnst@fujitsu.com
> <houzj.fnst@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