On Fri, Aug 13, 2021 at 11:47 AM Drouvot, Bertrand <bdrou...@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. -- With Regards, Amit Kapila.
