Re: postgres_fdw vs data formatting GUCs (was Re: [v9.3] writable foreign tables)

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