Re: proposal: schema variables

ne 20. 12. 2020 v 21:43 odesílatel Zhihong Yu <zyu(at)yugabyte(dot)com> napsal:

> Hi,
> This is continuation of the previous review.
>
> + * We should to use schema variable buffer,
> when
> + * it is available.
>
> 'should use' (no to)
>

fixed

> + /* When buffer of used schema variables loaded from shared memory
> */
>
> A verb seems missing in the above comment.
>

I changed this comment

<--><-->/*
<--><--> * link shared memory with working copy of schema variable's values
<--><--> * used in this query. This access is used by parallel query
executor's
<--><--> * workers.
<--><--> */

> + elog(ERROR, "unexpected non-SELECT command in LET ... SELECT");
>
> Since non-SELECT is mentioned, I wonder if the trailing SELECT can be
> omitted.
>

done

> + * some collision can be solved simply here to reduce errors
> based
> + * on simply existence of some variables. Often error can be
> using
>
> simply occurred twice above - I think one should be enough.
> If you want to keep the second, it should be 'simple'.
>

I rewrote this comment

updated patch attached

Regards

Pavel

> Cheers
>
> On Sun, Dec 20, 2020 at 11:25 AM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
>
>> Hi,
>> I took a look at the rebased patch.
>>
>> + <entry><structfield>varisnotnull</structfield></entry>
>> + <entry><type>boolean</type></entry>
>> + <entry></entry>
>> + <entry>
>> + True if the schema variable doesn't allow null value. The default
>> value is false.
>>
>> I wonder whether the field can be named in positive tense: e.g.
>> varallowsnull with default of true.
>>
>> + <entry><structfield>vareoxaction</structfield></entry>
>> + <literal>n</literal> = no action, <literal>d</literal> = drop the
>> variable,
>> + <literal>r</literal> = reset the variable to its default value.
>>
>> Looks like there is only one action allowed. I wonder if there is a
>> possibility of having two actions at the same time in the future.
>>
>> + The <application>PL/pgSQL</application> language has not packages
>> + and then it has not package variables and package constants. The
>>
>> 'has not' -> 'has no'
>>
>> + a null value. A variable created as NOT NULL and without an
>> explicitely
>>
>> explicitely -> explicitly
>>
>> + int nnewmembers;
>> + Oid *oldmembers;
>> + Oid *newmembers;
>>
>> I wonder if naming nnewmembers newmembercount would be more readable.
>>
>> For pg_variable_aclcheck:
>>
>> + return ACLCHECK_OK;
>> + else
>> + return ACLCHECK_NO_PRIV;
>>
>> The 'else' can be omitted.
>>
>> + * Ownership check for a schema variables (specified by OID).
>>
>> 'a schema variable' (no s)
>>
>> For NamesFromList():
>>
>> + if (IsA(n, String))
>> + {
>> + result = lappend(result, n);
>> + }
>> + else
>> + break;
>>
>> There would be no more name if current n is not a String ?
>>
>> + * both variants, and returns InvalidOid with not_uniq flag,
>> when
>>
>> 'and return' (no s)
>>
>> + return InvalidOid;
>> + }
>> + else if (OidIsValid(varoid_without_attr))
>>
>> 'else' is not needed (since the if block ends with return).
>>
>> For clean_cache_callback(),
>>
>> + if (hash_search(schemavarhashtab,
>> + (void *) &svar->varid,
>> + HASH_REMOVE,
>> + NULL) == NULL)
>> + elog(DEBUG1, "hash table corrupted");
>>
>> Maybe add more information to the debug, such as var name.
>> Should we come out of the while loop in this scenario ?
>>
>> Cheers
>>
>