Re: Simplify some codes in pgoutput

On Wed, Mar 15, 2023 at 7:30 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)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.
>
> Best Regards,
> Hou Zhijie

Hi Hou-san.

I had a quick look at the 0001 patch.

Here are some first 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. */"

~~~

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?

~~~

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?

~~~

4.
+
+cleanup:
if (RelationIsValid(ancestor))
{
RelationClose(ancestor);

~

Since you've introduced a new label 'cleanup:' then IMO you can remove
that old comment "/* Cleanup */".

------
Kind Regards,
Peter Smith.
Fujitsu Australia