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