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

Hi,

On 8/18/21 12:01 PM, Amit Kapila wrote:
> On Wed, Aug 18, 2021 at 1:27 PM Drouvot, Bertrand <bdrouvot@amazon.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@amazon.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.

Thanks!

I've updated the comment and prepared the back patch versions:

Please find attached:

v4-0001-toast-rewrite-master-branch.patch: to be applied on the master 
and REL_14_STABLE branches

v4-0001-toast-rewrite-13-stable-branch.patch: to be applied on the 
REL_13_STABLE and REL_12_STABLE branches

v4-0001-toast-rewrite-11-stable-branch.patch: to be applied on the 
REL_11_STABLE branch

I stopped the back patching here as the issue has been introduced in 
325f2ec555.

Thanks

Bertrand