>> The get_formatted_string function is needed. Instead of using multiple if > statements for the pretty flag, it’s better to have a generic function. > This will be useful if the pretty-format DDL implementation is accepted by > the community, as it can be reused by other pg_get_<object>_ddl() DDL > functions added in the future. > >> >> in pg_get_triggerdef_worker, I found the below code pattern: >> /* >> * In non-pretty mode, always schema-qualify the target table name for >> * safety. In pretty mode, schema-qualify only if not visible. >> */ >> appendStringInfo(&buf, " ON %s ", >> pretty ? >> generate_relation_name(trigrec->tgrelid, NIL) : >> generate_qualified_relation_name(trigrec->tgrelid)); >> >> maybe we can apply it too while construct query string: >> "CREATE POLICY %s ON %s", >> > > In my opinion, the table name should always be schema-qualified, which I > have addressed in the v4 patch. The implementation of the pretty flag > here differs from pg_get_triggerdef_worker, which is used only for the > target table name. In my patch, the pretty flag adds \t and \n to each > different clause (example AS, FOR, USING ...) > >
I think that's where the confusion lies with adding `pretty` to the DDL functions. The `pretty` flag set to `true` on other functions is used to "not" show the schema name. But in the case of this function, it is used to format the statement. I wonder if the word `format` or `pretty_format` is a better name? `pretty` itself in the codebase isn't a very clear name regardless. -- Best, Phil Alger
