RE: Simplify some codes in pgoutput

On Thursday, March 30, 2023 9:15 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Hou-san,
>
> I tried to compare the logic of patch v3-0001 versus the original HEAD code.
>
> IMO this patch logic is not exactly the same as before -- there are
> some subtle differences. I am not sure if these differences represent
> real problems or not.
>
> Below are all my review comments:

Thanks for the check and comments.

>
> ======
>
> 1.
> /* Switch relation if publishing via root. */
> if (relentry->publish_as_relid != RelationGetRelid(relation))
> {
> Assert(relation->rd_rel->relispartition);
> ancestor = RelationIdGetRelation(relentry->publish_as_relid);
> targetrel = ancestor;
> }
>
> ~
>
> The "switch relation if publishing via root" logic is now happening
> first, whereas the original code was doing this after the slot
> assignments. AFAIK it does not matter, it's just a small point of
> difference.

I also think it doesn't matter.

> ======
>
> 2.
> /* Convert tuple if needed. */
> if (relentry-> attrmap)
> {
> ...
> }
>
> The "Convert tuple if needed." logic looks the same, but when it is
> executed is NOT the same. It could be a problem.
>
> Previously, the conversion would only happen within the "Switch
> relation if publishing via root." condition. But the patch no longer
> has that extra condition -- now I think it attempts conversion every
> time regardless of "publishing via root".
>
> I would expect the "publish via root" case to be less common, so even
> if the current code works, by omitting that check won't this patch
> have an unnecessary performance hit due to the extra conversions?

No, the conversions won't happen in normal cases because "if (relentry-> attrmap)"
will pass only if we need to switch relation(publish via root).

> ~~
>
> 3.
> if (old_slot)
> old_slot =
> execute_attr_map_slot(relentry->attrmap,old_slot,MakeTupleTableSlot(tupde
> sc,
> &TTSOpsVirtual));
>
> ~
>
> The previous conversion code for UPDATE (shown above) was checking
> "if (old_slot)". Actually, I don't know why that check was even
> necessary before but it seems to have been accounting for a
> possibility that UPDATE might not have "oldtuple".

If the RI key wasn't updated, then it's possible the old tuple is null.

>
> But this combination (if indeed it was possible) is not handled
> anymore with the patch code because the old_slot is unconditionally
> assigned in the same block doing this conversion.

I think this case is handled by the generic old slot conversion in the patch.

> ======
>
> 4.
> AFAIK, the "if (change->data.tp.newtuple)" can only be true for INSERT
> or UPDATE, so the code would be better to include a sanity Assert.
>
> SUGGESTION
> if (change->data.tp.newtuple)
> {
> Assert(action == REORDER_BUFFER_CHANGE_INSERT || action ==
> REORDER_BUFFER_CHANGE_UPDATE);
> ...
> }
>
> ======
>
> 5.
> AFAIK, the "if (change->data.tp.oldtuple)" can only be true for UPDATE
> or DELETE, so the code would be better to include a sanity Assert.
>
> SUGGESTION
> if (change->data.tp.oldtuple)
> {
> Assert(action == REORDER_BUFFER_CHANGE_UPDATE || action ==
> REORDER_BUFFER_CHANGE_DELETE);
> ...
>

It might be fine but I am not sure if it's necessary to add this in this
patch as we don't have such assertion before.

>
> ======
>
> 6.
> I suggest moving the "change->data.tp.oldtuple" check before the
> "change->data.tp.newtuple" check. I don't think it makes any
> difference, but it seems more natural IMO to have old before new.

Changed.

Best Regards,
Hou zj