Thread: Tweaking ResolveNew's API
The submitted patch for auto-updatable views uses rewriteManip.c's
ResolveNew() function to replace Vars referencing the view with Vars
referencing the underlying table. That's mostly all right, except that
ResolveNew has some hard-wired choices about what it should do if a Var
to be replaced doesn't have any match in the replacement targetlist.
This should never occur in the auto-updatable view case, so really the
preferred behavior would be to throw an error, but that's not presently
one of the options.
What I'm thinking about doing is replacing ResolveNew's "event" argument
with a single-purpose enum listing the supported no-match actions,
along the lines of
enum { RESOLVENEW_CHANGE_VARNO, RESOLVENEW_SUBSTITUTE_NULL, RESOLVENEW_REPORT_ERROR}
A possible objection to this is that most C compilers wouldn't complain
if a call site is still trying to use the old convention of passing a
CmdType value. In the core code, there are only four call sites and
three are in rewriteHandler.c itself, so this isn't much of a problem
--- but if there's any third-party code such as FDWs that's trying to
make use of this function for querytree manipulation, there'd be a risk
of failing to notice the need to update the call.
One way to force a compile error would be to reorder the function's
argument list. But doing so in a way that would definitely get the
compiler's attention seems to require a fairly arbitrary choice of
argument order, and also it would add a little extra risk of not
making the code changes correctly. I'm inclined not to do that.
We have changed this function's API at least twice in the past, but each
time by adding new arguments, which will certainly draw a compile error;
so the lack of complaints about those changes doesn't necessarily prove
that nobody's using it outside core.
Thoughts, objections?
regards, tom lane
Tom Lane wrote: > A possible objection to this is that most C compilers wouldn't complain > if a call site is still trying to use the old convention of passing a > CmdType value. In the core code, there are only four call sites and > three are in rewriteHandler.c itself, so this isn't much of a problem > --- but if there's any third-party code such as FDWs that's trying to > make use of this function for querytree manipulation, there'd be a risk > of failing to notice the need to update the call. Failing to notice such changes is easy if the compiler doesn't even issue a warning, so *some* way to have old code fail (even better if it's a hard error and not just a warning) would be nice. I'm not sure I have useful suggestions on how to do it, though, just a +1 to doing it. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> A possible objection to this is that most C compilers wouldn't complain
>> if a call site is still trying to use the old convention of passing a
>> CmdType value. In the core code, there are only four call sites and
>> three are in rewriteHandler.c itself, so this isn't much of a problem
>> --- but if there's any third-party code such as FDWs that's trying to
>> make use of this function for querytree manipulation, there'd be a risk
>> of failing to notice the need to update the call.
> Failing to notice such changes is easy if the compiler doesn't even
> issue a warning, so *some* way to have old code fail (even better if
> it's a hard error and not just a warning) would be nice. I'm not sure I
> have useful suggestions on how to do it, though, just a +1 to doing it.
Actually, it occurs to me that there's a really easy way to get the
result: let's just rename the function. ResolveNew isn't an amazingly
mnemonic name anyway. How about ReplaceVarsFromTargetList?
regards, tom lane
On Thu, Nov 08, 2012 at 02:35:34PM -0500, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > Tom Lane wrote: > >> A possible objection to this is that most C compilers wouldn't complain > >> if a call site is still trying to use the old convention of passing a > >> CmdType value. In the core code, there are only four call sites and > >> three are in rewriteHandler.c itself, so this isn't much of a problem > >> --- but if there's any third-party code such as FDWs that's trying to > >> make use of this function for querytree manipulation, there'd be a risk > >> of failing to notice the need to update the call. > > > Failing to notice such changes is easy if the compiler doesn't even > > issue a warning, so *some* way to have old code fail (even better if > > it's a hard error and not just a warning) would be nice. I'm not sure I > > have useful suggestions on how to do it, though, just a +1 to doing it. > > Actually, it occurs to me that there's a really easy way to get the > result: let's just rename the function. ResolveNew isn't an amazingly > mnemonic name anyway. How about ReplaceVarsFromTargetList? +1 for descriptive names :) Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate