On Thu, Sep 10, 2026 at 2:21 PM Ajit Awekar <[email protected]> wrote:
>
> Hi Jakub,
>
> Thanks for the detailed repros. Attached patch fixes #1, #3, and #4;
> #2 as doc modification.
>
> #1/#3: The whole-row Var and per-column Vars overlapped, duplicating
> columns on the wire and misattributing conversion errors to the whole
> row instead of the failing column. Fixed by skipping the whole-row Var
> during deparse, fetching each column individually, and reconstructing
> the row locally -- restores per-column error context and drops the
> duplicate transfer.
>
> #4: Fixed by filtering to Vars of the scanned relation. Also fixed a
> zero-column-table assertion hit while testing this.
>
> #2: Added a note on this as a known limitation to the
> postgres_fdw "Remote Query Optimization" docs, recommending
> whole-query shipping where possible and limiting affected row counts
> otherwise.
>
> Verified against your repros plus the full regression.
>
Hi Ajit,
I've took a quick look:
0. This thing crashes with edge case of no having no columns (sic!) and using
trigger:
DROP TABLE t1;
DROP FOREIGN TABLE ft1;
CREATE TABLE t1 (a int);
INSERT INTO t1 VALUES (1),(2);
CREATE FOREIGN TABLE ft1 (a int) SERVER loopback OPTIONS (table_name 't1');
ALTER FOREIGN TABLE ft1 DROP COLUMN a; -- yay!
CREATE OR REPLACE FUNCTION trg() RETURNS trigger LANGUAGE plpgsql
AS $$ BEGIN RAISE NOTICE 'trg % old=% new=%', TG_OP, OLD, NEW; IF
TG_OP = 'DELETE' THEN RETURN OLD; END IF; RETURN NEW; END $$;
CREATE TRIGGER trg BEFORE DELETE ON ft1 FOR EACH ROW EXECUTE FUNCTION trg();
EXPLAIN (VERBOSE, COSTS OFF) DELETE FROM ft1 WHERE random() <= 1;
DELETE FROM ft1 WHERE random() <= 1;
causes
TRAP: failed Assert("relvalues != NULL"), File:
"../contrib/postgres_fdw/postgres_fdw.c", Line: 9268, PID: 382254
postgres: test postgres [local]
DELETE(ExceptionalCondition+0x74)[0x5ebb9182d4e4]
postgres_fdw.so(+0x1c0e6)[0x7ddec0d860e6]
postgres_fdw.so(+0x1c25f)[0x7ddec0d8625f]
postgres_fdw.so(+0x1f118)[0x7ddec0d89118]
postgres: test postgres [local] DELETE(+0x37e092)[0x5ebb91513092]
postgres: test postgres [local] DELETE(ExecScan+0x242)[0x5ebb914ffeb2]
[..]
I haven't investigated it further other than the EXPLAIN showing the
difference with trigger and without it like this:
- Output: ctid, remotetableoid
+ Output: ctid, remotetableoid, ft1.*
which seems to be caused by the trigger itself (w/o trigger it wont
fetch "ft1.*
but there are no columns there and so it will crash). Dunno if anybody would
use something like that (what's the point), but someone by accident could
remove all columns from fdw during maintenance and crashing whole server is
not good.
1. In doc/src/sgml/postgres-fdw.sgml
[..]
++ by first selecting the affected rows from the remote server and then
++ issuing a separate remote <command>UPDATE</command> or
++ <command>DELETE</command> for each row, identified by its
++ <literal>ctid</literal> and remote <literal>tableoid</literal>. If the
++ remote table is the root of a partitioning hierarchy, each such per-row
++ command targets that root and so cannot benefit from partition pruning,
++ which can make this considerably slower than the whole-query case when
perhaps it would read provide more details if the that last statement would
go refering tableoid explictly, I mean:
"If the remote table is the root of a partitioning hierarchy, each such per
-row command targets that root and so cannot benefit from partition pruning
>> (based on tableoid) <<, which can make this considerably slower than the
whole-query case (..)" (see those new statement between >> and <<)
2. The commitmsg is a bit way too verbose to my taste and covers everything.
As long as the source code provides explanations, I don't think we need to
duplicate information that much about implementation in the commitmsg. It
should also at least have Discussion: link, Authors , etc.
3. Wider question to the community: an open question persists that was earlier
raised by Etsuro and Michael, should this be backpatched or not (altough
they were discussing earlier iterations of the solution). With this patch
the varnullingrels used by this patch seems to be PG16+, and this thing
might alter the optimizer in some other ways. If that is not backpatchable
then what we should do in earlier versions? Etsuro had patch in [1] to
block dangerous thing (because it's bug/and we should block logical
corruption of the remote side, shouldn't we?).
BTW: this still needs to be reviewed by "the elders of the internet"^H^H^H^H^H
people that understand the optimizer :)
-J.
[1] -
https://www.postgresql.org/message-id/CAPmGK14KEFMTuQ1vYwWCo8SLks5rXv-56K-V%2BXMy4q8uQJvq1w%40mail.gmail.com
