RE: Simplify some codes in pgoutput - Mailing list pgsql-hackers

On Thursday, March 30, 2023 9:15 AM Peter Smith <smithpb2250@gmail.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