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

Hi Amit,

On 8/10/21 1:59 PM, Drouvot, Bertrand wrote:
> Hi Amit,
>
> On 8/9/21 1:12 PM, Amit Kapila wrote:
>> On Mon, Aug 9, 2021 at 3:37 PM Drouvot, Bertrand
>> <bdrouvot(at)amazon(dot)com> wrote:
>>> Hi Amit,
>>>
>>> On 8/9/21 10:37 AM, Amit Kapila wrote:
>>>> On Fri, Jul 9, 2021 at 12:22 PM Drouvot, Bertrand
>>>> <bdrouvot(at)amazon(dot)com> wrote:
>>>>> Please find enclosed a patch proposal to:
>>>>>
>>>>> * Avoid the failed assertion on current master and generate the
>>>>> error message instead (should the code reach that stage).
>>>>> * Reset the toast_hash in case of relation rewrite with toast (so
>>>>> that the logical decoding in the above repro is working).
>>>>>
>>>> I think instead of resetting toast_hash for this case why don't we set
>>>> 'relrewrite' for toast tables as well during rewrite? If we do that
>>>> then we will simply skip assembling toast chunks for the toast table.
>>> Thanks for looking at it!
>>>
>>> I do agree, that would be even better than the current patch approach:
>>> I'll work on it.
>>>
>>>> In make_new_heap(), we are calling NewHeapCreateToastTable() to create
>>>> toast table where we can pass additional information (probably
>>>> 'toastid'), if required to set 'relrewrite'. Additionally, let's add a
>>>> test case if possible for this.
>>> + 1 for the test case, it will be added in the next version of the
>>> patch.
>>>
>> Thanks, please see, if you can prepare patches for the back-branches
>> as well.
>
> Please find attached the new version that:
>
> - sets "relwrewrite" for the toast.
>
> - contains a new test case.

The first version of the patch contained a change in
ReorderBufferToastReplace() (to put the call to
RelationIsValid(toast_rel) and display the error message when it is not
valid before the call to ReorderBufferChangeMemoryUpdate()).

That way we also avoid the failed assertion described in the first
message of this thread (but would report the error message instead).

Forgot to mention that I did not add this change in the new patch
version as I’m thinking it would be better to create another patch for
that purpose (as not really related to toast rewrite), what do you think?

Thanks
Bertrand