From: | Julien Rouhaud <rjuju123(at)gmail(dot)com> |
---|---|
To: | Dmitry Dolgov <9erthalion6(at)gmail(dot)com> |
Cc: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Sergey Shinderuk <s(dot)shinderuk(at)postgrespro(dot)ru>, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, dean(dot)a(dot)rasheed(at)gmail(dot)com, er(at)xs4all(dot)nl, joel(at)compiler(dot)org, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Schema variables - new implementation for Postgres 15 |
Date: | 2023-08-12 12:00:55 |
Message-ID: | 20230812120055.z6fjl6znxaswyuia@jrouhaud |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
From | Date | Subject | |
---|---|---|---|
Next Message | Joe Conway | 2023-08-12 13:15:55 | Re: CREATE FUNCTION ... SEARCH { DEFAULT | SYSTEM | SESSION } |
Previous Message | Dmitry Dolgov | 2023-08-12 11:20:03 | Re: Schema variables - new implementation for Postgres 15 |