Re: postgres_fdw behaves oddly

Uh, where are we on this patch?

---------------------------------------------------------------------------

On Mon, Sep  8, 2014 at 09:30:32PM +0900, Etsuro Fujita wrote:
> (2014/09/02 18:55), Etsuro Fujita wrote:
> > (2014/09/01 20:15), Etsuro Fujita wrote:
> >> While working on [1], I've found that postgres_fdw behaves oddly:
> 
> I've revised the patch a bit further.  Please find attached a patch.
> 
> Thanks,
> 
> Best regards,
> Etsuro Fujita

> *** a/contrib/postgres_fdw/deparse.c
> --- b/contrib/postgres_fdw/deparse.c
> ***************
> *** 252,257 **** foreign_expr_walker(Node *node,
> --- 252,265 ----
>                   if (var->varno == glob_cxt->foreignrel->relid &&
>                       var->varlevelsup == 0)
>                   {
> +                     /*
> +                      * System columns can't be sent to remote.
> +                      *
> +                      * XXX: we should probably send ctid to remote.
> +                      */
> +                     if (var->varattno < 0)
> +                         return false;
> + 
>                       /* Var belongs to foreign table */
>                       collation = var->varcollid;
>                       state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
> ***************
> *** 1261,1266 **** deparseVar(Var *node, deparse_expr_cxt *context)
> --- 1269,1279 ----
>       if (node->varno == context->foreignrel->relid &&
>           node->varlevelsup == 0)
>       {
> +         /*
> +          * System columns shouldn't be examined here.
> +          */
> +         Assert(node->varattno >= 0);
> + 
>           /* Var belongs to foreign table */
>           deparseColumnRef(buf, node->varno, node->varattno, context->root);
>       }
> *** a/src/backend/optimizer/plan/createplan.c
> --- b/src/backend/optimizer/plan/createplan.c
> ***************
> *** 20,25 ****
> --- 20,26 ----
>   #include <math.h>
>   
>   #include "access/skey.h"
> + #include "access/sysattr.h"
>   #include "catalog/pg_class.h"
>   #include "foreign/fdwapi.h"
>   #include "miscadmin.h"
> ***************
> *** 1945,1950 **** create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
> --- 1946,1953 ----
>       RelOptInfo *rel = best_path->path.parent;
>       Index        scan_relid = rel->relid;
>       RangeTblEntry *rte;
> +     Bitmapset  *attrs_used = NULL;
> +     ListCell   *lc;
>       int            i;
>   
>       /* it should be a base rel... */
> ***************
> *** 1993,2008 **** create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
>        * bit of a kluge and might go away someday, so we intentionally leave it
>        * out of the API presented to FDWs.
>        */
>       scan_plan->fsSystemCol = false;
>       for (i = rel->min_attr; i < 0; i++)
>       {
> !         if (!bms_is_empty(rel->attr_needed[i - rel->min_attr]))
>           {
>               scan_plan->fsSystemCol = true;
>               break;
>           }
>       }
>   
>       return scan_plan;
>   }
>   
> --- 1996,2030 ----
>        * bit of a kluge and might go away someday, so we intentionally leave it
>        * out of the API presented to FDWs.
>        */
> + 
> +     /*
> +      * Add all the attributes needed for joins or final output.  Note: we must
> +      * look at reltargetlist, not the attr_needed data, because attr_needed
> +      * isn't computed for inheritance child rels.
> +      */
> +     pull_varattnos((Node *) rel->reltargetlist, rel->relid, &attrs_used);
> + 
> +     /* Add all the attributes used by restriction clauses. */
> +     foreach(lc, rel->baserestrictinfo)
> +     {
> +         RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
> + 
> +         pull_varattnos((Node *) rinfo->clause, rel->relid, &attrs_used);
> +     }
> + 
> +     /* Are any system columns requested from rel? */
>       scan_plan->fsSystemCol = false;
>       for (i = rel->min_attr; i < 0; i++)
>       {
> !         if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
>           {
>               scan_plan->fsSystemCol = true;
>               break;
>           }
>       }
>   
> +     bms_free(attrs_used);
> + 
>       return scan_plan;
>   }
>   

> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +