Re: A problem with dump/restore of views containing whole row references - Mailing list pgsql-hackers

Abbas Butt <abbas.butt@enterprisedb.com> writes:
> On Fri, Apr 27, 2012 at 11:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> More generally, it seems rather inelegant to be forcibly adding a cast
>> when in most cases the existing notation is not wrong.  AFAICS the
>> plain "relname" notation is only ambiguous if there is a column of the
>> same name as the relation.  I wonder whether we should instead address
>> this by not letting the parser strip the "no op" cast in the first
>> place.

> You mean that the parser should not strip the "no op" cast in all cases or
> in the case only when the parser somehow detects a column of the same name
> as the relation?

On reflection that's the wrong thing anyway.  While (AFAICS) one could
only initially create this type of situation by using an explicit cast
as in your example, the ambiguity could be introduced after the fact by
a rename or ALTER TABLE ADD COLUMN, which wouldn't even have to affect
the troublesome table itself --- a column matching the table's name
anywhere in the FROM clause would create the same ambiguity.  So there's
no guarantee that there ever was a cast there.

So I think that your patch is the right approach, if wrong in detail.
What we have to do is stop using the ambiguous table-name-only syntax,
and instead always print tabname.*, and then add a cast to prevent
expansion of the "*" if we are at top level of a SELECT targetlist.

Attached is a patch that I think does this correctly.  I renamed the
flag parameter (and flipped its sense) since it is no longer controlling
whether or not a "*" gets printed.  One thing I like about this is that
whole-row Vars are no longer ever special in terms of naming; looking
at the code with a fresh eye, I wonder whether we didn't have other bugs
here in cases such as where a schema qualification is needed.  Omitting
the star is just asking for trouble ...

            regards, tom lane

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 3beed37dd26d6fc51a4365b3f4086b97f71cc16c..7ad99a0ec32579760f6e10278ec8585a0b2ab855 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** static void get_rule_orderby(List *order
*** 209,215 ****
  static void get_rule_windowclause(Query *query, deparse_context *context);
  static void get_rule_windowspec(WindowClause *wc, List *targetList,
                      deparse_context *context);
! static char *get_variable(Var *var, int levelsup, bool showstar,
               deparse_context *context);
  static RangeTblEntry *find_rte_by_refname(const char *refname,
                      deparse_context *context);
--- 209,215 ----
  static void get_rule_windowclause(Query *query, deparse_context *context);
  static void get_rule_windowspec(WindowClause *wc, List *targetList,
                      deparse_context *context);
! static char *get_variable(Var *var, int levelsup, bool istoplevel,
               deparse_context *context);
  static RangeTblEntry *find_rte_by_refname(const char *refname,
                      deparse_context *context);
*************** get_target_list(List *targetList, depars
*** 3074,3084 ****
           * "foo.*", which is the preferred notation in most contexts, but at
           * the top level of a SELECT list it's not right (the parser will
           * expand that notation into multiple columns, yielding behavior
!          * different from a whole-row Var).  We want just "foo", instead.
           */
          if (tle->expr && IsA(tle->expr, Var))
          {
!             attname = get_variable((Var *) tle->expr, 0, false, context);
          }
          else
          {
--- 3074,3085 ----
           * "foo.*", which is the preferred notation in most contexts, but at
           * the top level of a SELECT list it's not right (the parser will
           * expand that notation into multiple columns, yielding behavior
!          * different from a whole-row Var).  We need to call get_variable
!          * directly so that we can tell it to do the right thing.
           */
          if (tle->expr && IsA(tle->expr, Var))
          {
!             attname = get_variable((Var *) tle->expr, 0, true, context);
          }
          else
          {
*************** get_utility_query_def(Query *query, depa
*** 3803,3815 ****
   * the Var's varlevelsup has to be interpreted with respect to a context
   * above the current one; levelsup indicates the offset.
   *
!  * If showstar is TRUE, whole-row Vars are displayed as "foo.*";
!  * if FALSE, merely as "foo".
   *
!  * Returns the attname of the Var, or NULL if not determinable.
   */
  static char *
! get_variable(Var *var, int levelsup, bool showstar, deparse_context *context)
  {
      StringInfo    buf = context->buf;
      RangeTblEntry *rte;
--- 3804,3823 ----
   * the Var's varlevelsup has to be interpreted with respect to a context
   * above the current one; levelsup indicates the offset.
   *
!  * If istoplevel is TRUE, the Var is at the top level of a SELECT's
!  * targetlist, which means we need special treatment of whole-row Vars.
!  * Instead of the normal "tab.*", we'll print "tab.*::typename", which is a
!  * dirty hack to prevent "tab.*" from being expanded into multiple columns.
!  * (The parser will strip the useless coercion, so no inefficiency is added in
!  * dump and reload.)  We used to print just "tab" in such cases, but that is
!  * ambiguous and will yield the wrong result if "tab" is also a plain column
!  * name in the query.
   *
!  * Returns the attname of the Var, or NULL if the Var has no attname (because
!  * it is a whole-row Var).
   */
  static char *
! get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context)
  {
      StringInfo    buf = context->buf;
      RangeTblEntry *rte;
*************** get_variable(Var *var, int levelsup, boo
*** 3998,4007 ****
                  if (IsA(aliasvar, Var))
                  {
                      return get_variable(aliasvar, var->varlevelsup + levelsup,
!                                         showstar, context);
                  }
              }
!             /* Unnamed join has neither schemaname nor refname */
              refname = NULL;
          }
      }
--- 4006,4021 ----
                  if (IsA(aliasvar, Var))
                  {
                      return get_variable(aliasvar, var->varlevelsup + levelsup,
!                                         istoplevel, context);
                  }
              }
!
!             /*
!              * Unnamed join has neither schemaname nor refname.  (Note: since
!              * it's unnamed, there is no way the user could have referenced it
!              * to create a whole-row Var for it.  So we don't have to cover
!              * that case below.)
!              */
              refname = NULL;
          }
      }
*************** get_variable(Var *var, int levelsup, boo
*** 4017,4029 ****
              appendStringInfo(buf, "%s.",
                               quote_identifier(schemaname));
          appendStringInfoString(buf, quote_identifier(refname));
!         if (attname || showstar)
!             appendStringInfoChar(buf, '.');
      }
      if (attname)
          appendStringInfoString(buf, quote_identifier(attname));
!     else if (showstar)
          appendStringInfoChar(buf, '*');

      return attname;
  }
--- 4031,4048 ----
              appendStringInfo(buf, "%s.",
                               quote_identifier(schemaname));
          appendStringInfoString(buf, quote_identifier(refname));
!         appendStringInfoChar(buf, '.');
      }
      if (attname)
          appendStringInfoString(buf, quote_identifier(attname));
!     else
!     {
          appendStringInfoChar(buf, '*');
+         if (istoplevel)
+             appendStringInfo(buf, "::%s",
+                              format_type_with_typemod(var->vartype,
+                                                       var->vartypmod));
+     }

      return attname;
  }
*************** get_rule_expr(Node *node, deparse_contex
*** 4974,4980 ****
      switch (nodeTag(node))
      {
          case T_Var:
!             (void) get_variable((Var *) node, 0, true, context);
              break;

          case T_Const:
--- 4993,4999 ----
      switch (nodeTag(node))
      {
          case T_Var:
!             (void) get_variable((Var *) node, 0, false, context);
              break;

          case T_Const: