Re: review: FDW API

I wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Feb 18, 2011 at 10:47 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> The main downside of that is that relation relkinds would have
>>> to become fixed (because there would be no practical way of updating
>>> RTEs in stored rules), which means the "convert relation to view" hack
>>> would have to go away.

>> That actually sounds like a possible problem, because it's possible to
>> create views with circular dependencies using CORV, and they won't
>> dump-and-reload properly without that hack.

> Urgh. That's problematic, because even if we changed pg_dump (which
> would not be that hard I think), we'd still have to cope with dump files
> emitted by existing versions of pg_dump.

> [ thinks a bit ... ] But we can probably hack our way around that:
> teach the rule rewriter to update relkind in any RTE it brings in from a
> stored rule. We already do something similar in some other cases where
> a stored parsetree node contains information that could become obsolete.

I did a bit of poking around here, and came to the following
conclusions:

1. We don't want to add another RTEKind. RTE_RELATION basically has the
semantics of "anything with a pg_class OID", so it ought to include
foreign tables. Therefore the fix ought to be to add relkind to
RangeTblEntry. (BTW, so far as I can tell, RTE_SPECIAL is obsolete:
there are assorted switch cases that handle it, but no place that can
generate the value. I'm inclined to delete it while we are messing
with this.)

2. In the current code layout, to make sense of relkind you need to
#include catalog/pg_class.h where the values for relkind are #defined.
I dislike the idea of that being true for a field of such a widely-known
struct as RangeTblEntry. Accordingly, I suggest that we move those
values into parsenodes.h. (Perhaps we could convert them to an enum,
too, though still keeping the same ASCII values.)

3. We can have the rewriter update an RTE's stored value of relkind
during AcquireRewriteLocks: it opens the rel for each RTE_RELATION entry
anyway, so copying over the relkind is essentially free. While it's not
instantly obvious that that is "soon enough", I think that it is, since
up to the point of acquiring a lock there we can't assume that the rel
isn't being changed or dropped undeneath us --- that is, any earlier
test on an RTE's relkind might be testing just-obsoleted state anyway.

4. I had hoped that we might be able to get rid of some pre-existing
syscache lookups, but at least so far as the parse/plan/execute chain
is concerned, there don't seem to be any other places that need to
fetch a relkind based on just an RTE entry.

So point #4 is a bit discouraging, but other that, this seems like
a fairly straightforward exercise. I'm inclined to go ahead and do it,
unless there are objections.

regards, tom lane