On Fri, Mar 22, 2013 at 12:29 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Daniel Farina <dan...@heroku.com> writes: >> This contains some edits to comments that referred to the obsolete and >> bogus TupleDesc scanning. No mechanical alterations. > > Applied with some substantial revisions. I didn't like where you'd put > the apply/restore calls, for one thing --- we need to wait to do the > applies until we have the PGresult in hand, else we might be applying > stale values of the remote's GUCs. Also, adding a call that could throw > errors right before materializeResult() won't do, because that would > result in leaking the PGresult on error.
Good catches. > The struct for state seemed a > bit of a mess too, given that you couldn't always initialize it in one > place. Yeah, I had to give that up when pushing things around, unless I wanted to push more state down. It used to be neater. > (In hindsight I could have left that alone given where I ended > up putting the calls, but it didn't seem to be providing any useful > isolation.) I studied your commit. Yeah, the idea I had was to try to avoid pushing down a loaded a value as a PGconn into the lower level helper functions, but perhaps that economy was false one after the modifications. Earlier versions used to push down the RemoteGucs struct instead of a full-blown conn to hint to the restricted purpose of that reference. By conceding to this pushdown I think the struct could have remained, as you said, but the difference to clarity is likely marginal. I thought I found a way to not have to widen the parameter list at all, so I preferred that one, but clearly it is wrong, w.r.t. leaks and the not up-to-date protocol state. Sorry you had to root around so much in there to get something you liked, but thanks for going through it. -- fdr -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
