Re: proposal: schema variables

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



+      <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 explicitely

explicitely -> 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

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Konstantin Knizhnik
Дата:
Сообщение: Re: On login trigger: take three
Следующее
От: Amit Kapila
Дата:
Сообщение: Re: [Patch] Optimize dropping of relation buffers using dlist