Re: BUG #15552: Unexpected error in COPY to a foreign table in a transaction

Hi,

> heap_sync should only be called for relations that actually have files to
> sync, which isn't true for foreign tables. So, a simple relkind check
> before calling heap_sync() in CopyFrom would suffice I think. Although,
> we might also need such a check higher up in CopyFrom where some
> optimizations that are specific to "heap" relations are enabled. For
> example, this piece of code:

thanks for the input, so it seems that is enough with adding the check as you suggested:

if (cstate->rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE &&
cstate->rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE &&
(cstate->rel->rd_createSubid != InvalidSubTransactionId ||
cstate->rel->rd_newRelfilenodeSubid != InvalidSubTransactionId))
{
hi_options |= HEAP_INSERT_SKIP_FSM;
if (!XLogIsNeeded())
hi_options |= HEAP_INSERT_SKIP_WAL;
}

Should a regression test be added (at postgres_fdw.sql)? If yes, how should be implemented, as max_wal_senders and wal_level need to be changed (to 0 and minimal) to trigger the bug, which is only possible on server start.

Cheers,

Dr. Luis M. Carril Rodríguez
Senior Software Engineer
luis(dot)carril(at)swarm64(dot)com<mailto:lisa(at)swarm64(dot)com>

Swarm64 AS
Parkveien 41 B | 0258 Oslo | Norway
Registered at Brønnøysundregistrene in Norway under Org.-Number 911 662 787
CEO/Geschäftsführer (Daglig Leder): Dr. Karsten Rönner; Chairman/Vorsitzender (Styrets Leder): Dr. Sverre Munck

Swarm64 AS Zweigstelle Hive
Ullsteinstr. 120 | 12109 Berlin | Germany

Registered at Amtsgericht Charlottenburg - HRB 154382 B

[cid:9642fa37-86ca-4a3b-af9b-3a297ed0bc4b]
________________________________
From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Sent: Monday, December 17, 2018 3:18:14 AM
To: Luis Carril; pgsql-bugs(at)lists(dot)postgresql(dot)org; PG Bug reporting form
Subject: Re: BUG #15552: Unexpected error in COPY to a foreign table in a transaction

Hi,

On 2018/12/14 19:42, PG Bug reporting form wrote:
> The following bug has been logged on the website:
>
> Bug reference: 15552
> Logged by: Luis M Carril
> Email address: luis(dot)carril(at)swarm64(dot)com
> PostgreSQL version: 11.1
> Operating system: Ubuntu 16.04
> Description:
>
> Hi,
> Postgres throws a "could not open file" error when inside a transaction
> we create a foreign table and copy data into it.
>
> Reproduction (code based on tests in postgres_fdw test suite):
> -----------------------
> CREATE EXTENSION postgres_fdw;
>
> CREATE SERVER testserver1 FOREIGN DATA WRAPPER postgres_fdw;
> DO $d$
> BEGIN
> EXECUTE $$CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
> OPTIONS (dbname '$$||current_database()||$$',
> port '$$||current_setting('port')||$$'
> )$$;
> EXECUTE $$CREATE SERVER loopback2 FOREIGN DATA WRAPPER
> postgres_fdw
> OPTIONS (dbname '$$||current_database()||$$',
> port '$$||current_setting('port')||$$'
> )$$;
> END;
> $d$;
>
> CREATE USER MAPPING FOR public SERVER testserver1
> OPTIONS (user 'value', password 'value');
> CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
>
> create table loct1 (a int check (a in (1)), b text);
>
> begin;
> create foreign table remp1 (a int check (a in (1)), b text) server loopback
> options (table_name 'loct1');
> copy remp1 from stdin delimiter ',';
> 1,f
> \.
> -----------------------
>
> Observed behavior:
> ERROR: could not open file "base/16385/16460": No such file or
> directory
>
> -----------------------

Thanks for the report. I can reproduce this both in 11 stable branch and
in HEAD.

> For what I saw, the error is triggered when synchronizing the heap in
> CopyFrom (backend/commands/copy.c:2890), see the following stacktrace (when
> setting a breakpoint at errcode_for_file_access():
>
> #0 errcode_for_file_access () at
> /build/postgresql-11-9gVVK7/postgresql-11-11.1/build/../src/backend/utils/error/elog.c:600
> #1 0x00005636212d33d2 in mdopen (reln=<optimized out>,
> forknum=forknum(at)entry=MAIN_FORKNUM, behavior=behavior(at)entry=1)
> at
> /build/postgresql-11-9gVVK7/postgresql-11-11.1/build/../src/backend/storage/smgr/md.c:606
> #2 0x00005636212d3961 in mdopen (behavior=1, forknum=MAIN_FORKNUM,
> reln=0x563622ba8a88) at
> /build/postgresql-11-9gVVK7/postgresql-11-11.1/build/../src/backend/storage/smgr/md.c:922
> #3 mdnblocks (reln=0x563622ba8a88, forknum=MAIN_FORKNUM) at
> /build/postgresql-11-9gVVK7/postgresql-11-11.1/build/../src/backend/storage/smgr/md.c:875
> #4 0x00005636212d39b9 in mdimmedsync (reln=0x563622ba8a88,
> forknum=MAIN_FORKNUM) at
> /build/postgresql-11-9gVVK7/postgresql-11-11.1/build/../src/backend/storage/smgr/md.c:1033
> #5 0x000056362103257c in heap_sync (rel=0x7f0e3e681aa8) at
> /build/postgresql-11-9gVVK7/postgresql-11-11.1/build/../src/backend/access/heap/heapam.c:9408
> #6 0x0000563621115e89 in CopyFrom (cstate=cstate(at)entry=0x563622ba2040) at
> /build/postgresql-11-9gVVK7/postgresql-11-11.1/build/../src/backend/commands/copy.c:2890

[ ... ]

> If someone gives me a hint on the expected behavior here, I would gladly
> submit a patch myself.

heap_sync should only be called for relations that actually have files to
sync, which isn't true for foreign tables. So, a simple relkind check
before calling heap_sync() in CopyFrom would suffice I think. Although,
we might also need such a check higher up in CopyFrom where some
optimizations that are specific to "heap" relations are enabled. For
example, this piece of code:

if (cstate->rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE &&
(cstate->rel->rd_createSubid != InvalidSubTransactionId ||
cstate->rel->rd_newRelfilenodeSubid != InvalidSubTransactionId))
{
hi_options |= HEAP_INSERT_SKIP_FSM;
if (!XLogIsNeeded())
hi_options |= HEAP_INSERT_SKIP_WAL;
}

Note here that we check that the relation being copied into is not a
partitioned table, because a partitioned table doesn't have storage itself.

Thanks,
Amit