On Wed, Aug 18, 2021 at 1:27 PM Drouvot, Bertrand <bdrou...@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 <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. > > 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. -- With Regards, Amit Kapila.
