Re: proposal: schema variables

Hi

ne 20. 12. 2020 v 20:24 odesílatel Zhihong Yu <zyu(at)yugabyte(dot)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
>