Re: proposal: schema variables

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