On Fri, Mar 22, 2013 at 12:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Daniel Farina <daniel@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
