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

On Wed, Mar 20, 2013 at 7:43 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Daniel Farina <daniel(at)heroku(dot)com> writes:
>> Okay, one more of those fridge-logic bugs. Sorry for the noise. v5.
>
>> A missing PG_RETHROW to get the properly finally-esque semantics:
>
>> --- a/contrib/dblink/dblink.c
>> +++ b/contrib/dblink/dblink.c
>> @@ -642,7 +642,10 @@ dblink_fetch(PG_FUNCTION_ARGS)
>> }
>> PG_CATCH();
>> {
>> + /* Pop any set GUCs, if necessary */
>> restoreLocalGucs(&rgs);
>> +
>> + PG_RE_THROW();
>> }
>> PG_END_TRY();
>
> Um ... you shouldn't need a PG_TRY for that at all. guc.c will take
> care of popping the values on transaction abort --- that's really rather
> the whole point of having that mechanism.

Hmm, well, merely raising the error doesn't reset the GUCs, so I was
rather thinking that this was a good idea to compose more neatly in
the case of nested exception processing, e.g.:

PG_TRY();
{
PG_TRY();
{
elog(NOTICE, "pre: %s",
GetConfigOption("DateStyle", false, true));
materializeResult(fcinfo, res);
}
PG_CATCH();
{
elog(NOTICE, "inner catch: %s",
GetConfigOption("DateStyle", false, true));
PG_RE_THROW();
}
PG_END_TRY();
}
PG_CATCH();
{
elog(NOTICE, "outer catch: %s",
GetConfigOption("DateStyle", false, true));
restoreLocalGucs(&rgs);
elog(NOTICE, "restored: %s",
GetConfigOption("DateStyle", false, true));
PG_RE_THROW();
}
PG_END_TRY();

I don't think this paranoia is made in other call sites for AtEOXact,
so included is a version that takes the same stance. This one shows up
best with the whitespace-insensitive option for git:

--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -634,21 +634,8 @@ dblink_fetch(PG_FUNCTION_ARGS)
* affect parsing and then un-set them afterwards.
*/
initRemoteGucs(&rgs, conn);
-
- PG_TRY();
- {
applyRemoteGucs(&rgs);
materializeResult(fcinfo, res);
- }
- PG_CATCH();
- {
- /* Pop any set GUCs, if necessary */
- restoreLocalGucs(&rgs);
-
- PG_RE_THROW();
- }
- PG_END_TRY();
-
restoreLocalGucs(&rgs);

return (Datum) 0;
@@ -823,9 +810,6 @@ dblink_record_internal(FunctionCallInfo fcinfo,
bool is_async)
if (freeconn)
PQfinish(conn);

- /* Pop any set GUCs, if necessary */
- restoreLocalGucs(&rgs);
-
PG_RE_THROW();
}
PG_END_TRY();

--
fdr