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

Andres,

I have just pushed the v10 patch. Yugo will reply back to your point
but I will look into your review as well.

Thanks.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

> 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