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

On Wed, Aug 18, 2021 at 1:27 PM Drouvot, Bertrand <bdrouvot(at)amazon(dot)com> wrote:
>
> Hi,
>
> On 8/13/21 11:17 AM, Amit Kapila wrote:
> > 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.
>
> I had a second thoughts on it and now think option b) is better.
>
> It would make the code clearer by using a new API and the extra cost of
> it (mainly search again for the pg_class tuple and update it) should be ok.
>

I agree especially because I am not very comfortable changing the
RenameRelationInternal() API in back branches. One minor comment:

+
+ /*
+ * Reset the relrewrite for the toast. We need to call
+ * CommandCounterIncrement() first to avoid the
+ * "tuple already updated by self" error. Indeed the exact same
+ * pg_class tuple has already been updated while
+ * calling RenameRelationInternal().
+ */
+ CommandCounterIncrement();

It would be better if we can write the above comment as "The
command-counter increment is required here as we are about to update
the tuple that is updated as part of RenameRelationInternal." or
something like that.

I would like to push and back-patch the proposed patch (after some
minor edits in the comments) unless someone thinks otherwise.

--
With Regards,
Amit Kapila.