On Sat, Aug 12, 2023 at 01:20:03PM +0200, Dmitry Dolgov wrote: > > On Sat, Aug 12, 2023 at 09:28:19AM +0800, Julien Rouhaud wrote: > > On Fri, Aug 11, 2023 at 05:55:26PM +0200, Dmitry Dolgov wrote: > > > > > > Another confusing example was this one at the end of set_session_variable: > > > > > > + /* > > > + * XXX While unlikely, an error here is possible. It wouldn't > > > leak memory > > > + * as the allocated chunk has already been correctly assigned > > > to the > > > + * session variable, but would contradict this function > > > contract, which is > > > + * that this function should either succeed or leave the > > > current value > > > + * untouched. > > > + */ > > > + elog(DEBUG1, "session variable \"%s.%s\" (oid:%u) has new > > > value", > > > + > > > get_namespace_name(get_session_variable_namespace(svar->varid)), > > > + get_session_variable_name(svar->varid), > > > + svar->varid); > > > > > > It's not clear, which exactly error you're talking about, it's the last > > > instruction in the function. > > > > FTR I think I'm the one that changed that. The error I was talking about is > > elog() itself (in case of OOM for instance), or even one of the get_* call, > > if > > running with log_level <= DEBUG1. It's clearly really unlikely but still > > possible, thus this comment which also tries to explain why this elog() is > > not > > done earlier. > > I see, thanks for clarification. Absolutely nitpicking, but the crucial > "that's why this elog is not done earlier" is only assumed in the > comment between the lines, not stated out loud :)
Well, yes although to be fair the original version of this had a prior comment that was making it much more obvious: + /* + * No error should happen after this poiht, otherwise we could leak the + * newly allocated value if any. + */ (which would maybe have been better said "Nothing that can error out should be called after that point"). After quite a lot of patch revisions it now simply says: + /* We can overwrite old variable now. No error expected */ I agree that a bit more explanation is needed, and maybe also reminding that this is because all of that is done in a persistent memory context.
