Re: logical replication worker accesses catalogs in error context callback

On Sat, Jul 3, 2021 at 2:19 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.