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

Hi Hou-san,

I looked again at v4-0001.

On Thu, Mar 30, 2023 at 2:01 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Thursday, March 30, 2023 9:15 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
...
> >
> > 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).
>

OK.

> > ~~
> >
> > 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.

Yeah, I think you are right. Sorry, this was my mistake when reading v3.

>
> > ======
> >
> > 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.

The Asserts are just for sanity and self-documentation regarding what
actions can get into this logic. IMO including them does no harm,
rather it does some small amount of good, so why not do it?

You can't really use the fact they were not there before as a reason
to not add them now --  There were no Asserts in the original code
because this same logic was duplicated multiple times and was always
within obvious scope of a particular switch (action) case:

~

Apart from the question of the Asserts, I have no more review comments
for this patch.

(FYI - patch v4 applied cleanly and the regression tests and TAP
subscription tests all pass OK)

------
Kind Regards,
Peter Smith.