Re: logical replication worker accesses catalogs in error context callback

On Sat, Jul 3, 2021 at 2:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Didn't look at 0002 yet.
>
> ... and now that I have, I don't like it much.  It adds a lot of
> complexity, plus some lookup cycles that might be wasted.  I experimented
> with looking into the range table as I suggested previously, and
> that seems to work; see attached.  (This includes a little bit of
> code cleanup along with the bug fix proper.)
>
> An interesting point here is that the range table data will represent
> table and column aliases, not necessarily their true names.  I don't
> find that wrong, it's just different from what the code presently
> does.  If we go with this, likely we should change the plain-relation
> code path so that it also prints aliases from the RTE instead of
> the actual names.

Thanks. This patch is way simpler than what I proposed. Also, I like
the generic message "processing expression at position %d in select
list" when relname or attname is not available.

The patch basically looks good to me except a minor comment to have a
local variable for var->varattno which makes the code shorter.
                if (IsA(tle->expr, Var))
                {
-                       RangeTblEntry *rte;
                        Var                *var = (Var *) tle->expr;
+                       AttrNumber      attno = var->varattno;
+                       RangeTblEntry *rte = exec_rt_fetch(var->varno, estate);

-                       rte = exec_rt_fetch(var->varno, estate);
+                       relname = rte->eref->aliasname;

-                       if (var->varattno == 0)
+                       if (attno == 0)
                                is_wholerow = true;
-                       else
-                               attname = get_attname(rte->relid,
var->varattno, false);
-
-                       relname = get_rel_name(rte->relid);
+                       else if (attno > 0 && attno <=
list_length(rte->eref->colnames))
+                               attname =
strVal(list_nth(rte->eref->colnames, attno - 1));
                }

Regards,
Bharath Rupireddy.