From 3f88b080030682adf359248c9cb7f8b2068a539e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 24 Dec 2012 17:52:19 -0500 Subject: [PATCH] Fix some minor issues in view pretty-printing. Code review for commit 2f582f76b1945929ff07116cd4639747ce9bb8a1: don't use a static variable for what ought to be a deparse_context field, fix non-multibyte-safe test for spaces, avoid useless and potentially O(N^2) (though admittedly with a very small constant) calculations of wrap positions when we aren't going to wrap. --- doc/src/sgml/func.sgml | 11 +- src/backend/utils/adt/ruleutils.c | 215 ++++++++++++++++-------------- 2 files changed, 119 insertions(+), 107 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index e184d99626..35c7f75eab 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -13858,7 +13858,7 @@ SELECT pg_type_is_visible('myschema.widget'::regtype); pg_get_viewdef(view_name, pretty_bool) text - get underlying SELECT command for view, + get underlying SELECT command for view; lines with fields are wrapped to 80 columns if pretty_bool is true (deprecated) @@ -13869,14 +13869,15 @@ SELECT pg_type_is_visible('myschema.widget'::regtype); pg_get_viewdef(view_oid, pretty_bool) text - get underlying SELECT command for view, + get underlying SELECT command for view; lines with fields are wrapped to 80 columns if pretty_bool is true - pg_get_viewdef(view_oid, wrap_int) + pg_get_viewdef(view_oid, wrap_column_int) text - get underlying SELECT command for view, - wrapping lines with fields as specified, pretty printing is implied + get underlying SELECT command for view; + lines with fields are wrapped to specified number of columns, + pretty printing is implied pg_options_to_table(reloptions) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index b7aff1189b..f58418e0ce 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -74,7 +74,8 @@ #define PRETTYFLAG_PAREN 1 #define PRETTYFLAG_INDENT 2 -#define PRETTY_WRAP_DEFAULT 79 +/* Default line length for pretty-print wrapping */ +#define WRAP_COLUMN_DEFAULT 79 /* macro to test if pretty action needed */ #define PRETTY_PAREN(context) ((context)->prettyFlags & PRETTYFLAG_PAREN) @@ -94,6 +95,7 @@ typedef struct List *windowClause; /* Current query level's WINDOW clause */ List *windowTList; /* targetlist for resolving WINDOW clause */ int prettyFlags; /* enabling of pretty-print functions */ + int wrapColumn; /* max line length, or -1 for no limit */ int indentLevel; /* current indent level for prettyprint */ bool varprefix; /* TRUE to print prefixes on Vars */ } deparse_context; @@ -142,7 +144,6 @@ static SPIPlanPtr plan_getrulebyoid = NULL; static const char *query_getrulebyoid = "SELECT * FROM pg_catalog.pg_rewrite WHERE oid = $1"; static SPIPlanPtr plan_getviewrule = NULL; static const char *query_getviewrule = "SELECT * FROM pg_catalog.pg_rewrite WHERE ev_class = $1 AND rulename = $2"; -static int pretty_wrap = PRETTY_WRAP_DEFAULT; /* GUC parameters */ bool quote_all_identifiers = false; @@ -159,7 +160,8 @@ bool quote_all_identifiers = false; static char *deparse_expression_pretty(Node *expr, List *dpcontext, bool forceprefix, bool showimplicit, int prettyFlags, int startIndent); -static char *pg_get_viewdef_worker(Oid viewoid, int prettyFlags); +static char *pg_get_viewdef_worker(Oid viewoid, + int prettyFlags, int wrapColumn); static char *pg_get_triggerdef_worker(Oid trigid, bool pretty); static void decompile_column_index_array(Datum column_index_array, Oid relId, StringInfo buf); @@ -192,9 +194,10 @@ static void pop_ancestor_plan(deparse_namespace *dpns, static void make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, int prettyFlags); static void make_viewdef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, - int prettyFlags); + int prettyFlags, int wrapColumn); static void get_query_def(Query *query, StringInfo buf, List *parentnamespace, - TupleDesc resultDesc, int prettyFlags, int startIndent); + TupleDesc resultDesc, + int prettyFlags, int wrapColumn, int startIndent); static void get_values_def(List *values_lists, deparse_context *context); static void get_with_clause(Query *query, deparse_context *context); static void get_select_query_def(Query *query, deparse_context *context, @@ -374,7 +377,7 @@ pg_get_viewdef(PG_FUNCTION_ARGS) /* By OID */ Oid viewoid = PG_GETARG_OID(0); - PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, 0))); + PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, 0, -1))); } @@ -387,7 +390,7 @@ pg_get_viewdef_ext(PG_FUNCTION_ARGS) int prettyFlags; prettyFlags = pretty ? PRETTYFLAG_PAREN | PRETTYFLAG_INDENT : 0; - PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, prettyFlags))); + PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, prettyFlags, WRAP_COLUMN_DEFAULT))); } Datum @@ -401,9 +404,7 @@ pg_get_viewdef_wrap(PG_FUNCTION_ARGS) /* calling this implies we want pretty printing */ prettyFlags = PRETTYFLAG_PAREN | PRETTYFLAG_INDENT; - pretty_wrap = wrap; - result = pg_get_viewdef_worker(viewoid, prettyFlags); - pretty_wrap = PRETTY_WRAP_DEFAULT; + result = pg_get_viewdef_worker(viewoid, prettyFlags, wrap); PG_RETURN_TEXT_P(string_to_text(result)); } @@ -419,7 +420,7 @@ pg_get_viewdef_name(PG_FUNCTION_ARGS) viewrel = makeRangeVarFromNameList(textToQualifiedNameList(viewname)); viewoid = RangeVarGetRelid(viewrel, NoLock, false); - PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, 0))); + PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, 0, -1))); } @@ -439,14 +440,14 @@ pg_get_viewdef_name_ext(PG_FUNCTION_ARGS) viewrel = makeRangeVarFromNameList(textToQualifiedNameList(viewname)); viewoid = RangeVarGetRelid(viewrel, NoLock, false); - PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, prettyFlags))); + PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, prettyFlags, WRAP_COLUMN_DEFAULT))); } /* * Common code for by-OID and by-name variants of pg_get_viewdef */ static char * -pg_get_viewdef_worker(Oid viewoid, int prettyFlags) +pg_get_viewdef_worker(Oid viewoid, int prettyFlags, int wrapColumn) { Datum args[2]; char nulls[2]; @@ -504,7 +505,7 @@ pg_get_viewdef_worker(Oid viewoid, int prettyFlags) */ ruletup = SPI_tuptable->vals[0]; rulettc = SPI_tuptable->tupdesc; - make_viewdef(&buf, ruletup, rulettc, prettyFlags); + make_viewdef(&buf, ruletup, rulettc, prettyFlags, wrapColumn); } /* @@ -711,6 +712,7 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty) context.windowTList = NIL; context.varprefix = true; context.prettyFlags = pretty ? PRETTYFLAG_PAREN : 0; + context.wrapColumn = WRAP_COLUMN_DEFAULT; context.indentLevel = PRETTYINDENT_STD; get_rule_expr(qual, &context, false); @@ -2157,6 +2159,7 @@ deparse_expression_pretty(Node *expr, List *dpcontext, context.windowTList = NIL; context.varprefix = forceprefix; context.prettyFlags = prettyFlags; + context.wrapColumn = WRAP_COLUMN_DEFAULT; context.indentLevel = startIndent; get_rule_expr(expr, &context, showimplicit); @@ -2678,6 +2681,7 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, context.windowTList = NIL; context.varprefix = (list_length(query->rtable) != 1); context.prettyFlags = prettyFlags; + context.wrapColumn = WRAP_COLUMN_DEFAULT; context.indentLevel = PRETTYINDENT_STD; memset(&dpns, 0, sizeof(dpns)); @@ -2704,7 +2708,8 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, foreach(action, actions) { query = (Query *) lfirst(action); - get_query_def(query, buf, NIL, NULL, prettyFlags, 0); + get_query_def(query, buf, NIL, NULL, + prettyFlags, WRAP_COLUMN_DEFAULT, 0); if (prettyFlags) appendStringInfo(buf, ";\n"); else @@ -2721,7 +2726,8 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, Query *query; query = (Query *) linitial(actions); - get_query_def(query, buf, NIL, NULL, prettyFlags, 0); + get_query_def(query, buf, NIL, NULL, + prettyFlags, WRAP_COLUMN_DEFAULT, 0); appendStringInfo(buf, ";"); } } @@ -2734,7 +2740,7 @@ make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, */ static void make_viewdef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, - int prettyFlags) + int prettyFlags, int wrapColumn) { Query *query; char ev_type; @@ -2789,7 +2795,7 @@ make_viewdef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, ev_relation = heap_open(ev_class, AccessShareLock); get_query_def(query, buf, NIL, RelationGetDescr(ev_relation), - prettyFlags, 0); + prettyFlags, wrapColumn, 0); appendStringInfo(buf, ";"); heap_close(ev_relation, AccessShareLock); @@ -2805,7 +2811,8 @@ make_viewdef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc, */ static void get_query_def(Query *query, StringInfo buf, List *parentnamespace, - TupleDesc resultDesc, int prettyFlags, int startIndent) + TupleDesc resultDesc, + int prettyFlags, int wrapColumn, int startIndent) { deparse_context context; deparse_namespace dpns; @@ -2825,6 +2832,7 @@ get_query_def(Query *query, StringInfo buf, List *parentnamespace, context.varprefix = (parentnamespace != NIL || list_length(query->rtable) != 1); context.prettyFlags = prettyFlags; + context.wrapColumn = wrapColumn; context.indentLevel = startIndent; memset(&dpns, 0, sizeof(dpns)); @@ -2961,7 +2969,8 @@ get_with_clause(Query *query, deparse_context *context) if (PRETTY_INDENT(context)) appendContextKeyword(context, "", 0, 0, 0); get_query_def((Query *) cte->ctequery, buf, context->namespaces, NULL, - context->prettyFlags, context->indentLevel); + context->prettyFlags, context->wrapColumn, + context->indentLevel); if (PRETTY_INDENT(context)) appendContextKeyword(context, "", 0, 0, 0); appendStringInfoChar(buf, ')'); @@ -3218,10 +3227,14 @@ get_target_list(List *targetList, deparse_context *context, TupleDesc resultDesc) { StringInfo buf = context->buf; + StringInfoData targetbuf; + bool last_was_multiline = false; char *sep; int colno; ListCell *l; - bool last_was_multiline = false; + + /* we use targetbuf to hold each TLE's text temporarily */ + initStringInfo(&targetbuf); sep = " "; colno = 0; @@ -3230,10 +3243,6 @@ get_target_list(List *targetList, deparse_context *context, TargetEntry *tle = (TargetEntry *) lfirst(l); char *colname; char *attname; - StringInfoData targetbuf; - int leading_nl_pos = -1; - char *trailing_nl; - int pos; if (tle->resjunk) continue; /* ignore junk entries */ @@ -3243,11 +3252,10 @@ get_target_list(List *targetList, deparse_context *context, colno++; /* - * Put the new field spec into targetbuf so we can decide after we've + * Put the new field text into targetbuf so we can decide after we've * got it whether or not it needs to go on a new line. */ - - initStringInfo(&targetbuf); + resetStringInfo(&targetbuf); context->buf = &targetbuf; /* @@ -3288,63 +3296,60 @@ get_target_list(List *targetList, deparse_context *context, appendStringInfo(&targetbuf, " AS %s", quote_identifier(colname)); } - /* Restore context buffer */ - + /* Restore context's output buffer */ context->buf = buf; - /* Does the new field start with whitespace plus a new line? */ - - for (pos = 0; pos < targetbuf.len; pos++) + /* Consider line-wrapping if enabled */ + if (PRETTY_INDENT(context) && context->wrapColumn >= 0) { - if (targetbuf.data[pos] == '\n') + int leading_nl_pos = -1; + char *trailing_nl; + int pos; + + /* Does the new field start with whitespace plus a new line? */ + for (pos = 0; pos < targetbuf.len; pos++) { - leading_nl_pos = pos; - break; + if (targetbuf.data[pos] == '\n') + { + leading_nl_pos = pos; + break; + } + if (targetbuf.data[pos] != ' ') + break; } - if (targetbuf.data[pos] > ' ') - break; - } - /* Locate the start of the current line in the buffer */ - - trailing_nl = (strrchr(buf->data, '\n')); - if (trailing_nl == NULL) - trailing_nl = buf->data; - else - trailing_nl++; + /* Locate the start of the current line in the output buffer */ + trailing_nl = strrchr(buf->data, '\n'); + if (trailing_nl == NULL) + trailing_nl = buf->data; + else + trailing_nl++; - /* - * If the field we're adding is the first in the list, or it already - * has a leading newline, or wrap mode is disabled (pretty_wrap < 0), - * don't add anything. Otherwise, add a newline, plus some - * indentation, if either the new field would cause an overflow or the - * last field used more than one line. - */ + /* + * If the field we're adding is the first in the list, or it + * already has a leading newline, don't add anything. Otherwise, + * add a newline, plus some indentation, if either the new field + * would cause an overflow or the last field used more than one + * line. + */ + if (colno > 1 && + leading_nl_pos == -1 && + ((strlen(trailing_nl) + strlen(targetbuf.data) > context->wrapColumn) || + last_was_multiline)) + appendContextKeyword(context, "", -PRETTYINDENT_STD, + PRETTYINDENT_STD, PRETTYINDENT_VAR); - if (colno > 1 && - leading_nl_pos == -1 && - pretty_wrap >= 0 && - ((strlen(trailing_nl) + strlen(targetbuf.data) > pretty_wrap) || - last_was_multiline)) - { - appendContextKeyword(context, "", -PRETTYINDENT_STD, - PRETTYINDENT_STD, PRETTYINDENT_VAR); + /* Remember this field's multiline status for next iteration */ + last_was_multiline = + (strchr(targetbuf.data + leading_nl_pos + 1, '\n') != NULL); } /* Add the new field */ - appendStringInfoString(buf, targetbuf.data); - - - /* Keep track of this field's status for next iteration */ - - last_was_multiline = - (strchr(targetbuf.data + leading_nl_pos + 1, '\n') != NULL); - - /* cleanup */ - - pfree(targetbuf.data); } + + /* clean up */ + pfree(targetbuf.data); } static void @@ -3371,7 +3376,8 @@ get_setop_query(Node *setOp, Query *query, deparse_context *context, if (need_paren) appendStringInfoChar(buf, '('); get_query_def(subquery, buf, context->namespaces, resultDesc, - context->prettyFlags, context->indentLevel); + context->prettyFlags, context->wrapColumn, + context->indentLevel); if (need_paren) appendStringInfoChar(buf, ')'); } @@ -3787,7 +3793,8 @@ get_insert_query_def(Query *query, deparse_context *context) { /* Add the SELECT */ get_query_def(select_rte->subquery, buf, NIL, NULL, - context->prettyFlags, context->indentLevel); + context->prettyFlags, context->wrapColumn, + context->indentLevel); } else if (values_rte) { @@ -6639,7 +6646,8 @@ get_sublink_expr(SubLink *sublink, deparse_context *context) appendStringInfoChar(buf, '('); get_query_def(query, buf, context->namespaces, NULL, - context->prettyFlags, context->indentLevel); + context->prettyFlags, context->wrapColumn, + context->indentLevel); if (need_paren) appendStringInfo(buf, "))"); @@ -6693,47 +6701,49 @@ get_from_clause(Query *query, const char *prefix, deparse_context *context) } else { - StringInfoData targetbuf; - char *trailing_nl; + StringInfoData itembuf; appendStringInfoString(buf, ", "); - initStringInfo(&targetbuf); - context->buf = &targetbuf; + /* + * Put the new FROM item's text into itembuf so we can decide + * after we've got it whether or not it needs to go on a new line. + */ + initStringInfo(&itembuf); + context->buf = &itembuf; get_from_clause_item(jtnode, query, context); + /* Restore context's output buffer */ context->buf = buf; - /* Locate the start of the current line in the buffer */ - - trailing_nl = (strrchr(buf->data, '\n')); - if (trailing_nl == NULL) - trailing_nl = buf->data; - else - trailing_nl++; + /* Consider line-wrapping if enabled */ + if (PRETTY_INDENT(context) && context->wrapColumn >= 0) + { + char *trailing_nl; - /* - * Add a newline, plus some indentation, if pretty_wrap is on and - * the new from-clause item would cause an overflow. - */ + /* Locate the start of the current line in the buffer */ + trailing_nl = strrchr(buf->data, '\n'); + if (trailing_nl == NULL) + trailing_nl = buf->data; + else + trailing_nl++; - if (pretty_wrap >= 0 && - (strlen(trailing_nl) + strlen(targetbuf.data) > pretty_wrap)) - { - appendContextKeyword(context, "", -PRETTYINDENT_STD, - PRETTYINDENT_STD, PRETTYINDENT_VAR); + /* + * Add a newline, plus some indentation, if the new item would + * cause an overflow. + */ + if (strlen(trailing_nl) + strlen(itembuf.data) > context->wrapColumn) + appendContextKeyword(context, "", -PRETTYINDENT_STD, + PRETTYINDENT_STD, PRETTYINDENT_VAR); } /* Add the new item */ + appendStringInfoString(buf, itembuf.data); - appendStringInfoString(buf, targetbuf.data); - - /* cleanup */ - - pfree(targetbuf.data); + /* clean up */ + pfree(itembuf.data); } - } } @@ -6766,7 +6776,8 @@ get_from_clause_item(Node *jtnode, Query *query, deparse_context *context) /* Subquery RTE */ appendStringInfoChar(buf, '('); get_query_def(rte->subquery, buf, context->namespaces, NULL, - context->prettyFlags, context->indentLevel); + context->prettyFlags, context->wrapColumn, + context->indentLevel); appendStringInfoChar(buf, ')'); break; case RTE_FUNCTION: -- 2.40.0