Обсуждение: Invalid remote sampling test in postgres_fdw.

On Mon, Aug 11, 2025 at 10:23:47AM -0400, Corey Huinker wrote:
> Patch 0001 concerns what appears to be a bug in the postgres_fdw regression
> tests.
>
> As it was, the tests set the sampling method for the foreign server, but
> rather than analyzing a table on that foreign server, the local table
> analyze_table was analyzed instead.
>
> The analyze commands now analyze a remote table, and the remote table
> definition links it to the local table that was previously being analyzed
> repeatedly.

 CREATE FOREIGN TABLE analyze_ftable (id int, a text, b bigint)
-       SERVER loopback OPTIONS (table_name 'analyze_rtable1');
+       SERVER loopback OPTIONS (table_name 'analyze_table');

Good catch.  So what your patch is telling here is that
analyze_rtable1 does not exist.  I suspect a fuzz from a rebase.  Will
backpatch accordingly once the stable branches reopen.

> Patch 0002 isn't a bug, just an annoyance.
>
> It creates two psql variables which can then be interpolated into SQL
> commands as string literals, eliminating the need to wrap commands in a DO
> block. This makes the queries a bit more readable, and allows for any
> errors (however unlikely) in those commands to be reported individually,
> rather than grouped and potentially obfuscated by the DO block.

Agreed that using variables leads to a more readable result.  However,
I can see an argument against your proposal.  It's worth noting that
Sawada-san has been relying on the trick with the DO block a couple of
days ago to get an isolation test working, inspired from the existing
SQL test:
https://www.postgresql.org/message-id/CAD21AoCm1XYSD_R1capjuvLZ_3fKXnCu0C751Vk3hVOBDSwaCQ@mail.gmail.com

I would suggest waiting a bit for the resolution of the other bug, and
make sure that we have at least one reference to the trick with the DO
block.  While the way you are writing the test is more readable,
there's also a case for keeping documented this trick in the tree in
the shape of a SQL sequence that does not depend on \gset and psql.
--
Michael

On Tue, Aug 12, 2025 at 08:58:47AM +0900, Michael Paquier wrote:
>  CREATE FOREIGN TABLE analyze_ftable (id int, a text, b bigint)
> -       SERVER loopback OPTIONS (table_name 'analyze_rtable1');
> +       SERVER loopback OPTIONS (table_name 'analyze_table');
>
> Good catch.  So what your patch is telling here is that
> analyze_rtable1 does not exist.  I suspect a fuzz from a rebase.  Will
> backpatch accordingly once the stable branches reopen.

Applied 0001 down to v16 to fix the test, as of 327bd6111aed.
--
Michael

On Tue, Aug 12, 2025 at 08:58:47AM +0900, Michael Paquier wrote:
> I would suggest waiting a bit for the resolution of the other bug, and
> make sure that we have at least one reference to the trick with the DO
> block.  While the way you are writing the test is more readable,
> there's also a case for keeping documented this trick in the tree in
> the shape of a SQL sequence that does not depend on \gset and psql.

I was looking again an this thread, and this argument feels a bit off
because we have a test already posted on the thread, and there would
be some past history.  There is also the ease of debugging failures
when the commands fail in the DO block.  At the end, applied 0002 on
HEAD.
--
Michael