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

On Thu, Mar 14, 2013 at 8:07 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Daniel Farina <daniel(at)heroku(dot)com> writes:
>> On Tue, Mar 12, 2013 at 11:51 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Yeah, watching the remote side's datestyle and intervalstyle and
>>> matching them (for both input and output) would probably work.
>
>> Alright, so I've been whacking at this and there's one interesting
>> thing to ask about: saving and restoring the local GUCs. There are a
>> bunch of things about GUCs besides their value that are maintained,
>> such as their 'source', so writing a little ad-hoc save/restore is not
>> going to do the right thing.
>
> Right, you should use NewGUCNestLevel/AtEOXact_GUC. Look at the fixes
> I committed in postgres_fdw a day or two ago for an example.

Thanks. Here's a patch. Here is the comments on top of the patch file inlined:

Similar in purpose to cc3f281ffb0a5d9b187e7a7b7de4a045809ff683, but
taking into account that a dblink caller may choose to cause arbitrary
changes to DateStyle and IntervalStyle. To handle this, it is
necessary to use PQparameterStatus before parsing any input, every
time. This is avoided in cases that do not include the two
problematic types treated -- interval and timestamptz -- by scanning
the TupleDesc's types first.

Although it has been suggested that extra_float_digits would need
similar treatment as IntervalStyle and DateStyle (as it's seen in
pg_dump), extra_float_digits is not an GUC_REPORT-ed GUC and is not
able to be checked in PQparameterStatus, and furthermore, the float4
and float8 parsers are not sensitive to the GUC anyway: both accept as
much precision as is provided in an unambiguous way.

>> So, I can add one more such use of AtEOXact_GUC for the dblink fix,
>> but would it also be appropriate to find some new names for the
>> concepts (instead of AtEOXact_GUC and isCommit) here to more
>> accurately express what's going on?
>
> Meh. I guess we could invent an "EndGUCNestLevel" that's a wrapper
> around AtEOXact_GUC, but I'm not that excited about it ...

Per your sentiment, I won't pursue that then.

Overall, the patch I think has reasonably thorough regression testing
(I tried to hit the several code paths whereby one would have to scan
TupleDescs, as well as a few other edge cases) and has some of the
obvious optimizations in place: it doesn't call PQparameterStatus more
than once per vulnerable type per TupleDesc scan, and avoids using the
parameter status procedure at all if there are no affected types.

The mechanisms may be overwrought though, since it was originally
intended to generalize to three types before I realized that
extra_float_digits is another kind of problem entirely, leaving just
IntervalStyle and DateStyle remaining, which perhaps could have been
treated even more specially to make the patch more terse.

I'll add it to the commitfest.

--
fdr