Re: proposal: schema variables

Hi,
This is continuation of the previous review.

+ * We should to use schema variable buffer, when
+ * it is available.

'should use' (no to)

+ /* When buffer of used schema variables loaded from shared memory */

A verb seems missing in the above comment.

+ elog(ERROR, "unexpected non-SELECT command in LET ... SELECT");

Since non-SELECT is mentioned, I wonder if the trailing SELECT can be
omitted.

+ * 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'.

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
>