Re: [PATCH] postgres_fdw extension support - Mailing list pgsql-hackers

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