Re: review: FDW API

On 15.02.2011 23:00, Tom Lane wrote:
> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  writes:
>> On 15.02.2011 21:13, Tom Lane wrote:
>>> Hmm.  I don't have a problem with adding relkind to the planner's
>>> RelOptInfo, but it seems to me that if parse analysis needs to know
>>> this, you have put functionality into parse analysis that does not
>>> belong there.
>
>> Possibly. We throw the existing errors, for example if you try to do
>> "FOR UPDATE OF foo" where foo is a set-returning function, in
>> transformLockingClause(), so it seemed like the logical place to check
>> for foreign tables too.
>
>> Hmm, one approach would be to go ahead and create the RowMarkClauses for
>> all relations in the parse analysis phase, foreign or not, and throw the
>> error later, in preprocess_rowmarks().
>
> I think moving the error check downstream would be a good thing.

Ok, I tried moving the error checks to preprocess_rowmarks(). 
Unfortunately RelOptInfos haven't been built at that stage yet, so you 
still have to do the catalog lookup to get the relkind. That defeats the 
purpose.

We could delay the error checking further, but preprocess_rowmarks() 
would need to distinguish foreign tables anyway, so that it can mark 
them with ROW_MARK_COPY instead of ROW_MARK_REFERENCE.

> IIRC, at the moment we're basically duplicating the tests between parse
> analysis and the planner, but it's not clear what the value of that is.

There's duplicate logic in parse analysis and rewriter, to be precise. 
And then there's this one check in make_outerjoininfo:
>     /*>      * Presently the executor cannot support FOR UPDATE/SHARE marking of 
rels>      * appearing on the nullable side of an outer join. (It's somewhat 
unclear>      * what that would mean, anyway: what should we mark when a result 
row is>      * generated from no element of the nullable relation?)  So, complain if>      * any nullable rel is FOR
UPDATE/SHARE.>     *>      * You might be wondering why this test isn't made far upstream in the>      * parser.
It'sbecause the parser hasn't got enough info --- consider>      * FOR UPDATE applied to a view.  Only after rewriting
andflattening do>      * we know whether the view contains an outer join.>      *>      * We use the original
RowMarkClauselist here; the PlanRowMark list 
 
would>      * list everything.>      */>     foreach(l, root->parse->rowMarks)>     {>         RowMarkClause *rc =
(RowMarkClause*) lfirst(l);>>         if (bms_is_member(rc->rti, right_rels) ||>             (jointype == JOIN_FULL &&
bms_is_member(rc->rti,left_rels)))>             ereport(ERROR,>
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),>                     errmsg("SELECT FOR UPDATE/SHARE cannot be applied to the

nullable side of an outer join")));>     }

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com