Re: Schema variables - new implementation for Postgres 15

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.