On Tue, Aug 10, 2021 at 5:30 PM Drouvot, Bertrand <bdrou...@amazon.com> wrote: > > Hi Amit, > > On 8/9/21 1:12 PM, Amit Kapila wrote: > > On Mon, Aug 9, 2021 at 3:37 PM Drouvot, Bertrand <bdrou...@amazon.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 <bdrou...@amazon.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. >
--- 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? -- With Regards, Amit Kapila.
