On Thu, Feb 26, 2026 at 7:58 AM Matheus Alcantara
<[email protected]> wrote:
>
> On Wed Feb 25, 2026 at 10:39 PM -03, Masahiko Sawada wrote:
> >> Note that using COPY as the remote SQL is not always feasible. If the
> >> remote table has a trigger that modifies the row, and the local foreign
> >> table also has an insert trigger, we cannot capture those changes. While
> >> postgres_fdw typically relies on INSERT ... RETURNING * to synchronize
> >> the TupleTableSlot with remote side effects, the COPY command does not
> >> support a RETURNING clause. Without this synchronization, local triggers
> >> would see the original data rather than the actual values inserted. This
> >> limitation is why the ri_TrigDesc == NULL check is necessary; removing
> >> it causes the "Test a combination of local and remote triggers"
> >> regression test on postgres_fdw.sql to fail.
> >
> > Agreed. If this problem happens only when the local table has an AFTER
> > INSERT trigger, can we check ri_TrigDesc->trig_insert_after_row too?
> >
>
> Yes, it's better to only fallback to insert mode when the table have a
> AFTER trigger. Fixed.
>
> > Regarding the third condition, resultRelInfo->ri_returningList == NIL,
> > can we make it an Assert() because checking
> > resultRelInfo->RootResultRelInfo == NULL already checks if it's called
> > via COPY?
> >
>
> Yes, souns better. Fixed.
>
> > One thing it might be worth considering is to add some regression
> > tests verifying that COPY commands are actually being used on the
> > remote server in success cases. That way, we can be aware of changes
> > even if we change the assumption in the future that RootResultRelInfo
> > == NULL only when postgresBeginForeignInsert() is called via COPY. One
> > idea is to define a trigger on the remote server that checks if the
> > executed query is INSERT or COPY. For example,
> >
> > create function insert_or_copy() returns trigger as $$
> > declare query text;
> > begin
> > query := current_query();
> > if query ~* '^COPY' then
> > raise notice 'COPY command';
> > elsif query ~* '^INSERT' then
> > raise notice 'INSERT command';
> > end if;
> > return new;
> > end;
> > $$ language plpgsql;
> >
> > Note that we need to set client_min_message to 'log' so that we can
> > write the notice message raised via postgres_fdw.
> >
>
> Good, thanks for this! I've added on this new version.
>
> Please see the new attached version. Thank you for reviewing this!
>
Thank you for updating the patch! Here are some review comments:
+ Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum - 1);
+
+ if (attr->attgenerated)
+ continue;
+
+ if (!first)
+ appendStringInfoString(buf, ", ");
+
+ first = false;
+
+ appendStringInfoString(buf,
quote_identifier(NameStr(attr->attname)));
We need to take care of the 'column_name' option here. If it's set we
should not use attr->attname as it is.
--
+ }
+ if (nattrs > 0)
+ appendStringInfoString(buf, ") FROM STDIN");
+ else
+ appendStringInfoString(buf, " FROM STDIN");
+}
It might be better to explicitly specify the format 'text'.
---
+ if (useCopy)
+ {
+ deparseCopySql(&sql, rel, targetAttrs);
+ values_end_len = 0; /* Keep compiler quiet */
+ }
+ else
+ deparseInsertSql(&sql, rte, resultRelation, rel,
targetAttrs, doNothing,
+
resultRelInfo->ri_WithCheckOptions,
+
resultRelInfo->ri_returningList,
+ &retrieved_attrs,
&values_end_len);
I think we should consider whether it's okay to use the COPY command
even if resultRelInfo->ri_WithCheckOptions is non-NULL. As far as I
researched, it's okay as we currently don't support COPY to a view but
please consider it as well. We might want to explain it too in the
comment.
How about initializing values_end_len with 0 at its declaration?
---
+-- test that fdw also use COPY FROM as a remote sql
+set client_min_messages to 'log';
+
+create function insert_or_copy() returns trigger as $$
+declare query text;
+begin
+ query := current_query();
+ if query ~* '^COPY' then
+ raise notice 'COPY command';
+ elsif query ~* '^INSERT' then
+ raise notice 'INSERT command';
+ end if;
+return new;
+end;
+$$ language plpgsql;
On second thoughts, it might be okay to output the current_query() as it is.
---
+copy grem1 from stdin;
+3
+\.
I think it would be good to have more tests, for example, checking if
the COPY command method can work properly with batch_size and
column_name options.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
