Re: [bug] Logical Decoding of relation rewrite with toast does not reset toast_hash

On Fri, Aug 13, 2021 at 11:47 AM Drouvot, Bertrand <bdrouvot(at)amazon(dot)com> wrote:
>
> On 8/12/21 1:00 PM, Amit Kapila wrote:
> >>
> >> - sets "relwrewrite" for the toast.
> >>
> > --- a/src/backend/commands/tablecmds.c
> > +++ b/src/backend/commands/tablecmds.c
> > @@ -3861,6 +3861,10 @@ RenameRelationInternal(Oid myrelid, const char
> > *newrelname, bool is_internal, bo
> > */
> > namestrcpy(&(relform->relname), newrelname);
> >
> > + /* reset relrewrite for toast */
> > + if (relform->relkind == RELKIND_TOASTVALUE)
> > + relform->relrewrite = InvalidOid;
> > +
> >
> > I find this change quite ad-hoc. I think this API is quite generic to
> > make such a change. I see two ways for this (a) pass a new bool flag
> > (reset_toast_rewrite) in this API and then make this change, (b) in
> > the specific place where we need this, change relrewrite separately
> > via a new API.
> >
> > I would prefer (b) in the ideal case, but I understand it would be an
> > additional cost, so maybe (a) is also okay. What do you people think?
>
> I would prefer a) because:
>
> - b) would need to update the exact same tuple one more time (means
> doing more or less the same work: open relation, search for the tuple,
> update the tuple...)
>
> - a) would still give the ability for someone reading the code to
> understand where the relrewrite reset is needed (as opposed to the way
> the patch is currently written)
>
> - finish_heap_swap() with swap_toast_by_content set to false, is the
> only place where we currently need to reset explicitly relrewrite (so
> that we would have the new API produced by b) being called only at that
> place).
>
> - That means that b) would be only for code readability but at the price
> of extra cost.
>

Anybody else would like to weigh in? I think it is good if few others
also share their opinion as we need to backpatch this bug-fix.

--
With Regards,
Amit Kapila.