Queries may not be using ISO SQL where possible (W)

[dassl]

Queries may not be using ISO SQL where possible (W)

[afig]

I recall hearing "TEXT" is not in ISO SQL. Is NAME a std data type? Also is session_user in the std? Queries currently use session_user sometimes, current_user other times. Also TEXT and NAME are sometimes upper case, sometimes lower case

Sean Murthy (smurthys)

[afig]

TEXT is Postgres' implementation of the standard CLOB type (Essentially VARCHAR without a max length). NAME is an internal type used by the pg_catalog, I will rewrite the queries to remove it.

Steven Rollo (srrollo)

[smurthys]

Need to carefully review all data types (especially TEXT) and functions used: not saying there are errors, but it is important in the education context that every effort be made to use the correct types and queries. For example, should Student.lastDDLObject be TEXT?

Also, every non-standard use should be clearly documented in code.

[wildtayne]

pg_event_trigger_ddl_commands() returns TEXT for both object_identity (user for Student.lastDDLObject) and command_tag (used for Student.lastDDLOperation). source.
However, since object_identity returns a string made up of two identifiers, (schema.objname), and Postgres objects have a maximum length of 63 characters source, we can calculate that the maximum length of a string in Student.lastDDLObject will be 131 (63x2 for names + 2x2 for quotes + 1 for .)
Likewise, the longest statement tag that can trigger is 32 characters long source, so we can assume that the maximum length for Student.lastDDLOperation will be 32 characters.
I think we can replace TEXT in both of these fields with VARCHAR of appropriate length.

[wildtayne]

Here is a list of non-standard types used and some thoughts on replacing them:

prepareClassDB.sql
-Student.lastDDLObject TEXT: Can be replaced with VARCHAR(131)
-Student.lastDDLOperation TEXT: Can be replaced with VARCHAR(32)
-listUserConnectionsReturn.applicationName TEXT: This lists a string from the client identifying itself (ie. dBeaver sends DBeaver 4.0.7 - Main). I think this could safely be replaced with VARCHAR(63), since application_name uses NAME internally.
-killConnection INT4: INT4 is the input type for pg_terminate_backend, but it is probably safe to just use INT.

prepareUserLogging.sql
-classdb.postgresLog: uses mostly non-standard types, but I think this is OK since it is used for a completely Postgres specific feature, and the schema is recommended in the documentation. I also have a link to the documentation as a comment above the table.
-logPath TEXT: This will hold the full path for the log file. We can probably make this VARCHAR(4096), which seems to be the longest max path length for most Linux systems. Windows' max path length is only 260 characters.
-objID TEXT: This is the same as Student.lastDDLObject, so VARCHAR(63)

metaFunctions.sql
-All uses of TEXT in this script refer to fields that are stored as NAME internally, so they can be replaced with VARCHAR(63)

[smurthys]

With Student.lastDDL* let us safely add more space to take into account quoted identifiers, future changes, etc. I will make the changes to Student table since I am editing that file. I am thinking 256 and 63 for the two fields respectively.

I agree we leave the schema of classdb.postgres as is.

objID: I assume you mean VARCHAR(131) which is what you show for Student.lastDDLObject.

[wildtayne]

Yes, VARCHAR(131) for objID is what I meant

[smurthys]

In metaFunctions.sql, replace use of SETOF with TABLE for return type because "TABLE is more portable".

Using TABLE for return type also eliminates the need to define user-defined types.

[wildtayne]

One downside to TABLE notation is that the return type columns have to be explicitly enumerated for each function instead of referring to a user defined type. This results in some repeated code since each of the metaFunctions has an overload with the same return type. One way to refactor this would be to create one version of each function with appropriate default parameters, so the column definition list is not repeated.

Another place where SETOF can be replaced is classdb.listUserConnections in prepareClassDB.sql

[wildtayne]

I decided to go with the single function solution in 8479bc8. So far, I have only updated metaFunctions.sql. I will edit classdb.listUserConnections after the current PR.

[afig]

To keep the data types consistent, we should update the userName parameters in prepareClassDB and prepareClassServer to VARCHAR(63) instead of VARCHAR(50), in order to match the new changed types in PR #55.

[wildtayne]

Some more instances are located here

• classdb.listUserConnectionsReturn should be merged into classdb.listUserConnections using a TABLE return type
• classdb.killUserConnections should return TABLE(success BOOLEAN)

[wildtayne]

These instances were addressed in adf6656.