Re: [PATCH] postgres_fdw extension support

Hi,

On 2015-10-01 11:46:43 +0900, Michael Paquier wrote:
> diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
> index 7547ec2..864bf53 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"
>
>
> /*
> @@ -124,6 +126,11 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
> errmsg("%s requires a non-negative numeric value",
> def->defname)));
> }
> + else if (strcmp(def->defname, "extensions") == 0)
> + {
> + /* this must have already-installed extensions */

I don't understand that comment.

> PG_RETURN_VOID();
> @@ -153,6 +160,8 @@ InitPgFdwOptions(void)
> /* updatable is available on both server and table */
> {"updatable", ForeignServerRelationId, false},
> {"updatable", ForeignTableRelationId, false},
> + /* extensions is available on server */
> + {"extensions", ForeignServerRelationId, false},

awkward spelling in comment.

> +/*
> + * Parse a comma-separated string and return a List of the Oids of the
> + * extensions in the string. If an extension provided cannot be looked
> + * up in the catalog (it hasn't been installed or doesn't exist) then
> + * throw up an error.
> + */

s/throw up an error/throw an error/ or raise an error.

> +/*
> + * 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;
> +
> + /* Optional extensions to support (list of oid) */

*oids

> +/* objid is the lookup key, must appear first */
> +typedef struct
> +{
> + /* extension the object appears within, or InvalidOid if none */
> + Oid objid;
> +} ShippableCacheKey;
> +
> +typedef struct
> +{
> + /* lookup key - must be first */
> + ShippableCacheKey key;
> + bool shippable;
> +} ShippableCacheEntry;
> +
> +/*
> + * Flush all cache entries when 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");
> + }
> +}

> +/*
> + * 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)
> +{
> + static int nkeys = 1;
> + ScanKeyData key[nkeys];
> + HeapTuple tup;
> + Relation depRel;
> + SysScanDesc scan;
> + bool is_shippable = false;
> +
> + /* Always return false if we don't have any declared extensions */

Imo there's nothing declared here...

> + if (extension_list == NIL)
> + return false;
> +
> + /* We need this relation to scan */

Not sure what that means.

> + 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
> + */
> + ScanKeyInit(&key[0],
> + Anum_pg_depend_objid,
> + BTEqualStrategyNumber, F_OIDEQ,
> + ObjectIdGetDatum(objnumber));
> +
> + 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_member_oid(extension_list, foundDep->refobjid))
> + {
> + is_shippable = true;
> + break;
> + }
> + }

Hm.

> +/*
> + * is_shippable
> + * 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 objnumber, List *extension_list)
> +{
> + ShippableCacheKey key;
> + ShippableCacheEntry *entry;
> +
> + /* Always return false if we don't have any declared extensions */
> + if (extension_list == NIL)
> + return false;

I again dislike declared here ;)

> + /* Find existing cache, if any. */
> + if (!ShippableCacheHash)
> + InitializeShippableCache();
> +
> + /* Zero out the key */
> + memset(&key, 0, sizeof(key));
> +
> + key.objid = objnumber;

Hm. Oids can conflict between different system relations. Shouldn't the
key be over class (i.e. pg_proc, pg_type etc.) and object id?

> + 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 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,
> + (void *) &key,
> + HASH_ENTER,
> + NULL);
> +
> + entry->shippable = shippable;
> + }
>

I suggest adding a comment that we consciously are only HASH_ENTERing
the key after doing the lookup_shippable(), to avoid the issue that the
lookup might accept cache invalidations and thus delete the entry again.

Greetings,

Andres Freund