From bdc4d5c963fbf2653f7d152398b61a5beb6ae89f Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Mon, 8 Jun 2015 13:29:26 -0700 Subject: [PATCH 01/16] Added 'use_postgis' option to FDW TABLE options, and allow spatial quals to pass to remote if that option is set. --- contrib/postgres_fdw/deparse.c | 80 ++++++++++++++++++++++--- contrib/postgres_fdw/option.c | 5 +- contrib/postgres_fdw/postgres_fdw.c | 91 +++++++++++++++++------------ contrib/postgres_fdw/postgres_fdw.h | 39 +++++++++++++ 4 files changed, 171 insertions(+), 44 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 81cb2b447d84d..e57aaab9f264d 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -34,11 +34,15 @@ #include "postgres_fdw.h" +#include "access/genam.h" #include "access/heapam.h" #include "access/htup_details.h" #include "access/sysattr.h" #include "access/transam.h" +#include "catalog/dependency.h" +#include "catalog/indexing.h" #include "catalog/pg_collation.h" +#include "catalog/pg_depend.h" #include "catalog/pg_namespace.h" #include "catalog/pg_operator.h" #include "catalog/pg_proc.h" @@ -49,6 +53,7 @@ #include "optimizer/var.h" #include "parser/parsetree.h" #include "utils/builtins.h" +#include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/rel.h" #include "utils/syscache.h" @@ -138,6 +143,9 @@ static void printRemotePlaceholder(Oid paramtype, int32 paramtypmod, deparse_expr_cxt *context); +/* PostGIS */ +static bool is_in_postgis(Oid procid, PgFdwRelationInfo *fpinfo); + /* * Examine each qual clause in input_conds, and classify them into two groups, * which are returned as two lists: @@ -167,6 +175,7 @@ classifyConditions(PlannerInfo *root, } } + /* * Returns true if given expr is safe to evaluate on the foreign server. */ @@ -177,7 +186,7 @@ is_foreign_expr(PlannerInfo *root, { foreign_glob_cxt glob_cxt; foreign_loc_cxt loc_cxt; - + /* * Check that the expression consists of nodes that are safe to execute * remotely. @@ -207,6 +216,8 @@ is_foreign_expr(PlannerInfo *root, return true; } + + /* * Check if expression is safe to execute remotely, and return true if so. * @@ -229,6 +240,9 @@ foreign_expr_walker(Node *node, Oid collation; FDWCollateState state; + /* Access PostGIS metadata from fpinfo on baserel */ + PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *)(glob_cxt->foreignrel->fdw_private); + /* Need do nothing for empty subexpressions */ if (node == NULL) return true; @@ -361,7 +375,7 @@ foreign_expr_walker(Node *node, * can't be sent to remote because it might have incompatible * semantics on remote side. */ - if (!is_builtin(fe->funcid)) + if (!is_builtin(fe->funcid) && (!is_in_postgis(fe->funcid, fpinfo))) return false; /* @@ -407,7 +421,8 @@ foreign_expr_walker(Node *node, * (If the operator is, surely its underlying function is * too.) */ - if (!is_builtin(oe->opno)) + if ( (!is_builtin(oe->opno)) && + (!is_in_postgis(oe->opno, fpinfo)) ) return false; /* @@ -445,7 +460,7 @@ foreign_expr_walker(Node *node, /* * Again, only built-in operators can be sent to remote. */ - if (!is_builtin(oe->opno)) + if (!is_builtin(oe->opno) && (!is_in_postgis(oe->opno, fpinfo))) return false; /* @@ -591,7 +606,7 @@ foreign_expr_walker(Node *node, * If result type of given expression is not built-in, it can't be sent to * remote because it might have incompatible semantics on remote side. */ - if (check_type && !is_builtin(exprType(node))) + if (check_type && !is_builtin(exprType(node)) && (!is_in_postgis(exprType(node), fpinfo)) ) return false; /* @@ -643,6 +658,8 @@ foreign_expr_walker(Node *node, return true; } + + /* * Return true if given object is one of PostgreSQL's built-in objects. * @@ -668,6 +685,56 @@ is_builtin(Oid oid) } +/* + * Returns true if given operator is part of postgis extension + */ +static bool +is_in_postgis(Oid procnumber, PgFdwRelationInfo *fpinfo) +{ + static int nkeys = 1; + ScanKeyData key[nkeys]; + HeapTuple tup; + Relation depRel; + SysScanDesc scan; + int nresults = 0; + + /* Always return false if we aren't supposed to use PostGIS */ + if ( ! fpinfo->use_postgis ) + return false; + + /* We need this relation to scan */ + depRel = heap_open(DependRelationId, RowExclusiveLock); + + /* Scan the system dependency table for a all entries this operator */ + /* depends on, then iterate through and see if one of them */ + /* is the postgis extension */ + ScanKeyInit(&key[0], + Anum_pg_depend_objid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(procnumber)); + + scan = systable_beginscan(depRel, DependDependerIndexId, true, + NULL, nkeys, key); + + while (HeapTupleIsValid(tup = systable_getnext(scan))) + { + Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(tup); + + if ( foundDep->deptype == DEPENDENCY_EXTENSION && + foundDep->refobjid == fpinfo->postgis_oid ) + { + nresults++; + break; + } + } + + systable_endscan(scan); + relation_close(depRel, RowExclusiveLock); + + return nresults > 0; +} + + /* * Construct a simple SELECT statement that retrieves desired columns * of the specified foreign table, and append it to "buf". The output @@ -1404,8 +1471,7 @@ deparseConst(Const *node, deparse_expr_cxt *context) } if (needlabel) appendStringInfo(buf, "::%s", - format_type_with_typemod(node->consttype, - node->consttypmod)); + format_type_be_qualified(node->consttype)); } /* diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 7547ec28172e0..c4e05ae32255b 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -105,7 +105,8 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) * Validate option value, when we can do so without any context. */ if (strcmp(def->defname, "use_remote_estimate") == 0 || - strcmp(def->defname, "updatable") == 0) + strcmp(def->defname, "updatable") == 0 || + strcmp(def->defname, "used_postgis") == 0) { /* these accept only boolean values */ (void) defGetBoolean(def); @@ -153,6 +154,8 @@ InitPgFdwOptions(void) /* updatable is available on both server and table */ {"updatable", ForeignServerRelationId, false}, {"updatable", ForeignTableRelationId, false}, + /* use_remote_estimate is available on both server and table */ + {"use_postgis", ForeignTableRelationId, false}, {NULL, InvalidOid, false} }; diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 6da01e1d6f35f..51e752bec8687 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -17,6 +17,7 @@ #include "access/htup_details.h" #include "access/sysattr.h" #include "commands/defrem.h" +#include "commands/extension.h" #include "commands/explain.h" #include "commands/vacuum.h" #include "foreign/fdwapi.h" @@ -47,39 +48,6 @@ PG_MODULE_MAGIC; /* Default CPU cost to process 1 row (above and beyond cpu_tuple_cost). */ #define DEFAULT_FDW_TUPLE_COST 0.01 -/* - * FDW-specific planner information kept in RelOptInfo.fdw_private for a - * foreign table. This information is collected by postgresGetForeignRelSize. - */ -typedef struct PgFdwRelationInfo -{ - /* baserestrictinfo clauses, broken down into safe and unsafe subsets. */ - List *remote_conds; - List *local_conds; - - /* Bitmap of attr numbers we need to fetch from the remote server. */ - Bitmapset *attrs_used; - - /* Cost and selectivity of local_conds. */ - QualCost local_conds_cost; - Selectivity local_conds_sel; - - /* Estimated size and cost for a scan with baserestrictinfo quals. */ - double rows; - int width; - Cost startup_cost; - Cost total_cost; - - /* Options extracted from catalogs. */ - bool use_remote_estimate; - Cost fdw_startup_cost; - Cost fdw_tuple_cost; - - /* Cached catalog information. */ - ForeignTable *table; - ForeignServer *server; - UserMapping *user; /* only set in use_remote_estimate mode */ -} PgFdwRelationInfo; /* * Indexes of FDW-private information stored in fdw_private lists. @@ -422,10 +390,14 @@ postgresGetForeignRelSize(PlannerInfo *root, DefElem *def = (DefElem *) lfirst(lc); if (strcmp(def->defname, "use_remote_estimate") == 0) - { fpinfo->use_remote_estimate = defGetBoolean(def); - break; /* only need the one value */ - } + else if (strcmp(def->defname, "use_postgis") == 0) + fpinfo->use_postgis = defGetBoolean(def); + } + + if ( fpinfo->use_postgis ) + { + fpinfo->postgis_oid = get_extension_oid("postgis", true); } /* @@ -2994,3 +2966,50 @@ conversion_error_callback(void *arg) NameStr(tupdesc->attrs[errpos->cur_attno - 1]->attname), RelationGetRelationName(errpos->rel)); } + +/* + * postgresReadExtensions + * Test whether analyzing this foreign table is supported + */ +// static bool +// conn_has_postgis(PGconn *conn) +// { +// StringInfoData sql; +// PGresult *volatile res = NULL; +// bool has_postgis = false; +// +// /* +// * Construct command to get page count for relation. +// */ +// initStringInfo(&sql); +// +// appendStringInfoString(&sql, "SELECT oid, extname FROM pg_catalog.pg_extension WHERE extname = 'postgis'"); +// +// /* In what follows, do not risk leaking any PGresults. */ +// PG_TRY(); +// { +// res = PQexec(conn, sql.data); +// if (PQresultStatus(res) != PGRES_TUPLES_OK) +// pgfdw_report_error(ERROR, res, conn, false, sql.data); +// +// if (PQntuples(res) > 1 || PQnfields(res) != 2) +// elog(ERROR, "unexpected result from conn_get_postgis_oid query"); +// +// if (PQntuples(res) == 1) +// { +// has_postgis = true; +// } +// +// PQclear(res); +// res = NULL; +// } +// PG_CATCH(); +// { +// if (res) +// PQclear(res); +// PG_RE_THROW(); +// } +// PG_END_TRY(); +// +// return has_postgis; +// } diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index 3835ddb79ac61..49f92d03933fd 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -20,6 +20,45 @@ #include "libpq-fe.h" +/* + * FDW-specific planner information kept in RelOptInfo.fdw_private for a + * foreign table. This information is collected by postgresGetForeignRelSize. + */ +typedef struct PgFdwRelationInfo +{ + /* baserestrictinfo clauses, broken down into safe and unsafe subsets. */ + List *remote_conds; + List *local_conds; + + /* Bitmap of attr numbers we need to fetch from the remote server. */ + Bitmapset *attrs_used; + + /* Cost and selectivity of local_conds. */ + QualCost local_conds_cost; + Selectivity local_conds_sel; + + /* Estimated size and cost for a scan with baserestrictinfo quals. */ + double rows; + int width; + Cost startup_cost; + Cost total_cost; + + /* Options extracted from catalogs. */ + bool use_remote_estimate; + Cost fdw_startup_cost; + Cost fdw_tuple_cost; + + /* PostGIS metadata */ + bool use_postgis; + Oid postgis_oid; + + /* Cached catalog information. */ + ForeignTable *table; + ForeignServer *server; + UserMapping *user; /* only set in use_remote_estimate mode */ +} PgFdwRelationInfo; + + /* in postgres_fdw.c */ extern int set_transmission_modes(void); extern void reset_transmission_modes(int nestlevel); From 5f636f5e16b5df60f59b7976a0cc87407e8a0848 Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Tue, 9 Jun 2015 06:46:31 -0700 Subject: [PATCH 02/16] Add snapshot param Conflicts: contrib/postgres_fdw/deparse.c --- contrib/postgres_fdw/deparse.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index e57aaab9f264d..9b6d52e600be8 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -56,6 +56,7 @@ #include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/rel.h" +#include "utils/snapmgr.h" #include "utils/syscache.h" @@ -714,7 +715,7 @@ is_in_postgis(Oid procnumber, PgFdwRelationInfo *fpinfo) ObjectIdGetDatum(procnumber)); scan = systable_beginscan(depRel, DependDependerIndexId, true, - NULL, nkeys, key); + GetCatalogSnapshot(depRel->rd_id), nkeys, key); while (HeapTupleIsValid(tup = systable_getnext(scan))) { From fbfa660b03d1bce8611d6b1b9599c675976fb666 Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Wed, 15 Jul 2015 10:53:29 -0700 Subject: [PATCH 03/16] Generalize support for passing extension ops/functions through FDW --- contrib/postgres_fdw/deparse.c | 50 +++++++++++++++----------- contrib/postgres_fdw/option.c | 54 ++++++++++++++++++++++++++-- contrib/postgres_fdw/postgres_fdw.c | 56 ++--------------------------- contrib/postgres_fdw/postgres_fdw.h | 3 ++ 4 files changed, 86 insertions(+), 77 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 9b6d52e600be8..bbe3c9d08e70f 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -142,11 +142,9 @@ static void printRemoteParam(int paramindex, Oid paramtype, int32 paramtypmod, deparse_expr_cxt *context); static void printRemotePlaceholder(Oid paramtype, int32 paramtypmod, deparse_expr_cxt *context); +static bool is_in_extension(Oid procid, PgFdwRelationInfo *fpinfo); -/* PostGIS */ -static bool is_in_postgis(Oid procid, PgFdwRelationInfo *fpinfo); - /* * Examine each qual clause in input_conds, and classify them into two groups, * which are returned as two lists: @@ -241,7 +239,7 @@ foreign_expr_walker(Node *node, Oid collation; FDWCollateState state; - /* Access PostGIS metadata from fpinfo on baserel */ + /* Access extension metadata from fpinfo on baserel */ PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *)(glob_cxt->foreignrel->fdw_private); /* Need do nothing for empty subexpressions */ @@ -376,7 +374,7 @@ foreign_expr_walker(Node *node, * can't be sent to remote because it might have incompatible * semantics on remote side. */ - if (!is_builtin(fe->funcid) && (!is_in_postgis(fe->funcid, fpinfo))) + if (!is_builtin(fe->funcid) && (!is_in_extension(fe->funcid, fpinfo))) return false; /* @@ -422,8 +420,7 @@ foreign_expr_walker(Node *node, * (If the operator is, surely its underlying function is * too.) */ - if ( (!is_builtin(oe->opno)) && - (!is_in_postgis(oe->opno, fpinfo)) ) + if ( (!is_builtin(oe->opno)) && (!is_in_extension(oe->opno, fpinfo)) ) return false; /* @@ -461,7 +458,7 @@ foreign_expr_walker(Node *node, /* * Again, only built-in operators can be sent to remote. */ - if (!is_builtin(oe->opno) && (!is_in_postgis(oe->opno, fpinfo))) + if (!is_builtin(oe->opno) && (!is_in_extension(oe->opno, fpinfo))) return false; /* @@ -607,7 +604,7 @@ foreign_expr_walker(Node *node, * If result type of given expression is not built-in, it can't be sent to * remote because it might have incompatible semantics on remote side. */ - if (check_type && !is_builtin(exprType(node)) && (!is_in_postgis(exprType(node), fpinfo)) ) + if (check_type && !is_builtin(exprType(node)) && (!is_in_extension(exprType(node), fpinfo)) ) return false; /* @@ -687,10 +684,11 @@ is_builtin(Oid oid) /* - * Returns true if given operator is part of postgis extension + * Returns true if given operator/function is part of an extension declared in the + * server options. */ static bool -is_in_postgis(Oid procnumber, PgFdwRelationInfo *fpinfo) +is_in_extension(Oid procnumber, PgFdwRelationInfo *fpinfo) { static int nkeys = 1; ScanKeyData key[nkeys]; @@ -699,8 +697,8 @@ is_in_postgis(Oid procnumber, PgFdwRelationInfo *fpinfo) SysScanDesc scan; int nresults = 0; - /* Always return false if we aren't supposed to use PostGIS */ - if ( ! fpinfo->use_postgis ) + /* Always return false if we don't have any declared extensions */ + if ( ! fpinfo->extensions ) return false; /* We need this relation to scan */ @@ -708,7 +706,7 @@ is_in_postgis(Oid procnumber, PgFdwRelationInfo *fpinfo) /* Scan the system dependency table for a all entries this operator */ /* depends on, then iterate through and see if one of them */ - /* is the postgis extension */ + /* is a registered extension */ ScanKeyInit(&key[0], Anum_pg_depend_objid, BTEqualStrategyNumber, F_OIDEQ, @@ -720,22 +718,32 @@ is_in_postgis(Oid procnumber, PgFdwRelationInfo *fpinfo) while (HeapTupleIsValid(tup = systable_getnext(scan))) { Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(tup); - - if ( foundDep->deptype == DEPENDENCY_EXTENSION && - foundDep->refobjid == fpinfo->postgis_oid ) + + if ( foundDep->deptype == DEPENDENCY_EXTENSION ) { - nresults++; - break; + List *extlist = fpinfo->extensions; + ListCell *ext; + + foreach(ext, extlist) + { + Oid extension_oid = (Oid) lfirst(ext); + if ( foundDep->refobjid == extension_oid ) + { + nresults++; + } + } } + if ( nresults > 0 ) break; } systable_endscan(scan); relation_close(depRel, RowExclusiveLock); - - return nresults > 0; + + return nresults > 0; } + /* * Construct a simple SELECT statement that retrieves desired columns * of the specified foreign table, and append it to "buf". The output diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index c4e05ae32255b..29a3731ba885d 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -19,6 +19,8 @@ #include "catalog/pg_foreign_table.h" #include "catalog/pg_user_mapping.h" #include "commands/defrem.h" +#include "commands/extension.h" +#include "utils/builtins.h" /* @@ -105,8 +107,7 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) * Validate option value, when we can do so without any context. */ if (strcmp(def->defname, "use_remote_estimate") == 0 || - strcmp(def->defname, "updatable") == 0 || - strcmp(def->defname, "used_postgis") == 0) + strcmp(def->defname, "updatable") == 0 ) { /* these accept only boolean values */ (void) defGetBoolean(def); @@ -125,6 +126,10 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) errmsg("%s requires a non-negative numeric value", def->defname))); } + else if (strcmp(def->defname, "extensions") == 0) + { + extractExtensionList(defGetString(def), NULL); + } } PG_RETURN_VOID(); @@ -155,7 +160,7 @@ InitPgFdwOptions(void) {"updatable", ForeignServerRelationId, false}, {"updatable", ForeignTableRelationId, false}, /* use_remote_estimate is available on both server and table */ - {"use_postgis", ForeignTableRelationId, false}, + {"extensions", ForeignServerRelationId, false}, {NULL, InvalidOid, false} }; @@ -296,3 +301,46 @@ ExtractConnectionOptions(List *defelems, const char **keywords, } return i; } + + +bool +extractExtensionList(char *extensionString, List **extensionOids) +{ + List *extlist; + ListCell *l; + + if ( ! SplitIdentifierString(extensionString, ',', &extlist) ) + { + list_free(extlist); + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("unable to parse extension list '%s'", + extensionString))); + } + + if ( extensionOids ) + *extensionOids = NIL; + + foreach(l, extlist) + { + char *extension_name = (char *) lfirst(l); + Oid extension_oid = get_extension_oid(extension_name, true); + if ( extension_oid == InvalidOid ) + { + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("the '%s' extension must be installed locally before it can be used on a remote server", + extension_name))); + } + else + { + if ( extensionOids ) + { + *extensionOids = lappend_oid(*extensionOids, extension_oid); + } + } + } + + list_free(extlist); + return true; +} diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index a547479c15737..2b1c24024bdf3 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -384,6 +384,8 @@ postgresGetForeignRelSize(PlannerInfo *root, fpinfo->fdw_startup_cost = strtod(defGetString(def), NULL); else if (strcmp(def->defname, "fdw_tuple_cost") == 0) fpinfo->fdw_tuple_cost = strtod(defGetString(def), NULL); + else if (strcmp(def->defname, "extensions") == 0) + extractExtensionList(defGetString(def), &(fpinfo->extensions)); } foreach(lc, fpinfo->table->options) { @@ -391,13 +393,6 @@ postgresGetForeignRelSize(PlannerInfo *root, if (strcmp(def->defname, "use_remote_estimate") == 0) fpinfo->use_remote_estimate = defGetBoolean(def); - else if (strcmp(def->defname, "use_postgis") == 0) - fpinfo->use_postgis = defGetBoolean(def); - } - - if ( fpinfo->use_postgis ) - { - fpinfo->postgis_oid = get_extension_oid("postgis", true); } /* @@ -2967,49 +2962,4 @@ conversion_error_callback(void *arg) RelationGetRelationName(errpos->rel)); } -/* - * postgresReadExtensions - * Test whether analyzing this foreign table is supported - */ -// static bool -// conn_has_postgis(PGconn *conn) -// { -// StringInfoData sql; -// PGresult *volatile res = NULL; -// bool has_postgis = false; -// -// /* -// * Construct command to get page count for relation. -// */ -// initStringInfo(&sql); -// -// appendStringInfoString(&sql, "SELECT oid, extname FROM pg_catalog.pg_extension WHERE extname = 'postgis'"); -// -// /* In what follows, do not risk leaking any PGresults. */ -// PG_TRY(); -// { -// res = PQexec(conn, sql.data); -// if (PQresultStatus(res) != PGRES_TUPLES_OK) -// pgfdw_report_error(ERROR, res, conn, false, sql.data); -// -// if (PQntuples(res) > 1 || PQnfields(res) != 2) -// elog(ERROR, "unexpected result from conn_get_postgis_oid query"); -// -// if (PQntuples(res) == 1) -// { -// has_postgis = true; -// } -// -// PQclear(res); -// res = NULL; -// } -// PG_CATCH(); -// { -// if (res) -// PQclear(res); -// PG_RE_THROW(); -// } -// PG_END_TRY(); -// -// return has_postgis; -// } + diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index 49f92d03933fd..d27ed37e7dabc 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -49,6 +49,7 @@ typedef struct PgFdwRelationInfo Cost fdw_tuple_cost; /* PostGIS metadata */ + List *extensions; bool use_postgis; Oid postgis_oid; @@ -76,6 +77,8 @@ extern void pgfdw_report_error(int elevel, PGresult *res, PGconn *conn, extern int ExtractConnectionOptions(List *defelems, const char **keywords, const char **values); +extern bool extractExtensionList(char *extensionString, + List **extensionOids); /* in deparse.c */ extern void classifyConditions(PlannerInfo *root, From 75217514202529920cd74fd23afe760f584ac27e Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Wed, 15 Jul 2015 11:15:05 -0700 Subject: [PATCH 04/16] Add doco for the 'extensions' option on CREATE SERVER --- doc/src/sgml/postgres-fdw.sgml | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 14b12e37dc6cc..7b3d8c736992f 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -373,6 +373,37 @@ foreign tables, see . + + + Extension Options + + + By default only built-in operators and functions will be sent from the + local to the foreign server. This may be overridden using the following option: + + + + + + extensions + + + This option allows you to declare what extensions you expect are + installed on the foreign server, using a comma-separated list of + extension names. The extensions are also expected to be installed + on the local server too. + + +CREATE SERVER foreign_server + FOREIGN DATA WRAPPER postgres_fdw + OPTIONS (host '127.0.0.1', port '5432', dbname 'my_db', extensions 'cube, seg'); + + + + + + + From fbbec9d56fa4a1cfb68daa531e347430db86f2e5 Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Thu, 16 Jul 2015 09:16:59 -0700 Subject: [PATCH 05/16] Patch review improvements --- contrib/postgres_fdw/deparse.c | 19 ++++++------------- contrib/postgres_fdw/option.c | 4 +--- contrib/postgres_fdw/postgres_fdw.c | 5 +++-- contrib/postgres_fdw/postgres_fdw.h | 5 +---- 4 files changed, 11 insertions(+), 22 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index bbe3c9d08e70f..1a25e9daac1d3 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -174,7 +174,6 @@ classifyConditions(PlannerInfo *root, } } - /* * Returns true if given expr is safe to evaluate on the foreign server. */ @@ -185,7 +184,7 @@ is_foreign_expr(PlannerInfo *root, { foreign_glob_cxt glob_cxt; foreign_loc_cxt loc_cxt; - + /* * Check that the expression consists of nodes that are safe to execute * remotely. @@ -215,8 +214,6 @@ is_foreign_expr(PlannerInfo *root, return true; } - - /* * Check if expression is safe to execute remotely, and return true if so. * @@ -374,7 +371,7 @@ foreign_expr_walker(Node *node, * can't be sent to remote because it might have incompatible * semantics on remote side. */ - if (!is_builtin(fe->funcid) && (!is_in_extension(fe->funcid, fpinfo))) + if (!is_builtin(fe->funcid) && !is_in_extension(fe->funcid, fpinfo)) return false; /* @@ -420,7 +417,7 @@ foreign_expr_walker(Node *node, * (If the operator is, surely its underlying function is * too.) */ - if ( (!is_builtin(oe->opno)) && (!is_in_extension(oe->opno, fpinfo)) ) + if (!is_builtin(oe->opno) && !is_in_extension(oe->opno, fpinfo)) return false; /* @@ -458,7 +455,7 @@ foreign_expr_walker(Node *node, /* * Again, only built-in operators can be sent to remote. */ - if (!is_builtin(oe->opno) && (!is_in_extension(oe->opno, fpinfo))) + if (!is_builtin(oe->opno) && !is_in_extension(oe->opno, fpinfo)) return false; /* @@ -604,7 +601,7 @@ foreign_expr_walker(Node *node, * If result type of given expression is not built-in, it can't be sent to * remote because it might have incompatible semantics on remote side. */ - if (check_type && !is_builtin(exprType(node)) && (!is_in_extension(exprType(node), fpinfo)) ) + if (check_type && !is_builtin(exprType(node)) && !is_in_extension(exprType(node), fpinfo)) return false; /* @@ -656,8 +653,6 @@ foreign_expr_walker(Node *node, return true; } - - /* * Return true if given object is one of PostgreSQL's built-in objects. * @@ -742,8 +737,6 @@ is_in_extension(Oid procnumber, PgFdwRelationInfo *fpinfo) return nresults > 0; } - - /* * Construct a simple SELECT statement that retrieves desired columns * of the specified foreign table, and append it to "buf". The output @@ -1480,7 +1473,7 @@ deparseConst(Const *node, deparse_expr_cxt *context) } if (needlabel) appendStringInfo(buf, "::%s", - format_type_be_qualified(node->consttype)); + format_type_be_qualified(node->consttype)); } /* diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 29a3731ba885d..03c4be82c54dd 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -107,7 +107,7 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) * Validate option value, when we can do so without any context. */ if (strcmp(def->defname, "use_remote_estimate") == 0 || - strcmp(def->defname, "updatable") == 0 ) + strcmp(def->defname, "updatable") == 0) { /* these accept only boolean values */ (void) defGetBoolean(def); @@ -159,7 +159,6 @@ InitPgFdwOptions(void) /* updatable is available on both server and table */ {"updatable", ForeignServerRelationId, false}, {"updatable", ForeignTableRelationId, false}, - /* use_remote_estimate is available on both server and table */ {"extensions", ForeignServerRelationId, false}, {NULL, InvalidOid, false} }; @@ -302,7 +301,6 @@ ExtractConnectionOptions(List *defelems, const char **keywords, return i; } - bool extractExtensionList(char *extensionString, List **extensionOids) { diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 2b1c24024bdf3..3cc9b2f55c030 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -392,7 +392,10 @@ postgresGetForeignRelSize(PlannerInfo *root, DefElem *def = (DefElem *) lfirst(lc); if (strcmp(def->defname, "use_remote_estimate") == 0) + { fpinfo->use_remote_estimate = defGetBoolean(def); + break; /* only need the one value */ + } } /* @@ -2961,5 +2964,3 @@ conversion_error_callback(void *arg) NameStr(tupdesc->attrs[errpos->cur_attno - 1]->attname), RelationGetRelationName(errpos->rel)); } - - diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index d27ed37e7dabc..45731bde83c77 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -48,10 +48,8 @@ typedef struct PgFdwRelationInfo Cost fdw_startup_cost; Cost fdw_tuple_cost; - /* PostGIS metadata */ + /* Optional extensions to support (list of oid) */ List *extensions; - bool use_postgis; - Oid postgis_oid; /* Cached catalog information. */ ForeignTable *table; @@ -59,7 +57,6 @@ typedef struct PgFdwRelationInfo UserMapping *user; /* only set in use_remote_estimate mode */ } PgFdwRelationInfo; - /* in postgres_fdw.c */ extern int set_transmission_modes(void); extern void reset_transmission_modes(int nestlevel); From c91ac1161fffdc37e763eb3516cfcb4106706355 Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Thu, 16 Jul 2015 12:18:36 -0700 Subject: [PATCH 06/16] Use double quotes in error per PgSQL style guide --- contrib/postgres_fdw/option.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 03c4be82c54dd..2f78e22914b33 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -312,7 +312,7 @@ extractExtensionList(char *extensionString, List **extensionOids) list_free(extlist); ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("unable to parse extension list '%s'", + errmsg("unable to parse extension list \"%s\"", extensionString))); } @@ -327,7 +327,7 @@ extractExtensionList(char *extensionString, List **extensionOids) { ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("the '%s' extension must be installed locally before it can be used on a remote server", + errmsg("the \"%s\" extension must be installed locally before it can be used on a remote server", extension_name))); } else From 341553bce28d876fc64ccb56aa585ef164891979 Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Tue, 21 Jul 2015 07:15:15 -0700 Subject: [PATCH 07/16] Add 'extensions' option at FDW level as well --- contrib/postgres_fdw/option.c | 23 +++++++++++++++-------- contrib/postgres_fdw/postgres_fdw.c | 9 +++++++++ contrib/postgres_fdw/postgres_fdw.h | 1 + 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 2f78e22914b33..1abe8450b8371 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -15,6 +15,7 @@ #include "postgres_fdw.h" #include "access/reloptions.h" +#include "catalog/pg_foreign_data_wrapper.h" #include "catalog/pg_foreign_server.h" #include "catalog/pg_foreign_table.h" #include "catalog/pg_user_mapping.h" @@ -159,7 +160,9 @@ InitPgFdwOptions(void) /* updatable is available on both server and table */ {"updatable", ForeignServerRelationId, false}, {"updatable", ForeignTableRelationId, false}, + /* extensions is available on both wrapper and server */ {"extensions", ForeignServerRelationId, false}, + {"extensions", ForeignDataWrapperRelationId, false}, {NULL, InvalidOid, false} }; @@ -305,7 +308,7 @@ bool extractExtensionList(char *extensionString, List **extensionOids) { List *extlist; - ListCell *l; + ListCell *l, *o; if ( ! SplitIdentifierString(extensionString, ',', &extlist) ) { @@ -316,12 +319,9 @@ extractExtensionList(char *extensionString, List **extensionOids) extensionString))); } - if ( extensionOids ) - *extensionOids = NIL; - foreach(l, extlist) { - char *extension_name = (char *) lfirst(l); + const char *extension_name = (const char *) lfirst(l); Oid extension_oid = get_extension_oid(extension_name, true); if ( extension_oid == InvalidOid ) { @@ -330,12 +330,19 @@ extractExtensionList(char *extensionString, List **extensionOids) errmsg("the \"%s\" extension must be installed locally before it can be used on a remote server", extension_name))); } - else + else if ( extensionOids ) { - if ( extensionOids ) + bool found = false; + /* Only add this extension Oid to the list */ + /* if we don't already have it */ + foreach(o, *extensionOids) { - *extensionOids = lappend_oid(*extensionOids, extension_oid); + Oid oid = (Oid) lfirst(o); + if ( oid == extension_oid ) + found = true; } + if ( ! found ) + *extensionOids = lappend_oid(*extensionOids, extension_oid); } } diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 3cc9b2f55c030..05ca1e0ebc3d8 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -365,6 +365,7 @@ postgresGetForeignRelSize(PlannerInfo *root, /* Look up foreign-table catalog info. */ fpinfo->table = GetForeignTable(foreigntableid); fpinfo->server = GetForeignServer(fpinfo->table->serverid); + fpinfo->wrapper = GetForeignDataWrapper(fpinfo->server->fdwid); /* * Extract user-settable option values. Note that per-table setting of @@ -373,7 +374,15 @@ postgresGetForeignRelSize(PlannerInfo *root, fpinfo->use_remote_estimate = false; fpinfo->fdw_startup_cost = DEFAULT_FDW_STARTUP_COST; fpinfo->fdw_tuple_cost = DEFAULT_FDW_TUPLE_COST; + fpinfo->extensions = NIL; + foreach(lc, fpinfo->wrapper->options) + { + DefElem *def = (DefElem *) lfirst(lc); + + if (strcmp(def->defname, "extensions") == 0) + extractExtensionList(defGetString(def), &(fpinfo->extensions)); + } foreach(lc, fpinfo->server->options) { DefElem *def = (DefElem *) lfirst(lc); diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index 45731bde83c77..07eecdda909f8 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -52,6 +52,7 @@ typedef struct PgFdwRelationInfo List *extensions; /* Cached catalog information. */ + ForeignDataWrapper *wrapper; ForeignTable *table; ForeignServer *server; UserMapping *user; /* only set in use_remote_estimate mode */ From 51b3bf32521f836aa29b71eb8ddbb80fde40725a Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Tue, 21 Jul 2015 13:10:02 -0700 Subject: [PATCH 08/16] Add cache for holding info about what functions can be shipped to remote server, and organize changes into separate file. --- contrib/postgres_fdw/Makefile | 2 +- contrib/postgres_fdw/deparse.c | 77 +--------- contrib/postgres_fdw/option.c | 48 ------ contrib/postgres_fdw/postgres_fdw.c | 1 - contrib/postgres_fdw/postgres_fdw.h | 7 +- contrib/postgres_fdw/shippable.c | 231 ++++++++++++++++++++++++++++ 6 files changed, 242 insertions(+), 124 deletions(-) create mode 100644 contrib/postgres_fdw/shippable.c diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile index d2b98e10f3a5e..354331247a822 100644 --- a/contrib/postgres_fdw/Makefile +++ b/contrib/postgres_fdw/Makefile @@ -1,7 +1,7 @@ # contrib/postgres_fdw/Makefile MODULE_big = postgres_fdw -OBJS = postgres_fdw.o option.o deparse.o connection.o $(WIN32RES) +OBJS = postgres_fdw.o option.o deparse.o connection.o shippable.o $(WIN32RES) PGFILEDESC = "postgres_fdw - foreign data wrapper for PostgreSQL" PG_CPPFLAGS = -I$(libpq_srcdir) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 1a25e9daac1d3..904cc94243aaf 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -34,15 +34,11 @@ #include "postgres_fdw.h" -#include "access/genam.h" #include "access/heapam.h" #include "access/htup_details.h" #include "access/sysattr.h" #include "access/transam.h" -#include "catalog/dependency.h" -#include "catalog/indexing.h" #include "catalog/pg_collation.h" -#include "catalog/pg_depend.h" #include "catalog/pg_namespace.h" #include "catalog/pg_operator.h" #include "catalog/pg_proc.h" @@ -53,10 +49,8 @@ #include "optimizer/var.h" #include "parser/parsetree.h" #include "utils/builtins.h" -#include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/rel.h" -#include "utils/snapmgr.h" #include "utils/syscache.h" @@ -142,7 +136,6 @@ static void printRemoteParam(int paramindex, Oid paramtype, int32 paramtypmod, deparse_expr_cxt *context); static void printRemotePlaceholder(Oid paramtype, int32 paramtypmod, deparse_expr_cxt *context); -static bool is_in_extension(Oid procid, PgFdwRelationInfo *fpinfo); /* @@ -371,7 +364,7 @@ foreign_expr_walker(Node *node, * can't be sent to remote because it might have incompatible * semantics on remote side. */ - if (!is_builtin(fe->funcid) && !is_in_extension(fe->funcid, fpinfo)) + if (!is_builtin(fe->funcid) && !is_shippable(fe->funcid, fpinfo)) return false; /* @@ -417,7 +410,7 @@ foreign_expr_walker(Node *node, * (If the operator is, surely its underlying function is * too.) */ - if (!is_builtin(oe->opno) && !is_in_extension(oe->opno, fpinfo)) + if (!is_builtin(oe->opno) && !is_shippable(oe->opno, fpinfo)) return false; /* @@ -455,7 +448,7 @@ foreign_expr_walker(Node *node, /* * Again, only built-in operators can be sent to remote. */ - if (!is_builtin(oe->opno) && !is_in_extension(oe->opno, fpinfo)) + if (!is_builtin(oe->opno) && !is_shippable(oe->opno, fpinfo)) return false; /* @@ -601,7 +594,7 @@ foreign_expr_walker(Node *node, * If result type of given expression is not built-in, it can't be sent to * remote because it might have incompatible semantics on remote side. */ - if (check_type && !is_builtin(exprType(node)) && !is_in_extension(exprType(node), fpinfo)) + if (check_type && !is_builtin(exprType(node)) && !is_shippable(exprType(node), fpinfo)) return false; /* @@ -677,66 +670,6 @@ is_builtin(Oid oid) return (oid < FirstBootstrapObjectId); } - -/* - * Returns true if given operator/function is part of an extension declared in the - * server options. - */ -static bool -is_in_extension(Oid procnumber, PgFdwRelationInfo *fpinfo) -{ - static int nkeys = 1; - ScanKeyData key[nkeys]; - HeapTuple tup; - Relation depRel; - SysScanDesc scan; - int nresults = 0; - - /* Always return false if we don't have any declared extensions */ - if ( ! fpinfo->extensions ) - return false; - - /* We need this relation to scan */ - depRel = heap_open(DependRelationId, RowExclusiveLock); - - /* Scan the system dependency table for a all entries this operator */ - /* depends on, then iterate through and see if one of them */ - /* is a registered extension */ - ScanKeyInit(&key[0], - Anum_pg_depend_objid, - BTEqualStrategyNumber, F_OIDEQ, - ObjectIdGetDatum(procnumber)); - - scan = systable_beginscan(depRel, DependDependerIndexId, true, - GetCatalogSnapshot(depRel->rd_id), nkeys, key); - - while (HeapTupleIsValid(tup = systable_getnext(scan))) - { - Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(tup); - - if ( foundDep->deptype == DEPENDENCY_EXTENSION ) - { - List *extlist = fpinfo->extensions; - ListCell *ext; - - foreach(ext, extlist) - { - Oid extension_oid = (Oid) lfirst(ext); - if ( foundDep->refobjid == extension_oid ) - { - nresults++; - } - } - } - if ( nresults > 0 ) break; - } - - systable_endscan(scan); - relation_close(depRel, RowExclusiveLock); - - return nresults > 0; -} - /* * Construct a simple SELECT statement that retrieves desired columns * of the specified foreign table, and append it to "buf". The output @@ -1473,7 +1406,7 @@ deparseConst(Const *node, deparse_expr_cxt *context) } if (needlabel) appendStringInfo(buf, "::%s", - format_type_be_qualified(node->consttype)); + format_type_be_qualified(node->consttype)); } /* diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 1abe8450b8371..9aeaf1a0967b2 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -20,8 +20,6 @@ #include "catalog/pg_foreign_table.h" #include "catalog/pg_user_mapping.h" #include "commands/defrem.h" -#include "commands/extension.h" -#include "utils/builtins.h" /* @@ -303,49 +301,3 @@ ExtractConnectionOptions(List *defelems, const char **keywords, } return i; } - -bool -extractExtensionList(char *extensionString, List **extensionOids) -{ - List *extlist; - ListCell *l, *o; - - if ( ! SplitIdentifierString(extensionString, ',', &extlist) ) - { - list_free(extlist); - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("unable to parse extension list \"%s\"", - extensionString))); - } - - foreach(l, extlist) - { - const char *extension_name = (const char *) lfirst(l); - Oid extension_oid = get_extension_oid(extension_name, true); - if ( extension_oid == InvalidOid ) - { - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("the \"%s\" extension must be installed locally before it can be used on a remote server", - extension_name))); - } - else if ( extensionOids ) - { - bool found = false; - /* Only add this extension Oid to the list */ - /* if we don't already have it */ - foreach(o, *extensionOids) - { - Oid oid = (Oid) lfirst(o); - if ( oid == extension_oid ) - found = true; - } - if ( ! found ) - *extensionOids = lappend_oid(*extensionOids, extension_oid); - } - } - - list_free(extlist); - return true; -} diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 05ca1e0ebc3d8..42d7e25d47921 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -17,7 +17,6 @@ #include "access/htup_details.h" #include "access/sysattr.h" #include "commands/defrem.h" -#include "commands/extension.h" #include "commands/explain.h" #include "commands/vacuum.h" #include "foreign/fdwapi.h" diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index 07eecdda909f8..8e16ee92952bf 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -47,7 +47,7 @@ typedef struct PgFdwRelationInfo bool use_remote_estimate; Cost fdw_startup_cost; Cost fdw_tuple_cost; - + /* Optional extensions to support (list of oid) */ List *extensions; @@ -75,8 +75,11 @@ extern void pgfdw_report_error(int elevel, PGresult *res, PGconn *conn, extern int ExtractConnectionOptions(List *defelems, const char **keywords, const char **values); + +/* in shippable.c */ extern bool extractExtensionList(char *extensionString, - List **extensionOids); + List **extensionOids); +extern bool is_shippable(Oid procnumber, PgFdwRelationInfo *fpinfo); /* in deparse.c */ extern void classifyConditions(PlannerInfo *root, diff --git a/contrib/postgres_fdw/shippable.c b/contrib/postgres_fdw/shippable.c new file mode 100644 index 0000000000000..d490306675fab --- /dev/null +++ b/contrib/postgres_fdw/shippable.c @@ -0,0 +1,231 @@ +/*------------------------------------------------------------------------- + * + * nonbuiltin.c + * Non-built-in procs cache management and utilities. + * + * Non-built-in functions can only be passed to remote servers when + * properly declared. The list of declared "safe" functions is cached + * and refreshed here. + * + * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group + * + * IDENTIFICATION + * contrib/postgres_fdw/nonbuiltin.c + * + *------------------------------------------------------------------------- + */ + +#include "postgres.h" + +#include "postgres_fdw.h" + +#include "access/genam.h" +#include "access/heapam.h" +#include "access/htup_details.h" +#include "catalog/dependency.h" +#include "catalog/indexing.h" +#include "catalog/pg_depend.h" +#include "commands/extension.h" +#include "utils/builtins.h" +#include "utils/fmgroids.h" +#include "utils/hsearch.h" +#include "utils/rel.h" +#include "utils/snapmgr.h" + +/* Hash table for informations about remote procs we'll call */ +static HTAB *ShippableCacheHash = NULL; + +/* procid is the lookup key, must appear first */ +typedef struct +{ + Oid procid; +} ShippableCacheKey; + +typedef struct +{ + ShippableCacheKey key; /* lookup key - must be first */ + bool shippable; /* extension the proc appears within, or InvalidOid if none */ +} ShippableCacheEntry; + +/* + * InitializeShippableCache + * Initialize the cache of functions we can ship to remote server. + */ +static void +InitializeShippableCache(void) +{ + HASHCTL ctl; + + /* Initialize the hash table. */ + MemSet(&ctl, 0, sizeof(ctl)); + ctl.keysize = sizeof(ShippableCacheKey); + ctl.entrysize = sizeof(ShippableCacheEntry); + ShippableCacheHash = + hash_create("Shippable cache", 256, &ctl, HASH_ELEM); +} + +/* + * Returns true if given operator/function is part of an extension declared in the + * server options. + */ +static bool +lookup_shippable(Oid procnumber, List *extension_list) +{ + static int nkeys = 1; + ScanKeyData key[nkeys]; + HeapTuple tup; + Relation depRel; + SysScanDesc scan; + int nresults = 0; + + /* Always return false if we don't have any declared extensions */ + if (!extension_list) + return false; + + /* We need this relation to scan */ + depRel = heap_open(DependRelationId, RowExclusiveLock); + + /* Scan the system dependency table for a all entries this operator */ + /* depends on, then iterate through and see if one of them */ + /* is a registered extension */ + ScanKeyInit(&key[0], + Anum_pg_depend_objid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(procnumber)); + + scan = systable_beginscan(depRel, DependDependerIndexId, true, + GetCatalogSnapshot(depRel->rd_id), nkeys, key); + + while (HeapTupleIsValid(tup = systable_getnext(scan))) + { + Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(tup); + + if (foundDep->deptype == DEPENDENCY_EXTENSION) + { + ListCell *ext; + + foreach(ext, extension_list) + { + Oid extension_oid = (Oid) lfirst(ext); + if (foundDep->refobjid == extension_oid) + { + nresults++; + } + } + } + if (nresults > 0) break; + } + + systable_endscan(scan); + relation_close(depRel, RowExclusiveLock); + + return nresults > 0; +} + +/* + * is_shippable + * Is this procedure/op shippable to foreign server? + * Check cache first, then look-up whether proc/op is + * part of declared extension if not cached. + */ +bool +is_shippable(Oid procnumber, PgFdwRelationInfo *fpinfo) +{ + ShippableCacheKey key; + ShippableCacheEntry *entry; + + /* Find existing cache, if any. */ + if (!ShippableCacheHash) + InitializeShippableCache(); + + /* Zero out the key */ + memset(&key, 0, sizeof(key)); + + key.procid = procnumber; + + entry = (ShippableCacheEntry *) + hash_search(ShippableCacheHash, + (void *) &key, + HASH_FIND, + NULL); + + /* Not found in ShippableCacheHash cache. Construct new entry. */ + if (!entry) + { + /* Right now "shippability" is exclusively a function of whether */ + /* the proc is in an extension declared by the user. In the future */ + /* we could additionally have a whitelist of functions declared one */ + /* at a time. */ + bool shippable = lookup_shippable(procnumber, fpinfo->extensions); + + entry = (ShippableCacheEntry *) + hash_search(ShippableCacheHash, + (void *) &key, + HASH_ENTER, + NULL); + + entry->shippable = shippable; + } + + if (!entry) + return false; + else + return entry->shippable; +} + +/* + * extractExtensionList + * Parse a comma-separated string and fill out the list + * argument with the Oids of the extensions in the string. + * If an extenstion provided cannot be looked up in the + * catalog (it hasn't been installed or doesn't exist) + * then throw an error. + */ +bool +extractExtensionList(char *extensionString, List **extensionOids) +{ + List *extlist; + ListCell *l, *o; + + if (!SplitIdentifierString(extensionString, ',', &extlist)) + { + list_free(extlist); + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("unable to parse extension list \"%s\"", + extensionString))); + } + + foreach(l, extlist) + { + const char *extension_name = (const char *) lfirst(l); + Oid extension_oid = get_extension_oid(extension_name, true); + if (extension_oid == InvalidOid) + { + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("the \"%s\" extension must be installed locally before it can be used on a remote server", + extension_name))); + } + /* Option validation calls this function with NULL in the */ + /* extensionOids parameter, to just do existence/syntax */ + /* checking of the option */ + else if (extensionOids) + { + bool found = false; + /* Only add this extension Oid to the list */ + /* if we don't already have it in the list */ + foreach(o, *extensionOids) + { + Oid oid = (Oid) lfirst(o); + if (oid == extension_oid) + found = true; + } + if (!found) + *extensionOids = lappend_oid(*extensionOids, extension_oid); + } + } + + list_free(extlist); + return true; +} From 90ed619c2954da53ace41d0369938b41cd93e6a8 Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Tue, 21 Jul 2015 13:38:26 -0700 Subject: [PATCH 09/16] Comment cleanup --- contrib/postgres_fdw/shippable.c | 46 +++++++++++++++++--------------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/contrib/postgres_fdw/shippable.c b/contrib/postgres_fdw/shippable.c index d490306675fab..29745ed1fc0de 100644 --- a/contrib/postgres_fdw/shippable.c +++ b/contrib/postgres_fdw/shippable.c @@ -1,16 +1,16 @@ /*------------------------------------------------------------------------- * - * nonbuiltin.c - * Non-built-in procs cache management and utilities. + * shippable.c + * Non-built-in objects cache management and utilities. * - * Non-built-in functions can only be passed to remote servers when - * properly declared. The list of declared "safe" functions is cached - * and refreshed here. + * Is a non-built-in shippable to the remote server? Only if + * the object is in an extension declared by the user in the + * OPTIONS of the wrapper or the server. * * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group * * IDENTIFICATION - * contrib/postgres_fdw/nonbuiltin.c + * contrib/postgres_fdw/shippable.c * *------------------------------------------------------------------------- */ @@ -32,19 +32,19 @@ #include "utils/rel.h" #include "utils/snapmgr.h" -/* Hash table for informations about remote procs we'll call */ +/* Hash table for informations about remote objects we'll call */ static HTAB *ShippableCacheHash = NULL; -/* procid is the lookup key, must appear first */ +/* objid is the lookup key, must appear first */ typedef struct { - Oid procid; + Oid objid; } ShippableCacheKey; typedef struct { ShippableCacheKey key; /* lookup key - must be first */ - bool shippable; /* extension the proc appears within, or InvalidOid if none */ + bool shippable; /* extension the object appears within, or InvalidOid if none */ } ShippableCacheEntry; /* @@ -69,7 +69,7 @@ InitializeShippableCache(void) * server options. */ static bool -lookup_shippable(Oid procnumber, List *extension_list) +lookup_shippable(Oid objnumber, List *extension_list) { static int nkeys = 1; ScanKeyData key[nkeys]; @@ -85,13 +85,13 @@ lookup_shippable(Oid procnumber, List *extension_list) /* We need this relation to scan */ depRel = heap_open(DependRelationId, RowExclusiveLock); - /* Scan the system dependency table for a all entries this operator */ + /* Scan the system dependency table for all entries this object */ /* depends on, then iterate through and see if one of them */ - /* is a registered extension */ + /* is an extension declared by the user in the options */ ScanKeyInit(&key[0], Anum_pg_depend_objid, BTEqualStrategyNumber, F_OIDEQ, - ObjectIdGetDatum(procnumber)); + ObjectIdGetDatum(objnumber)); scan = systable_beginscan(depRel, DependDependerIndexId, true, GetCatalogSnapshot(depRel->rd_id), nkeys, key); @@ -124,16 +124,20 @@ lookup_shippable(Oid procnumber, List *extension_list) /* * is_shippable - * Is this procedure/op shippable to foreign server? - * Check cache first, then look-up whether proc/op is - * part of declared extension if not cached. + * Is this object (proc/op/type) shippable to foreign server? + * Check cache first, then look-up whether (proc/op/type) is + * part of a declared extension if it is not cached. */ bool -is_shippable(Oid procnumber, PgFdwRelationInfo *fpinfo) +is_shippable(Oid objnumber, PgFdwRelationInfo *fpinfo) { ShippableCacheKey key; ShippableCacheEntry *entry; + /* Always return false if we don't have any declared extensions */ + if (!fpinfo->extensions) + return false; + /* Find existing cache, if any. */ if (!ShippableCacheHash) InitializeShippableCache(); @@ -141,7 +145,7 @@ is_shippable(Oid procnumber, PgFdwRelationInfo *fpinfo) /* Zero out the key */ memset(&key, 0, sizeof(key)); - key.procid = procnumber; + key.objid = objnumber; entry = (ShippableCacheEntry *) hash_search(ShippableCacheHash, @@ -153,10 +157,10 @@ is_shippable(Oid procnumber, PgFdwRelationInfo *fpinfo) if (!entry) { /* Right now "shippability" is exclusively a function of whether */ - /* the proc is in an extension declared by the user. In the future */ + /* the obj (proc/op/type) is in an extension declared by the user. In the future */ /* we could additionally have a whitelist of functions declared one */ /* at a time. */ - bool shippable = lookup_shippable(procnumber, fpinfo->extensions); + bool shippable = lookup_shippable(objnumber, fpinfo->extensions); entry = (ShippableCacheEntry *) hash_search(ShippableCacheHash, From 1dd9e452122b66e4ac4b6f11d1c071a10eba8839 Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Thu, 23 Jul 2015 06:21:12 -0700 Subject: [PATCH 10/16] Match original whitespace --- contrib/postgres_fdw/deparse.c | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 904cc94243aaf..d6fff30c251da 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -670,6 +670,7 @@ is_builtin(Oid oid) return (oid < FirstBootstrapObjectId); } + /* * Construct a simple SELECT statement that retrieves desired columns * of the specified foreign table, and append it to "buf". The output From 91e826fe4a3ce4574b47a1f4d5fe8fcb31b50f79 Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Thu, 23 Jul 2015 07:10:55 -0700 Subject: [PATCH 11/16] Clear the cache when there are changes to wrapper/server definitions --- contrib/postgres_fdw/shippable.c | 33 ++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/contrib/postgres_fdw/shippable.c b/contrib/postgres_fdw/shippable.c index 29745ed1fc0de..50c0ea2a2edae 100644 --- a/contrib/postgres_fdw/shippable.c +++ b/contrib/postgres_fdw/shippable.c @@ -29,8 +29,10 @@ #include "utils/builtins.h" #include "utils/fmgroids.h" #include "utils/hsearch.h" +#include "utils/inval.h" #include "utils/rel.h" #include "utils/snapmgr.h" +#include "utils/syscache.h" /* Hash table for informations about remote objects we'll call */ static HTAB *ShippableCacheHash = NULL; @@ -47,6 +49,28 @@ typedef struct bool shippable; /* extension the object appears within, or InvalidOid if none */ } ShippableCacheEntry; +/* + * InvalidateShippableCacheCallback + * Flush all cache entries when pg_foreign_data_wrapper + * or pg_foreign_server is updated. + */ +static void +InvalidateShippableCacheCallback(Datum arg, int cacheid, uint32 hashvalue) +{ + HASH_SEQ_STATUS status; + ShippableCacheEntry *entry; + + hash_seq_init(&status, ShippableCacheHash); + while ((entry = (ShippableCacheEntry *) hash_seq_search(&status)) != NULL) + { + if (hash_search(ShippableCacheHash, + (void *) &entry->key, + HASH_REMOVE, + NULL) == NULL) + elog(ERROR, "hash table corrupted"); + } +} + /* * InitializeShippableCache * Initialize the cache of functions we can ship to remote server. @@ -62,6 +86,15 @@ InitializeShippableCache(void) ctl.entrysize = sizeof(ShippableCacheEntry); ShippableCacheHash = hash_create("Shippable cache", 256, &ctl, HASH_ELEM); + + /* Watch for invalidation events. */ + CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID, + InvalidateShippableCacheCallback, + (Datum) 0); + + CacheRegisterSyscacheCallback(FOREIGNSERVEROID, + InvalidateShippableCacheCallback, + (Datum) 0); } /* From 35ad5133f0f2d6421f9e7fbb80294179e917e3ac Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Fri, 24 Jul 2015 06:10:40 -0700 Subject: [PATCH 12/16] Add note that extensions can be set on wrappers too --- doc/src/sgml/postgres-fdw.sgml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 7b3d8c736992f..55bdca42924ca 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -391,12 +391,15 @@ This option allows you to declare what extensions you expect are installed on the foreign server, using a comma-separated list of extension names. The extensions are also expected to be installed - on the local server too. + on the local server too. The option is available on the wrapper and + on servers. CREATE SERVER foreign_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host '127.0.0.1', port '5432', dbname 'my_db', extensions 'cube, seg'); + +ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( SET extensions 'seg' ); From c6362af97a35b4e5289334061aa7ab2798ac10a8 Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Fri, 25 Sep 2015 12:16:08 -0700 Subject: [PATCH 13/16] Changes based on code review from Michael Paquier --- contrib/postgres_fdw/Makefile | 3 +- contrib/postgres_fdw/deparse.c | 21 +++-- contrib/postgres_fdw/expected/shippable.out | 84 ++++++++++++++++++++ contrib/postgres_fdw/postgres_fdw.h | 2 +- contrib/postgres_fdw/shippable.c | 85 ++++++++++----------- contrib/postgres_fdw/sql/shippable.sql | 67 ++++++++++++++++ 6 files changed, 210 insertions(+), 52 deletions(-) create mode 100644 contrib/postgres_fdw/expected/shippable.out create mode 100644 contrib/postgres_fdw/sql/shippable.sql diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile index 354331247a822..63576c481ddc7 100644 --- a/contrib/postgres_fdw/Makefile +++ b/contrib/postgres_fdw/Makefile @@ -10,7 +10,8 @@ SHLIB_LINK = $(libpq) EXTENSION = postgres_fdw DATA = postgres_fdw--1.0.sql -REGRESS = postgres_fdw +REGRESS = postgres_fdw shippable +EXTRA_INSTALL = contrib/seg ifdef USE_PGXS PG_CONFIG = pg_config diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index ce4c673a1267f..897ecdbd93192 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -381,7 +381,7 @@ foreign_expr_walker(Node *node, * can't be sent to remote because it might have incompatible * semantics on remote side. */ - if (!is_builtin(fe->funcid) && !is_shippable(fe->funcid, fpinfo)) + if (!is_builtin(fe->funcid) && !is_shippable(fe->funcid, fpinfo->extensions)) return false; /* @@ -429,7 +429,7 @@ foreign_expr_walker(Node *node, * (If the operator is, surely its underlying function is * too.) */ - if (!is_builtin(oe->opno) && !is_shippable(oe->opno, fpinfo)) + if (!is_builtin(oe->opno) && !is_shippable(oe->opno, fpinfo->extensions)) return false; /* @@ -469,7 +469,7 @@ foreign_expr_walker(Node *node, /* * Again, only built-in operators can be sent to remote. */ - if (!is_builtin(oe->opno) && !is_shippable(oe->opno, fpinfo)) + if (!is_builtin(oe->opno) && !is_shippable(oe->opno, fpinfo->extensions)) return false; /* @@ -619,7 +619,7 @@ foreign_expr_walker(Node *node, * If result type of given expression is not built-in, it can't be sent to * remote because it might have incompatible semantics on remote side. */ - if (check_type && !is_builtin(exprType(node)) && !is_shippable(exprType(node), fpinfo)) + if (check_type && !is_builtin(exprType(node)) && !is_shippable(exprType(node), fpinfo->extensions)) return false; /* @@ -1354,6 +1354,9 @@ deparseConst(Const *node, deparse_expr_cxt *context) bool isfloat = false; bool needlabel; + /* Access extension metadata from fpinfo on baserel */ + PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *)(context->foreignrel->fdw_private); + if (node->constisnull) { appendStringInfoString(buf, "NULL"); @@ -1431,8 +1434,16 @@ deparseConst(Const *node, deparse_expr_cxt *context) break; } if (needlabel) + { + /* + * References to extension types need to be fully qualified, + * but references to built-in types shouldn't be. + */ appendStringInfo(buf, "::%s", - format_type_be_qualified(node->consttype)); + is_shippable(node->consttype, fpinfo->extensions) ? + format_type_be_qualified(node->consttype) : + format_type_with_typemod(node->consttype, node->consttypmod)); + } } /* diff --git a/contrib/postgres_fdw/expected/shippable.out b/contrib/postgres_fdw/expected/shippable.out new file mode 100644 index 0000000000000..17878305172a5 --- /dev/null +++ b/contrib/postgres_fdw/expected/shippable.out @@ -0,0 +1,84 @@ +-- =================================================================== +-- create FDW objects +-- =================================================================== +-- Error, extension isn't installed yet +ALTER SERVER loopback OPTIONS (ADD extensions 'seg'); +ERROR: the "seg" extension must be installed locally before it can be used on a remote server +-- Try again +CREATE EXTENSION seg; +ALTER SERVER loopback OPTIONS (ADD extensions 'seg'); +ALTER SERVER loopback OPTIONS (DROP extensions); +-- =================================================================== +-- create objects used through FDW loopback server +-- =================================================================== +CREATE SCHEMA "SH 1"; +CREATE TABLE "SH 1"."TBL 1" ( + "C 1" int NOT NULL, + c2 int NOT NULL, + c3 seg, + c4 timestamptz +); +INSERT INTO "SH 1"."TBL 1" + SELECT id, + 2 * id, + (id || ' .. ' || 2*id)::seg, + '1970-01-01'::timestamptz + ((id % 100) || ' days')::interval + FROM generate_series(1, 1000) id; +ANALYZE "SH 1"."TBL 1"; +-- =================================================================== +-- create foreign table +-- =================================================================== +CREATE FOREIGN TABLE shft1 ( + "C 1" int NOT NULL, + c2 int NOT NULL, + c3 seg, + c4 timestamptz +) SERVER loopback +OPTIONS (schema_name 'SH 1', table_name 'TBL 1'); +-- =================================================================== +-- simple queries +-- =================================================================== +-- without operator shipping +EXPLAIN (COSTS false) SELECT * FROM shft1 LIMIT 1; + QUERY PLAN +----------------------------- + Limit + -> Foreign Scan on shft1 +(2 rows) + +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; + QUERY PLAN +--------------------------------------------------------------------- + Foreign Scan on public.shft1 (cost=100.00..205.06 rows=15 width=4) + Output: c2 + Filter: (shft1.c3 && '1.5 .. 2.5'::seg) + Remote SQL: SELECT c2, c3 FROM "SH 1"."TBL 1" +(4 rows) + +SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; + c2 +---- + 2 + 4 +(2 rows) + +-- with operator shipping +ALTER SERVER loopback OPTIONS (ADD extensions 'seg'); +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; + QUERY PLAN +------------------------------------------------------------------------------------------------------- + Foreign Scan on public.shft1 (cost=100.00..146.86 rows=15 width=4) + Output: c2 + Remote SQL: SELECT c2 FROM "SH 1"."TBL 1" WHERE ((c3 OPERATOR(public.&&) '1.5 .. 2.5'::public.seg)) +(3 rows) + +SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; + c2 +---- + 2 + 4 +(2 rows) + +-- =================================================================== +-- clean up +-- =================================================================== diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index 8e16ee92952bf..a264e497d7a0a 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -79,7 +79,7 @@ extern int ExtractConnectionOptions(List *defelems, /* in shippable.c */ extern bool extractExtensionList(char *extensionString, List **extensionOids); -extern bool is_shippable(Oid procnumber, PgFdwRelationInfo *fpinfo); +extern bool is_shippable(Oid procnumber, List *extension_list); /* in deparse.c */ extern void classifyConditions(PlannerInfo *root, diff --git a/contrib/postgres_fdw/shippable.c b/contrib/postgres_fdw/shippable.c index 50c0ea2a2edae..1422300fe897b 100644 --- a/contrib/postgres_fdw/shippable.c +++ b/contrib/postgres_fdw/shippable.c @@ -45,8 +45,10 @@ typedef struct typedef struct { - ShippableCacheKey key; /* lookup key - must be first */ - bool shippable; /* extension the object appears within, or InvalidOid if none */ + /* lookup key - must be first */ + ShippableCacheKey key; + /* extension the object appears within, or InvalidOid if none */ + bool shippable; } ShippableCacheEntry; /* @@ -98,8 +100,8 @@ InitializeShippableCache(void) } /* - * Returns true if given operator/function is part of an extension declared in the - * server options. + * Returns true if given operator/function is part of an extension declared in + * the server options. */ static bool lookup_shippable(Oid objnumber, List *extension_list) @@ -109,18 +111,20 @@ lookup_shippable(Oid objnumber, List *extension_list) HeapTuple tup; Relation depRel; SysScanDesc scan; - int nresults = 0; + bool is_shippable = false; /* Always return false if we don't have any declared extensions */ - if (!extension_list) + if (extension_list == NIL) return false; /* We need this relation to scan */ depRel = heap_open(DependRelationId, RowExclusiveLock); - /* Scan the system dependency table for all entries this object */ - /* depends on, then iterate through and see if one of them */ - /* is an extension declared by the user in the options */ + /* + * Scan the system dependency table for all entries this object + * depends on, then iterate through and see if one of them + * is an extension declared by the user in the options + */ ScanKeyInit(&key[0], Anum_pg_depend_objid, BTEqualStrategyNumber, F_OIDEQ, @@ -133,26 +137,18 @@ lookup_shippable(Oid objnumber, List *extension_list) { Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(tup); - if (foundDep->deptype == DEPENDENCY_EXTENSION) + if (foundDep->deptype == DEPENDENCY_EXTENSION && + list_member_oid(extension_list, foundDep->refobjid)) { - ListCell *ext; - - foreach(ext, extension_list) - { - Oid extension_oid = (Oid) lfirst(ext); - if (foundDep->refobjid == extension_oid) - { - nresults++; - } - } + is_shippable = true; + break; } - if (nresults > 0) break; } systable_endscan(scan); relation_close(depRel, RowExclusiveLock); - return nresults > 0; + return is_shippable; } /* @@ -162,13 +158,13 @@ lookup_shippable(Oid objnumber, List *extension_list) * part of a declared extension if it is not cached. */ bool -is_shippable(Oid objnumber, PgFdwRelationInfo *fpinfo) +is_shippable(Oid objnumber, List *extension_list) { ShippableCacheKey key; ShippableCacheEntry *entry; /* Always return false if we don't have any declared extensions */ - if (!fpinfo->extensions) + if (extension_list == NIL) return false; /* Find existing cache, if any. */ @@ -189,11 +185,13 @@ is_shippable(Oid objnumber, PgFdwRelationInfo *fpinfo) /* Not found in ShippableCacheHash cache. Construct new entry. */ if (!entry) { - /* Right now "shippability" is exclusively a function of whether */ - /* the obj (proc/op/type) is in an extension declared by the user. In the future */ - /* we could additionally have a whitelist of functions declared one */ - /* at a time. */ - bool shippable = lookup_shippable(objnumber, fpinfo->extensions); + /* + * Right now "shippability" is exclusively a function of whether + * the obj (proc/op/type) is in an extension declared by the user. + * In the future we could additionally have a whitelist of functions + * declared one at a time. + */ + bool shippable = lookup_shippable(objnumber, extension_list); entry = (ShippableCacheEntry *) hash_search(ShippableCacheHash, @@ -222,11 +220,10 @@ bool extractExtensionList(char *extensionString, List **extensionOids) { List *extlist; - ListCell *l, *o; + ListCell *l; if (!SplitIdentifierString(extensionString, ',', &extlist)) { - list_free(extlist); ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("unable to parse extension list \"%s\"", @@ -241,24 +238,22 @@ extractExtensionList(char *extensionString, List **extensionOids) { ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("the \"%s\" extension must be installed locally before it can be used on a remote server", + errmsg("the \"%s\" extension must be installed locally " + "before it can be used on a remote server", extension_name))); } - /* Option validation calls this function with NULL in the */ - /* extensionOids parameter, to just do existence/syntax */ - /* checking of the option */ + /* + * Option validation calls this function with NULL in the + * extensionOids parameter, to just do existence/syntax + * checking of the option + */ else if (extensionOids) { - bool found = false; - /* Only add this extension Oid to the list */ - /* if we don't already have it in the list */ - foreach(o, *extensionOids) - { - Oid oid = (Oid) lfirst(o); - if (oid == extension_oid) - found = true; - } - if (!found) + /* + * Only add this extension Oid to the list + * if we don't already have it in the list + */ + if (!list_member_oid(*extensionOids, extension_oid)) *extensionOids = lappend_oid(*extensionOids, extension_oid); } } diff --git a/contrib/postgres_fdw/sql/shippable.sql b/contrib/postgres_fdw/sql/shippable.sql new file mode 100644 index 0000000000000..3d24d9754ef8f --- /dev/null +++ b/contrib/postgres_fdw/sql/shippable.sql @@ -0,0 +1,67 @@ +-- =================================================================== +-- create FDW objects +-- =================================================================== + +-- Error, extension isn't installed yet +ALTER SERVER loopback OPTIONS (ADD extensions 'seg'); + +-- Try again +CREATE EXTENSION seg; +ALTER SERVER loopback OPTIONS (ADD extensions 'seg'); +ALTER SERVER loopback OPTIONS (DROP extensions); + + +-- =================================================================== +-- create objects used through FDW loopback server +-- =================================================================== + +CREATE SCHEMA "SH 1"; +CREATE TABLE "SH 1"."TBL 1" ( + "C 1" int NOT NULL, + c2 int NOT NULL, + c3 seg, + c4 timestamptz +); + +INSERT INTO "SH 1"."TBL 1" + SELECT id, + 2 * id, + (id || ' .. ' || 2*id)::seg, + '1970-01-01'::timestamptz + ((id % 100) || ' days')::interval + FROM generate_series(1, 1000) id; + +ANALYZE "SH 1"."TBL 1"; + +-- =================================================================== +-- create foreign table +-- =================================================================== + +CREATE FOREIGN TABLE shft1 ( + "C 1" int NOT NULL, + c2 int NOT NULL, + c3 seg, + c4 timestamptz +) SERVER loopback +OPTIONS (schema_name 'SH 1', table_name 'TBL 1'); + +-- =================================================================== +-- simple queries +-- =================================================================== + +-- without operator shipping +EXPLAIN (COSTS false) SELECT * FROM shft1 LIMIT 1; +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; +SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; + +-- with operator shipping +ALTER SERVER loopback OPTIONS (ADD extensions 'seg'); +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; +SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; + + +-- =================================================================== +-- clean up +-- =================================================================== + + + From 1634fc80106b16cf0cb95ba357459dc6dfe919e2 Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Fri, 25 Sep 2015 12:46:03 -0700 Subject: [PATCH 14/16] Use 'cube' as extension basis for regression test --- contrib/postgres_fdw/Makefile | 2 +- contrib/postgres_fdw/expected/shippable.out | 81 +++++++++++++++++---- contrib/postgres_fdw/shippable.c | 2 +- contrib/postgres_fdw/sql/shippable.sql | 29 +++++--- 4 files changed, 85 insertions(+), 29 deletions(-) diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile index 63576c481ddc7..d36128f37ccf5 100644 --- a/contrib/postgres_fdw/Makefile +++ b/contrib/postgres_fdw/Makefile @@ -11,7 +11,7 @@ EXTENSION = postgres_fdw DATA = postgres_fdw--1.0.sql REGRESS = postgres_fdw shippable -EXTRA_INSTALL = contrib/seg +EXTRA_INSTALL = contrib/cube ifdef USE_PGXS PG_CONFIG = pg_config diff --git a/contrib/postgres_fdw/expected/shippable.out b/contrib/postgres_fdw/expected/shippable.out index 17878305172a5..bc5e3203b0903 100644 --- a/contrib/postgres_fdw/expected/shippable.out +++ b/contrib/postgres_fdw/expected/shippable.out @@ -2,11 +2,11 @@ -- create FDW objects -- =================================================================== -- Error, extension isn't installed yet -ALTER SERVER loopback OPTIONS (ADD extensions 'seg'); -ERROR: the "seg" extension must be installed locally before it can be used on a remote server +ALTER SERVER loopback OPTIONS (ADD extensions 'cube'); +ERROR: the "cube" extension must be installed locally before it can be used on a remote server -- Try again -CREATE EXTENSION seg; -ALTER SERVER loopback OPTIONS (ADD extensions 'seg'); +CREATE EXTENSION cube; +ALTER SERVER loopback OPTIONS (ADD extensions 'cube'); ALTER SERVER loopback OPTIONS (DROP extensions); -- =================================================================== -- create objects used through FDW loopback server @@ -15,13 +15,13 @@ CREATE SCHEMA "SH 1"; CREATE TABLE "SH 1"."TBL 1" ( "C 1" int NOT NULL, c2 int NOT NULL, - c3 seg, + c3 cube, c4 timestamptz ); INSERT INTO "SH 1"."TBL 1" SELECT id, 2 * id, - (id || ' .. ' || 2*id)::seg, + cube(id,2*id), '1970-01-01'::timestamptz + ((id % 100) || ' days')::interval FROM generate_series(1, 1000) id; ANALYZE "SH 1"."TBL 1"; @@ -31,7 +31,7 @@ ANALYZE "SH 1"."TBL 1"; CREATE FOREIGN TABLE shft1 ( "C 1" int NOT NULL, c2 int NOT NULL, - c3 seg, + c3 cube, c4 timestamptz ) SERVER loopback OPTIONS (schema_name 'SH 1', table_name 'TBL 1'); @@ -46,33 +46,82 @@ EXPLAIN (COSTS false) SELECT * FROM shft1 LIMIT 1; -> Foreign Scan on shft1 (2 rows) -EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && cube(1.5,2.5); QUERY PLAN --------------------------------------------------------------------- Foreign Scan on public.shft1 (cost=100.00..205.06 rows=15 width=4) Output: c2 - Filter: (shft1.c3 && '1.5 .. 2.5'::seg) + Filter: (shft1.c3 && '(1.5),(2.5)'::cube) Remote SQL: SELECT c2, c3 FROM "SH 1"."TBL 1" (4 rows) -SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; +SELECT c2 FROM shft1 WHERE c3 && cube(1.5,2.5); c2 ---- 2 4 (2 rows) +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && '(1.5),(2.5)'::cube; + QUERY PLAN +--------------------------------------------------------------------- + Foreign Scan on public.shft1 (cost=100.00..205.06 rows=15 width=4) + Output: c2 + Filter: (shft1.c3 && '(1.5),(2.5)'::cube) + Remote SQL: SELECT c2, c3 FROM "SH 1"."TBL 1" +(4 rows) + -- with operator shipping -ALTER SERVER loopback OPTIONS (ADD extensions 'seg'); -EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; - QUERY PLAN -------------------------------------------------------------------------------------------------------- +ALTER SERVER loopback OPTIONS (ADD extensions 'cube'); +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && cube(1.5,2.5); + QUERY PLAN +--------------------------------------------------------------------------------------------------------- Foreign Scan on public.shft1 (cost=100.00..146.86 rows=15 width=4) Output: c2 - Remote SQL: SELECT c2 FROM "SH 1"."TBL 1" WHERE ((c3 OPERATOR(public.&&) '1.5 .. 2.5'::public.seg)) + Remote SQL: SELECT c2 FROM "SH 1"."TBL 1" WHERE ((c3 OPERATOR(public.&&) '(1.5),(2.5)'::public.cube)) (3 rows) -SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; +SELECT c2 FROM shft1 WHERE c3 && cube(1.5,2.5); + c2 +---- + 2 + 4 +(2 rows) + +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && '(1.5),(2.5)'::cube; + QUERY PLAN +--------------------------------------------------------------------------------------------------------- + Foreign Scan on public.shft1 (cost=100.00..146.86 rows=15 width=4) + Output: c2 + Remote SQL: SELECT c2 FROM "SH 1"."TBL 1" WHERE ((c3 OPERATOR(public.&&) '(1.5),(2.5)'::public.cube)) +(3 rows) + +EXPLAIN VERBOSE SELECT cube_dim(c3) FROM shft1 WHERE c3 && '(1.5),(2.5)'::cube; + QUERY PLAN +--------------------------------------------------------------------------------------------------------- + Foreign Scan on public.shft1 (cost=100.00..128.43 rows=7 width=32) + Output: cube_dim(c3) + Remote SQL: SELECT c3 FROM "SH 1"."TBL 1" WHERE ((c3 OPERATOR(public.&&) '(1.5),(2.5)'::public.cube)) +(3 rows) + +SELECT cube_dim(c3) FROM shft1 WHERE c3 && '(1.5),(2.5)'::cube; + cube_dim +---------- + 1 + 1 +(2 rows) + +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE cube_dim(c3) = 1 LIMIT 2; + QUERY PLAN +------------------------------------------------------------------------------------- + Limit (cost=100.00..107.22 rows=2 width=4) + Output: c2 + -> Foreign Scan on public.shft1 (cost=100.00..154.18 rows=15 width=4) + Output: c2 + Remote SQL: SELECT c2 FROM "SH 1"."TBL 1" WHERE ((public.cube_dim(c3) = 1)) +(5 rows) + +SELECT c2 FROM shft1 WHERE cube_dim(c3) = 1 LIMIT 2; c2 ---- 2 diff --git a/contrib/postgres_fdw/shippable.c b/contrib/postgres_fdw/shippable.c index 1422300fe897b..65533f7ab527d 100644 --- a/contrib/postgres_fdw/shippable.c +++ b/contrib/postgres_fdw/shippable.c @@ -239,7 +239,7 @@ extractExtensionList(char *extensionString, List **extensionOids) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("the \"%s\" extension must be installed locally " - "before it can be used on a remote server", + "before it can be used on a remote server", extension_name))); } /* diff --git a/contrib/postgres_fdw/sql/shippable.sql b/contrib/postgres_fdw/sql/shippable.sql index 3d24d9754ef8f..f2a991b309e75 100644 --- a/contrib/postgres_fdw/sql/shippable.sql +++ b/contrib/postgres_fdw/sql/shippable.sql @@ -3,11 +3,11 @@ -- =================================================================== -- Error, extension isn't installed yet -ALTER SERVER loopback OPTIONS (ADD extensions 'seg'); +ALTER SERVER loopback OPTIONS (ADD extensions 'cube'); -- Try again -CREATE EXTENSION seg; -ALTER SERVER loopback OPTIONS (ADD extensions 'seg'); +CREATE EXTENSION cube; +ALTER SERVER loopback OPTIONS (ADD extensions 'cube'); ALTER SERVER loopback OPTIONS (DROP extensions); @@ -19,14 +19,14 @@ CREATE SCHEMA "SH 1"; CREATE TABLE "SH 1"."TBL 1" ( "C 1" int NOT NULL, c2 int NOT NULL, - c3 seg, + c3 cube, c4 timestamptz ); INSERT INTO "SH 1"."TBL 1" SELECT id, 2 * id, - (id || ' .. ' || 2*id)::seg, + cube(id,2*id), '1970-01-01'::timestamptz + ((id % 100) || ' days')::interval FROM generate_series(1, 1000) id; @@ -39,7 +39,7 @@ ANALYZE "SH 1"."TBL 1"; CREATE FOREIGN TABLE shft1 ( "C 1" int NOT NULL, c2 int NOT NULL, - c3 seg, + c3 cube, c4 timestamptz ) SERVER loopback OPTIONS (schema_name 'SH 1', table_name 'TBL 1'); @@ -50,13 +50,20 @@ OPTIONS (schema_name 'SH 1', table_name 'TBL 1'); -- without operator shipping EXPLAIN (COSTS false) SELECT * FROM shft1 LIMIT 1; -EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; -SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && cube(1.5,2.5); +SELECT c2 FROM shft1 WHERE c3 && cube(1.5,2.5); +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && '(1.5),(2.5)'::cube; -- with operator shipping -ALTER SERVER loopback OPTIONS (ADD extensions 'seg'); -EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; -SELECT c2 FROM shft1 WHERE c3 && '1.5 .. 2.5'::seg; +ALTER SERVER loopback OPTIONS (ADD extensions 'cube'); +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && cube(1.5,2.5); +SELECT c2 FROM shft1 WHERE c3 && cube(1.5,2.5); +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE c3 && '(1.5),(2.5)'::cube; +EXPLAIN VERBOSE SELECT cube_dim(c3) FROM shft1 WHERE c3 && '(1.5),(2.5)'::cube; +SELECT cube_dim(c3) FROM shft1 WHERE c3 && '(1.5),(2.5)'::cube; + +EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE cube_dim(c3) = 1 LIMIT 2; +SELECT c2 FROM shft1 WHERE cube_dim(c3) = 1 LIMIT 2; -- =================================================================== From 9b465976c93f95d8f6403ee65d8f5e1994fd24fe Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Mon, 28 Sep 2015 06:45:51 -0700 Subject: [PATCH 15/16] Minor feedback changes --- contrib/postgres_fdw/Makefile | 1 + contrib/postgres_fdw/shippable.c | 7 +++---- contrib/postgres_fdw/sql/shippable.sql | 5 ----- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile index d36128f37ccf5..f78fc64c06919 100644 --- a/contrib/postgres_fdw/Makefile +++ b/contrib/postgres_fdw/Makefile @@ -10,6 +10,7 @@ SHLIB_LINK = $(libpq) EXTENSION = postgres_fdw DATA = postgres_fdw--1.0.sql +# Note: shippable tests depend on postgres_fdw tests setup REGRESS = postgres_fdw shippable EXTRA_INSTALL = contrib/cube diff --git a/contrib/postgres_fdw/shippable.c b/contrib/postgres_fdw/shippable.c index 65533f7ab527d..b529bec3ca103 100644 --- a/contrib/postgres_fdw/shippable.c +++ b/contrib/postgres_fdw/shippable.c @@ -1,11 +1,10 @@ /*------------------------------------------------------------------------- * * shippable.c - * Non-built-in objects cache management and utilities. + * Facility to track database objects shippable to a foreign server. * - * Is a non-built-in shippable to the remote server? Only if - * the object is in an extension declared by the user in the - * OPTIONS of the wrapper or the server. + * Determine if functions and operators for non-built-in types/functions/ops + * are shippable to the remote server. * * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group * diff --git a/contrib/postgres_fdw/sql/shippable.sql b/contrib/postgres_fdw/sql/shippable.sql index f2a991b309e75..b865c4d56fe16 100644 --- a/contrib/postgres_fdw/sql/shippable.sql +++ b/contrib/postgres_fdw/sql/shippable.sql @@ -66,9 +66,4 @@ EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE cube_dim(c3) = 1 LIMIT 2; SELECT c2 FROM shft1 WHERE cube_dim(c3) = 1 LIMIT 2; --- =================================================================== --- clean up --- =================================================================== - - From 9dfe1d7612fc3965fac001be101626985622d530 Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Wed, 30 Sep 2015 15:50:21 -0700 Subject: [PATCH 16/16] Clean up after regression tests, change parameter parse signature --- contrib/postgres_fdw/expected/shippable.out | 5 +++++ contrib/postgres_fdw/option.c | 2 +- contrib/postgres_fdw/postgres_fdw.c | 9 +-------- contrib/postgres_fdw/postgres_fdw.h | 3 +-- contrib/postgres_fdw/shippable.c | 17 +++++++++-------- contrib/postgres_fdw/sql/shippable.sql | 8 ++++++++ 6 files changed, 25 insertions(+), 19 deletions(-) diff --git a/contrib/postgres_fdw/expected/shippable.out b/contrib/postgres_fdw/expected/shippable.out index bc5e3203b0903..54749a3e7a46f 100644 --- a/contrib/postgres_fdw/expected/shippable.out +++ b/contrib/postgres_fdw/expected/shippable.out @@ -131,3 +131,8 @@ SELECT c2 FROM shft1 WHERE cube_dim(c3) = 1 LIMIT 2; -- =================================================================== -- clean up -- =================================================================== +DROP FOREIGN TABLE shft1; +DROP TABLE "SH 1"."TBL 1"; +DROP SCHEMA "SH 1"; +DROP EXTENSION cube; +ALTER SERVER loopback OPTIONS (DROP extensions); diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 9aeaf1a0967b2..e4c174447c4e4 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -127,7 +127,7 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) } else if (strcmp(def->defname, "extensions") == 0) { - extractExtensionList(defGetString(def), NULL); + extractExtensionList(defGetString(def), false); } } diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 42d7e25d47921..eb5eb6751277a 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -375,13 +375,6 @@ postgresGetForeignRelSize(PlannerInfo *root, fpinfo->fdw_tuple_cost = DEFAULT_FDW_TUPLE_COST; fpinfo->extensions = NIL; - foreach(lc, fpinfo->wrapper->options) - { - DefElem *def = (DefElem *) lfirst(lc); - - if (strcmp(def->defname, "extensions") == 0) - extractExtensionList(defGetString(def), &(fpinfo->extensions)); - } foreach(lc, fpinfo->server->options) { DefElem *def = (DefElem *) lfirst(lc); @@ -393,7 +386,7 @@ postgresGetForeignRelSize(PlannerInfo *root, else if (strcmp(def->defname, "fdw_tuple_cost") == 0) fpinfo->fdw_tuple_cost = strtod(defGetString(def), NULL); else if (strcmp(def->defname, "extensions") == 0) - extractExtensionList(defGetString(def), &(fpinfo->extensions)); + fpinfo->extensions = extractExtensionList(defGetString(def), true); } foreach(lc, fpinfo->table->options) { diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index a264e497d7a0a..98d9fea4d9b70 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -77,8 +77,7 @@ extern int ExtractConnectionOptions(List *defelems, const char **values); /* in shippable.c */ -extern bool extractExtensionList(char *extensionString, - List **extensionOids); +extern List* extractExtensionList(char *extensionString, bool populateList); extern bool is_shippable(Oid procnumber, List *extension_list); /* in deparse.c */ diff --git a/contrib/postgres_fdw/shippable.c b/contrib/postgres_fdw/shippable.c index b529bec3ca103..0e2aa77c5149b 100644 --- a/contrib/postgres_fdw/shippable.c +++ b/contrib/postgres_fdw/shippable.c @@ -209,16 +209,17 @@ is_shippable(Oid objnumber, List *extension_list) /* * extractExtensionList - * Parse a comma-separated string and fill out the list - * argument with the Oids of the extensions in the string. + * Parse a comma-separated string and return a List + * of the Oids of the extensions in the string. * If an extenstion provided cannot be looked up in the * catalog (it hasn't been installed or doesn't exist) * then throw an error. */ -bool -extractExtensionList(char *extensionString, List **extensionOids) +List * +extractExtensionList(char *extensionString, bool populateList) { List *extlist; + List *extensionOids = NIL; ListCell *l; if (!SplitIdentifierString(extensionString, ',', &extlist)) @@ -246,17 +247,17 @@ extractExtensionList(char *extensionString, List **extensionOids) * extensionOids parameter, to just do existence/syntax * checking of the option */ - else if (extensionOids) + else if (populateList) { /* * Only add this extension Oid to the list * if we don't already have it in the list */ - if (!list_member_oid(*extensionOids, extension_oid)) - *extensionOids = lappend_oid(*extensionOids, extension_oid); + if (!list_member_oid(extensionOids, extension_oid)) + extensionOids = lappend_oid(extensionOids, extension_oid); } } list_free(extlist); - return true; + return extensionOids; } diff --git a/contrib/postgres_fdw/sql/shippable.sql b/contrib/postgres_fdw/sql/shippable.sql index b865c4d56fe16..9ea6fc0246de7 100644 --- a/contrib/postgres_fdw/sql/shippable.sql +++ b/contrib/postgres_fdw/sql/shippable.sql @@ -65,5 +65,13 @@ SELECT cube_dim(c3) FROM shft1 WHERE c3 && '(1.5),(2.5)'::cube; EXPLAIN VERBOSE SELECT c2 FROM shft1 WHERE cube_dim(c3) = 1 LIMIT 2; SELECT c2 FROM shft1 WHERE cube_dim(c3) = 1 LIMIT 2; +-- =================================================================== +-- clean up +-- =================================================================== +DROP FOREIGN TABLE shft1; +DROP TABLE "SH 1"."TBL 1"; +DROP SCHEMA "SH 1"; +DROP EXTENSION cube; +ALTER SERVER loopback OPTIONS (DROP extensions);