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

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.

Please find patch version V3 making use of a new API.

Thanks

Bertrand