Tweaking ResolveNew's API

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: Tweaking ResolveNew's API
Date: 2012-11-08 19:17:59
Message-ID: 718.1352402279@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2012-11-08 19:29:16 Re: Proof of concept: auto updatable views [Review of Patch]
Previous Message Josh Berkus 2012-11-08 19:16:55 Re: auto_explain WAS: RFC: Timing Events