Thread: pgsql: Avoid doing catalog lookups in postgres_fdw's conversion_error_c

Avoid doing catalog lookups in postgres_fdw's conversion_error_callback.

As in 50371df26, this is a bad idea since the callback can't really
know what error is being thrown and thus whether or not it is safe
to attempt catalog accesses.  Rather than pushing said accesses into
the mainline code where they'd usually be a waste of cycles, we can
look at the query's rangetable instead.

This change does mean that we'll be printing query aliases (if any
were used) rather than the table or column's true name.  But that
doesn't seem like a bad thing: it's certainly a more useful definition
in self-join cases, for instance.  In any case, it seems unlikely that
any applications would be depending on this detail, so it seems safe
to change.

Patch by me.  Original complaint by Andres Freund; Bharath Rupireddy
noted the connection to conversion_error_callback.

Discussion: https://postgr.es/m/20210106020229.ne5xnuu6wlondjpe@alap3.anarazel.de

Branch
------
REL_11_STABLE

Details
-------
https://git.postgresql.org/pg/commitdiff/a9460dbf1577da440aa8c947d97b04875f61aee5

Modified Files
--------------
contrib/postgres_fdw/expected/postgres_fdw.out | 14 +++--
contrib/postgres_fdw/postgres_fdw.c            | 82 ++++++++++++--------------
contrib/postgres_fdw/sql/postgres_fdw.sql      |  8 ++-
3 files changed, 51 insertions(+), 53 deletions(-)

Hi!

> 6 июля 2021 г., в 19:36, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
>
> Avoid doing catalog lookups in postgres_fdw's conversion_error_callback.

I'm observing a related coredump in

+   ForeignScan *fsplan = castNode(ForeignScan, fsstate->ss.ps.plan);

#0  conversion_error_callback (arg=0x7ffe445cdf40) at ./build/../contrib/postgres_fdw/postgres_fdw.c:6455
#1  0x000055a51eb108c7 in errfinish (dummy=dummy@entry=0) at ./build/../src/backend/utils/error/elog.c:435
#2  0x000055a51eae07c5 in varchar_input (
    s=0x55a520b54c24 "--skipped user data--"..., len=516, atttypmod=<optimized out>) at
./build/../src/backend/utils/adt/varchar.c:471
#3  0x000055a51eb166f6 in InputFunctionCall (flinfo=0x55a520b26dc8,
    str=0x55a520b54c24 "--skipped user data--"..., typioparam=<optimized out>, typmod=<optimized out>) at
./build/../src/backend/utils/fmgr/fmgr.c:1548
#4  0x00007f373d0d06b9 in make_tuple_from_result_row (res=res@entry=0x55a5208145e0, row=row@entry=92, rel=<optimized
out>,attinmeta=0x55a520a148b8, retrieved_attrs=<optimized out>, 
    fsstate=fsstate@entry=0x0, temp_context=0x55a520893150) at ./build/../contrib/postgres_fdw/postgres_fdw.c:6377
#5  0x00007f373d0d0c3e in analyze_row_processor (astate=0x7ffe445ce060, row=92, res=0x55a5208145e0) at
./build/../contrib/postgres_fdw/postgres_fdw.c:4664
#6  postgresAcquireSampleRowsFunc (relation=relation@entry=0x55a520a8e278, elevel=elevel@entry=13,
rows=rows@entry=0x55a520c43c38,targrows=targrows@entry=300000, 
    totalrows=totalrows@entry=0x7ffe445ce348, totaldeadrows=totaldeadrows@entry=0x7ffe445ce350) at
./build/../contrib/postgres_fdw/postgres_fdw.c:4566
#7  0x000055a51e7ffdad in do_analyze_rel (onerel=onerel@entry=0x55a520a8e278, params=params@entry=0x7ffe445ce7c0,
va_cols=va_cols@entry=0x0,
    acquirefunc=0x7f373d0d0830 <postgresAcquireSampleRowsFunc>, relpages=272774, inh=inh@entry=false,
in_outer_xact=true,elevel=13) at ./build/../src/backend/commands/analyze.c:502 
#8  0x000055a51e8013d4 in analyze_rel (relid=<optimized out>, relation=<optimized out>,
params=params@entry=0x7ffe445ce7c0,va_cols=0x0, in_outer_xact=<optimized out>, 
    bstrategy=<optimized out>) at ./build/../src/backend/commands/analyze.c:260
#9  0x000055a51e87660b in vacuum (relations=0x55a520845e10, params=params@entry=0x7ffe445ce7c0, bstrategy=<optimized
out>,bstrategy@entry=0x0, isTopLevel=isTopLevel@entry=true) 
    at ./build/../src/backend/commands/vacuum.c:413
#10 0x000055a51e876c59 in ExecVacuum (pstate=pstate@entry=0x55a52085d660, vacstmt=vacstmt@entry=0x55a5209789e0,
isTopLevel=isTopLevel@entry=true)
    at ./build/../src/backend/commands/vacuum.c:199

If this is known issue, sorry for the noise.

Thanks!

Best regards, Andrey Borodin.

Andrey Borodin <x4mmm@yandex-team.ru> writes:
> I'm observing a related coredump in
> +   ForeignScan *fsplan = castNode(ForeignScan, fsstate->ss.ps.plan);

Could we have a test case for that please?  It seems to be related
to ANALYZE, but I'm unexcited about reverse-engineering the details.

            regards, tom lane

> 5 окт. 2021 г., в 22:08, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
>
> Andrey Borodin <x4mmm@yandex-team.ru> writes:
>> I'm observing a related coredump in
>> +   ForeignScan *fsplan = castNode(ForeignScan, fsstate->ss.ps.plan);
>
> Could we have a test case for that please?  It seems to be related
> to ANALYZE, but I'm unexcited about reverse-engineering the details.

Sure, I'll compose a repro a bit later.
It's an "ANALYZE (SKIP_LOCKED)" of a foreign table containing data that triggered varchar_input() to use
ereport(ERROR).

Thanks!

Best regards, Andrey Borodin.

Andrey Borodin <x4mmm@yandex-team.ru> writes:
> 5 окт. 2021 г., в 22:08, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
>> Could we have a test case for that please?  It seems to be related
>> to ANALYZE, but I'm unexcited about reverse-engineering the details.

> Sure, I'll compose a repro a bit later.

Ah, never mind, I found I can duplicate it just by adding an ANALYZE
in the stanza of postgres_fdw.sql that's already checking for
conversion-error cases.  Will fix.

            regards, tom lane

> 6 окт. 2021 г., в 21:36, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
> Will fix.

Thanks Tom!

Best regards, Andrey Borodin.