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

On Tue, Mar 19, 2013 at 6:10 PM, Daniel Farina <daniel@heroku.com> wrote:
> On Tue, Mar 19, 2013 at 3:06 PM, Daniel Farina <daniel@heroku.com> wrote:
>> On Tue, Mar 19, 2013 at 2:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Daniel Farina <daniel@heroku.com> writes:
>>>> On Tue, Mar 19, 2013 at 1:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>>> I'd be inclined to eat the cost of calling PQparameterStatus every time
>>>>> (which won't be that much) and instead try to optimize by avoiding the
>>>>> GUC-setting overhead if the current value matches the local setting.
>>>>> But even that might be premature optimization.  Did you do any
>>>>> performance testing to see if there was a problem worth avoiding?
>>>
>>>> Nope; should I invent a new way to do that, or would it be up to
>>>> commit standard based on inspection alone?  I'm okay either way, let
>>>> me know.
>>>
>>> Doesn't seem that hard to test: run a dblink query that pulls back a
>>> bunch of data under best-case conditions (ie, local not remote server),
>>> and time it with and without the proposed fix.  If the difference
>>> is marginal then it's not worth working hard to optimize.
>>
>> Okay, will do, and here's the shorter and less mechanically intensive
>> naive version that I think is the baseline: it doesn't try to optimize
>> out any GUC settings and sets up the GUCs before the two
>> materialization paths in dblink.
>
> The results.  Summary: seems like grabbing the GUC and strcmp-ing is
> worthwhile, but the amount of ping-ponging between processes adds some
> noise to the timing results: utilization is far short of 100% on
> either processor involved.  Attached is a cumulative diff of the new
> version, and also reproduced below are the changes to v2 that make up
> v3.

I added programming around various NULL returns reading GUCs in this
revision, v4.

The non-cumulative changes:

--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -3005,8 +3005,22 @@ applyRemoteGucs(remoteGucs *rgs)
  /*
  * Attempt to avoid GUC setting if the remote and local GUCs
  * already have the same value.
+ *
+ * NB: Must error if the GUC is not found.
  */
- localVal = GetConfigOption(gucName, true, true);
+ localVal = GetConfigOption(gucName, false, true);
+
+ if (remoteVal == NULL)
+ ereport(ERROR,
+ (errmsg("could not load parameter status of %s",
+ gucName)));
+
+ /*
+ * An error must have been raised by now if GUC values could
+ * not be loaded for any reason.
+ */
+ Assert(localVal != NULL);
+ Assert(remoteVal != NULL);

  if (strcmp(remoteVal, localVal) == 0)
  continue;

--
fdr