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.