Re: review: FDW API - Mailing list pgsql-hackers

From Tom Lane
Subject Re: review: FDW API
Date
Msg-id 8513.1298420885@sss.pgh.pa.us
Whole thread Raw
In response to Re: review: FDW API  (Tom Lane <tgl@sss.pgh.pa.us>)
List pgsql-hackers
I wrote:
> 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.

Applied, except I ended up not moving the RELKIND #defines as suggested
in point #2.  Those #defines are used by pg_dump, and having pg_dump.c
#include parsenodes.h seemed like a bad idea.
        regards, tom lane


pgsql-hackers by date:

Previous
From: "Kevin Grittner"
Date:
Subject: Re: SSI bug?
Next
From: Joachim Wieland
Date:
Subject: Re: Snapshot synchronization, again...