On 17.03.23 21:50, Pavel Stehule wrote:
> rebase + fix-update pg_dump tests
I have a few comments on the code:
0001
ExecGrant_Variable() could probably use ExecGrant_common().
The additions to syscache.c should be formatted to the new style.
in pg_variable.h:
- create_lsn ought to have a "var" prefix.
- typo: "typmode for variable's type"
- What is the purpose of struct Variable? It seems very similar to
FormData_pg_variable. At least a comment would be useful.
Preserve the trailing comma in ParseExprKind.
0002
expr_kind_allows_session_variables() should have some explanation
about criteria for determining which expression kinds should allow
variables.
Usually, we handle EXPR_KIND_* switches without default case, so we
get notified what needs to be changed if a new enum symbol is added.
0010
The material from the tutorial (advanced.sgml) might be better in
ddl.sgml.
In catalogs.sgml, the columns don't match the ones actually defined in
pg_variable.h in patch 0001 (e.g., create_lsn is missing and the order
doesn't match).
(The order of columns in pg_variable.h didn't immediately make sense to
me either, so maybe there is a middle ground to be found.)
session_variables_ambiguity_warning: There needs to be more
information about this. The current explanation is basically just,
"warn if your query is confusing". Why do I want that? Why would I
not want that? What is the alternative? What are some examples?
Shouldn't there be a standard behavior without a need to configure
anything?
In allfiles.sgml, dropVariable should be before dropView.
