Re: proposal: schema variables
От | Pavel Stehule |
---|---|
Тема | Re: proposal: schema variables |
Дата | |
Msg-id | CAFj8pRAgWYrAf7xBHYUVoB+hD37gpu4aAC=_QkmpehtrHK-oNQ@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: proposal: schema variables (Zhihong Yu <zyu@yugabyte.com>) |
Список | pgsql-hackers |
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
Вложения
В списке pgsql-hackers по дате отправления: