Thread: 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.
+ <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
+ 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;
+ 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;
+ 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;
+ {
+ 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 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");
+ (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
Hi,
This is continuation of the previous review.
+ * We should to use schema variable buffer, when
+ * it is available.
+ * 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
+ * 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@yugabyte.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 explicitelyexplicitely -> 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
Hi
ne 20. 12. 2020 v 20:24 odesílatel Zhihong Yu <zyu@yugabyte.com> napsal:
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.
although I prefer positive designed variables, in this case this negative form is better due consistency with other system tables
postgres=# select table_name, column_name from information_schema.columns where column_name like '%null';
┌──────────────┬──────────────┐
│ table_name │ column_name │
╞══════════════╪══════════════╡
│ pg_type │ typnotnull │
│ pg_attribute │ attnotnull │
│ pg_variable │ varisnotnull │
└──────────────┴──────────────┘
(3 rows)
┌──────────────┬──────────────┐
│ table_name │ column_name │
╞══════════════╪══════════════╡
│ pg_type │ typnotnull │
│ pg_attribute │ attnotnull │
│ pg_variable │ varisnotnull │
└──────────────┴──────────────┘
(3 rows)
+ <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.
Surely not - these three possibilities are supported and tested
CREATE VARIABLE t1 AS int DEFAULT -1 ON TRANSACTION END RESET
CREATE TEMP VARIABLE g AS int ON COMMIT DROP;
+ The <application>PL/pgSQL</application> language has not packages
+ and then it has not package variables and package constants. The'has not' -> 'has no'
fixed
+ a null value. A variable created as NOT NULL and without an explicitelyexplicitely -> explicitly
fixed
+ int nnewmembers;
+ Oid *oldmembers;
+ Oid *newmembers;I wonder if naming nnewmembers newmembercount would be more readable.
It is not bad idea, but nnewmembers is used more times on more places, and then this refactoring should be done in independent patch
For pg_variable_aclcheck:+ return ACLCHECK_OK;
+ else
+ return ACLCHECK_NO_PRIV;The 'else' can be omitted.
again - this pattern is used more often in related source file, and I used it for consistency with other functions. It is not visible from the patch, but when you check the related file, it will be clean.
+ * 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 ?
It tries to derive the name of a variable from the target list. Usually there can be only strings, but there can be array subscripting too (A_indices node)
I wrote a comment there.
+ * 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).
I understand. The `else` is not necessary, but I think so it is better due symmetry
if ()
{
return ..
}
else if ()
{
return ..
}
else
{
return ..
}
This style is used more times in Postgres, and usually I prefer it, when I want to emphasize so all ways have some similar pattern. My opinion about it is not too strong, The functionality is same, and I think so readability is a little bit better (due symmetry) (but I understand well so this can be subjective).
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 ?
I checked other places, and the same pattern is used in this text. If there are problems, then the problem is not with some specific schema variable, but in implementation of a hash table.
Maybe it is better to ignore the result (I did it), like it is done on some other places. This part is implementation of some simple garbage collector, and is not important if value was removed this or different way. I changed comments for this routine.
Regards
Pavel
Cheers
ne 20. 12. 2020 v 21:43 odesílatel Zhihong Yu <zyu@yugabyte.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.
<--><--> */
<--><--> * 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 usingsimply 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
CheersOn Sun, Dec 20, 2020 at 11:25 AM Zhihong Yu <zyu@yugabyte.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 explicitelyexplicitely -> 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