Re: [HACKERS] [PATCH] Lockable views - Mailing list pgsql-hackers

Hi,

On 2018-03-28 20:26:48 +0900, Yugo Nagata wrote:
> diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
> index b2c7203..96d477c 100644
> --- a/doc/src/sgml/ref/lock.sgml
> +++ b/doc/src/sgml/ref/lock.sgml
> @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ]
>    </para>
>  
>    <para>
> +   When a view is specified to be locked, all relations appearing in the view
> +   definition query are also locked recursively with the same lock mode. 
> +  </para>

Trailing space added. I'd remove "specified to be" from the sentence.

I think mentioning how this interacts with permissions would be a good
idea too. Given how operations use the view's owner to recurse, that's
not obvious. Should also explain what permissions are required to do the
operation on the view.


> @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
>          return;                    /* woops, concurrently dropped; no permissions
>                                   * check */
>  
> -    /* Currently, we only allow plain tables to be locked */
> -    if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
> +

This newline looks spurious to me.


>  /*
> + * Apply LOCK TABLE recursively over a view
> + *
> + * All tables and views appearing in the view definition query are locked
> + * recursively with the same lock mode.
> + */
> +
> +typedef struct
> +{
> +    Oid root_reloid;
> +    LOCKMODE lockmode;
> +    bool nowait;
> +    Oid viewowner;
> +    Oid viewoid;
> +} LockViewRecurse_context;

Probably wouldn't hurt to pgindent the larger changes in the patch.


> +static bool
> +LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
> +{
> +    if (node == NULL)
> +        return false;
> +
> +    if (IsA(node, Query))
> +    {
> +        Query        *query = (Query *) node;
> +        ListCell    *rtable;
> +
> +        foreach(rtable, query->rtable)
> +        {
> +            RangeTblEntry    *rte = lfirst(rtable);
> +            AclResult         aclresult;
> +
> +            Oid relid = rte->relid;
> +            char relkind = rte->relkind;
> +            char *relname = get_rel_name(relid);
> +
> +            /* The OLD and NEW placeholder entries in the view's rtable are skipped. */
> +            if (relid == context->viewoid &&
> +                (!strcmp(rte->eref->aliasname, "old") || !strcmp(rte->eref->aliasname, "new")))
> +                continue;
> +
> +            /* Currently, we only allow plain tables or views to be locked. */
> +            if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
> +                relkind != RELKIND_VIEW)
> +                continue;
> +
> +            /* Check infinite recursion in the view definition. */
> +            if (relid == context->root_reloid)
> +                ereport(ERROR,
> +                        (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> +                        errmsg("infinite recursion detected in rules for relation \"%s\"",
> +                                get_rel_name(context->root_reloid))));

Hm, how can that happen? And if it can happen, why can it only happen
with the root relation?

Greetings,

Andres Freund