pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[PG19][PATCH] Make postgres_fdw statistics import atomic
12+ messages / 5 participants
[nested] [flat]
* [PG19][PATCH] Make postgres_fdw statistics import atomic
@ 2026-09-15 09:36 Nikolay Samokhvalov <nik@postgres.ai>
2026-09-16 15:48 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Nathan Bossart <nathandbossart@gmail.com>
2026-09-16 20:45 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
0 siblings, 2 replies; 12+ messages in thread
From: Nikolay Samokhvalov @ 2026-09-15 09:36 UTC (permalink / raw)
To: pgsql-hackers <pgsql-hackers@lists.postgresql.org>; +Cc: etsuro.fujita@gmail.com, Corey Huinker <corey.huinker@gmail.com>; ashutosh.bapat.oss@gmail.com
Hi hackers,
postgres_fdw can leave partial pg_statistic changes behind when statistics
import fails and analyze falls back to sampling.
My AI correctness harness found this while I was testing new PG19 features.
It also prepared and tested the attached patch. I have not fully reviewed
that patch by hand because I am testing many PG19 areas in parallel. I
nevertheless think it is useful to post: I mostly trust this harness, and
the exact reproducer and patch passed its independent execution and review
gates. Please treat the patch as AI-prepared and review it in the usual way.
import_fetched_statistics() deletes and updates one attribute at a time.
attribute_statistics_update_internal() can update a partial row and return
false after a conversion warning. Earlier attributes have already been
updated too. If the fallback sample is empty, analyze does not replace
attribute statistics, so those changes commit.
Here is a complete reproducer. Run it with psql as a superuser against a
fresh PG19 server that accepts a loopback connection on its Unix socket:
create extension postgres_fdw;
create table remote_t (a, b) as values (11, '21'::text), (11, '21');
analyze remote_t;
create server loopback foreign data wrapper postgres_fdw
options (host :'HOST', port :'PORT', dbname :'DBNAME');
create user mapping for current_user server loopback options (user :'USER');
create foreign table ft (a int, b int) server loopback
options (table_name 'remote_t', import_stats 'true');
analyze ft;
update remote_t set a = 111, b = 'bad-x';
analyze remote_t;
delete from remote_t;
analyze ft;
select attname, coalesce(most_common_vals::text, 'NULL') as mcv
from pg_stats
where tablename = 'ft'
order by attname;
On REL_19_STABLE at e7c1b57012b the decisive output is below (psql's file
and line prefixes are omitted):
WARNING: invalid input syntax for type integer: "bad-x"
WARNING: could not import statistics for foreign table "public.ft"
--- attribute statistics import failed for column "b" of this foreign
table
attname | mcv
---------+-------
a | {111}
b | NULL
(2 rows)
The attached patch runs the local catalog import in an internal
subtransaction. It releases the subtransaction on success and rolls it
back when the import returns false. Errors are rolled back and rethrown.
The remote fetch stays outside the subtransaction.
The new regression test fails without the code change with the same partial
update. With the patch, the old coherent statistics survive:
attname | mcv
---------+------
a | {11}
b | {21}
(2 rows)
postgres_fdw regression and isolation tests pass. I also applied the exact
attachment to a clean REL_19_STABLE checkout and reran both suites.
Nik
Attachments:
[application/octet-stream] 0001-postgres_fdw-Make-statistics-import-atomic.patch (6.7K, ../../CAM527d-gHX4zZOhi9vvX+OsgEb+b2+fPbLUq8XRjAPYWQxDWjw@mail.gmail.com/2-0001-postgres_fdw-Make-statistics-import-atomic.patch)
download | inline diff:
From 1c8a042f385ca8fbc5d8873b297224ab3a494ca9 Mon Sep 17 00:00:00 2001
From: Nik Samokhvalov <nik@postgres.ai>
Date: Mon, 14 Sep 2026 19:53:26 -0700
Subject: [PATCH] postgres_fdw: Make statistics import atomic
---
.../postgres_fdw/expected/postgres_fdw.out | 34 +++++++++++++++
contrib/postgres_fdw/postgres_fdw.c | 43 ++++++++++++++++++-
contrib/postgres_fdw/sql/postgres_fdw.sql | 29 +++++++++++++
3 files changed, 104 insertions(+), 2 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 0cf84fee8..923b8d456 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -13270,6 +13270,38 @@ WHERE schemaname = 'public' AND tablename = 'dtest_ftable';
---------+-----------+-----------+-----------+------------+-----+-------------------+----+-------------
(0 rows)
+-- A failed import must not leave partial attribute statistics behind for an
+-- empty sampling fallback.
+CREATE TABLE simport_atomicity_table (a int, b text);
+CREATE FOREIGN TABLE simport_atomicity_ftable (a int, b int)
+ SERVER loopback OPTIONS (table_name 'simport_atomicity_table',
+ import_stats 'true');
+INSERT INTO simport_atomicity_table
+SELECT CASE WHEN i <= 9 THEN 11 ELSE 12 END,
+ CASE WHEN i <= 8 THEN '21' ELSE '22' END
+FROM generate_series(1, 10) i;
+ANALYZE simport_atomicity_table;
+ANALYZE simport_atomicity_ftable;
+TRUNCATE simport_atomicity_table;
+INSERT INTO simport_atomicity_table
+SELECT CASE WHEN i <= 7 THEN 111 ELSE 112 END,
+ CASE WHEN i <= 6 THEN 'not-an-integer-x' ELSE 'not-an-integer-y' END
+FROM generate_series(1, 10) i;
+ANALYZE simport_atomicity_table;
+DELETE FROM simport_atomicity_table;
+ANALYZE simport_atomicity_ftable;
+WARNING: invalid input syntax for type integer: "not-an-integer-x"
+WARNING: could not import statistics for foreign table "public.simport_atomicity_ftable" --- attribute statistics import failed for column "b" of this foreign table
+SELECT attname, most_common_vals::text
+FROM pg_stats
+WHERE schemaname = 'public' AND tablename = 'simport_atomicity_ftable'
+ORDER BY attname;
+ attname | most_common_vals
+---------+------------------
+ a | {11}
+ b | {21,22}
+(2 rows)
+
-- cleanup
DROP FOREIGN TABLE simport_ftable;
DROP FOREIGN TABLE simport_fview;
@@ -13279,6 +13311,8 @@ DROP FOREIGN TABLE simport_fpt;
DROP TABLE simport_pt;
DROP FOREIGN TABLE dtest_ftable;
DROP TABLE dtest_table;
+DROP FOREIGN TABLE simport_atomicity_ftable;
+DROP TABLE simport_atomicity_table;
-- ===================================================================
-- test for postgres_fdw_get_connections function with check_conn = true
-- ===================================================================
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 61e4e6fb2..775c407db 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -17,6 +17,7 @@
#include "access/htup_details.h"
#include "access/sysattr.h"
#include "access/table.h"
+#include "access/xact.h"
#include "catalog/pg_opfamily.h"
#include "commands/defrem.h"
#include "commands/explain_format.h"
@@ -51,6 +52,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/rel.h"
+#include "utils/resowner.h"
#include "utils/sampling.h"
#include "utils/selfuncs.h"
#include "utils/timestamp.h"
@@ -5538,8 +5540,45 @@ postgresImportForeignStatistics(Relation relation, List *va_cols, int elevel)
&remstats, &remattrmap, &attrcnt);
if (ok)
- ok = import_fetched_statistics(relation, schemaname, relname,
- &remstats, remattrmap, attrcnt);
+ {
+ MemoryContext oldcontext = CurrentMemoryContext;
+ ResourceOwner oldowner = CurrentResourceOwner;
+
+ /*
+ * Import the fetched statistics atomically. An attribute conversion
+ * failure is reported by returning false, after possibly updating
+ * that attribute and any preceding attributes. Roll those changes
+ * back so they cannot leak into the sampling fallback.
+ */
+ BeginInternalSubTransaction(NULL);
+ MemoryContextSwitchTo(oldcontext);
+
+ PG_TRY();
+ {
+ ok = import_fetched_statistics(relation, schemaname, relname,
+ &remstats, remattrmap, attrcnt);
+
+ if (ok)
+ ReleaseCurrentSubTransaction();
+ else
+ RollbackAndReleaseCurrentSubTransaction();
+ MemoryContextSwitchTo(oldcontext);
+ CurrentResourceOwner = oldowner;
+ }
+ PG_CATCH();
+ {
+ ErrorData *edata;
+
+ MemoryContextSwitchTo(oldcontext);
+ edata = CopyErrorData();
+ FlushErrorState();
+ RollbackAndReleaseCurrentSubTransaction();
+ MemoryContextSwitchTo(oldcontext);
+ CurrentResourceOwner = oldowner;
+ ReThrowError(edata);
+ }
+ PG_END_TRY();
+ }
if (ok)
{
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 3f6ea7b13..d7652b1cf 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -4740,6 +4740,33 @@ SELECT attname, inherited, null_frac, avg_width, n_distinct,
FROM pg_stats
WHERE schemaname = 'public' AND tablename = 'dtest_ftable';
+-- A failed import must not leave partial attribute statistics behind for an
+-- empty sampling fallback.
+CREATE TABLE simport_atomicity_table (a int, b text);
+CREATE FOREIGN TABLE simport_atomicity_ftable (a int, b int)
+ SERVER loopback OPTIONS (table_name 'simport_atomicity_table',
+ import_stats 'true');
+INSERT INTO simport_atomicity_table
+SELECT CASE WHEN i <= 9 THEN 11 ELSE 12 END,
+ CASE WHEN i <= 8 THEN '21' ELSE '22' END
+FROM generate_series(1, 10) i;
+ANALYZE simport_atomicity_table;
+ANALYZE simport_atomicity_ftable;
+
+TRUNCATE simport_atomicity_table;
+INSERT INTO simport_atomicity_table
+SELECT CASE WHEN i <= 7 THEN 111 ELSE 112 END,
+ CASE WHEN i <= 6 THEN 'not-an-integer-x' ELSE 'not-an-integer-y' END
+FROM generate_series(1, 10) i;
+ANALYZE simport_atomicity_table;
+DELETE FROM simport_atomicity_table;
+ANALYZE simport_atomicity_ftable;
+
+SELECT attname, most_common_vals::text
+FROM pg_stats
+WHERE schemaname = 'public' AND tablename = 'simport_atomicity_ftable'
+ORDER BY attname;
+
-- cleanup
DROP FOREIGN TABLE simport_ftable;
DROP FOREIGN TABLE simport_fview;
@@ -4749,6 +4776,8 @@ DROP FOREIGN TABLE simport_fpt;
DROP TABLE simport_pt;
DROP FOREIGN TABLE dtest_ftable;
DROP TABLE dtest_table;
+DROP FOREIGN TABLE simport_atomicity_ftable;
+DROP TABLE simport_atomicity_table;
-- ===================================================================
-- test for postgres_fdw_get_connections function with check_conn = true
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PG19][PATCH] Make postgres_fdw statistics import atomic
2026-09-15 09:36 [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
@ 2026-09-16 15:48 ` Nathan Bossart <nathandbossart@gmail.com>
1 sibling, 0 replies; 12+ messages in thread
From: Nathan Bossart @ 2026-09-16 15:48 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>; etsuro.fujita@gmail.com, Corey Huinker <corey.huinker@gmail.com>; ashutosh.bapat.oss@gmail.com
[RMT hat]
On Tue, Sep 15, 2026 at 02:36:14AM -0700, Nikolay Samokhvalov wrote:
> postgres_fdw can leave partial pg_statistic changes behind when statistics
> import fails and analyze falls back to sampling.
Corey and Fujita-san: this is listed as an open item for v19. Please
provide an update on its status as soon as you're able.
--
nathan
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PG19][PATCH] Make postgres_fdw statistics import atomic
2026-09-15 09:36 [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
@ 2026-09-16 20:45 ` Corey Huinker <corey.huinker@gmail.com>
2026-09-16 23:24 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-18 17:10 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Andres Freund <andres@anarazel.de>
1 sibling, 2 replies; 12+ messages in thread
From: Corey Huinker @ 2026-09-16 20:45 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>; etsuro.fujita@gmail.com; ashutosh.bapat.oss@gmail.com
On Tue, Sep 15, 2026 at 5:36 AM Nikolay Samokhvalov <nik@postgres.ai> wrote:
import_fetched_statistics() deletes and updates one attribute at a time.
> attribute_statistics_update_internal() can update a partial row and return
> false after a conversion warning. Earlier attributes have already been
> updated too. If the fallback sample is empty, analyze does not replace
> attribute statistics, so those changes commit.
>
So this situation happens when a remote table has pg_stats statistics, but
has no underlying data to sample. There are two ways that can happen:
1) The table was populated, analyzed, and then the rows were removed via
delete/truncate.
2) The table was not populated since last analysis or creation, but someone
used statistics import functions on it.
>
> Here is a complete reproducer. Run it with psql as a superuser against a
> fresh PG19 server that accepts a loopback connection on its Unix socket:
>
> create extension postgres_fdw;
> create table remote_t (a, b) as values (11, '21'::text), (11, '21');
>
Text field on source table.
> analyze remote_t;
>
> create server loopback foreign data wrapper postgres_fdw
> options (host :'HOST', port :'PORT', dbname :'DBNAME');
> create user mapping for current_user server loopback options (user
> :'USER');
> create foreign table ft (a int, b int) server loopback
options (table_name 'remote_t', import_stats 'true');
Integer field on destination table. Which means that if 'bad-x' was a
potential value that could have been sent over the wire for processing by
the FDW table and we did a regular query:
# update remote_t set a = 111, b = 'bad-x';
UPDATE 2
# SELECT * FROM ft;
ERROR: invalid input syntax for type integer: "bad-x"
CONTEXT: column "b" of foreign table "ft"
So already we're dealing with a very brittle situation. Not only is there a
type mismatch in the "b" columns that can cause ordinary queries to fail,
but we've shown that situation can and does happen.
As is, this will fail in production until someone remote comes along and
deletes the bad-x row, and the situation can re-occur until someone local
redefines or removes the "b" column.
> update remote_t set a = 111, b = 'bad-x';
> analyze remote_t;
> delete from remote_t;
>
So how would a table that didn't have stats import have handled this?
# create foreign table ft2 (a int, b int) server loopback options
(table_name 'remote_t');
CREATE FOREIGN TABLE
# analyze ft2;
ERROR: invalid input syntax for type integer: "bad-x"
CONTEXT: column "b" of foreign table "ft2"
And how would the first foreign table handle this if we fixed the records
in a way that didn't also leave the table empty?
# UPDATE remote_t SET b = 4;
UPDATE 2
# analyze ft;
WARNING: invalid input syntax for type integer: "bad-x"
WARNING: could not import statistics for foreign table "public.ft" ---
attribute statistics import failed for column "b" of this foreign table
ANALYZE
# SELECT attname, most_common_vals FROM pg_stats WHERE tablename = 'ft';
attname | most_common_vals
---------+------------------
a | {111}
b | {4}
(2 rows)
So fallback to sampling works as intended, but not if the remote table was
empty.
So this problem occurs only when a foreign table is mis-configured in such
a way as to create a conversion error on the inputs from the remote table's
stats, this went undiscovered (i.e. nobody queried the foreign table)
before the foreign table was analyzed, AND the remote table has been
emptied since it was analyzed but not yet re-analyzed at the time when the
remote table was analyzed.
And the negative consequence of this is that a query plan will assume that
the table contains rows when it actually does not. That seems like a
problem we already have in this situation.
CREATE TABLE ihaverows (x int);
INSERT INTO ihaverows SELECT g.g FROM generate_series(1,100000) AS g;
ANALYZE ihaverows;
DELETE FROM ihaverows;
SELECT relname, relpages, reltuples FROM pg_class where relname =
'ihaverows';
relname | relpages | reltuples
-----------+----------+-----------
ihaverows | 443 | 100000
(1 row)
So the impact is that until the remote table is re-analyzed and then the
foreign table is re-analyzed, we get over-estimates on an empty table. In a
real-world situation that table would be repopulated and reanalyzed fairly
quickly to restore business operations, and the bad stats issue corrects
itself, exactly as would happen if we truncated and reloaded a local table.
I'd call that low-risk, low-impact.
Furthermore, It's pretty reasonable to assume that the table will be
re-loaded with data shortly, in which case the "bad" stats and "bad" row
estimate would then be more accurate for the table which is populated but
not yet re-analyzed than would stale stats showing a table to be empty when
it actually has lots of rows. I mention this because this is the exact
scenario that caused me to want to create statistics import functions in
the first place: A partitioned table is being queried
frequently/continuously by an application. An ETL is loading millions of
rows into a partition that was empty just minutes ago. The stats for the
partition reported it as empty, so the query planner said because the
partition was empty, a table scan was the cheapest plan....oops. In this
situation (on an Oracle database) we determined that injecting the stats
from a populated partition into the empty partiion, while all wrong on
histograms, gave a better row estimate than the collected stats (0 rows)
and we knew the ETL would re-analyze the partition when it was done.
As for the C patch itself, it seems overengineered. It would be easier to
enhance the failure case when calling import_attribute_statistics to make a
note of where it was in the attribute loop, then walk backward calling
delete_pg_statistic() on the attributes that we had set before the failure.
But we shouldn't need to do that, as do_analyze_rel() should already handle
cleaning up pg_statistics rows for a table that used to have rows but now
doesn't....so I'd need to dig more to determine if this corner case only
happens if the foreign table had never had a successful analyze before the
bad data was inserted, but this dive is probably already deeper than most
people wanted, so I'm going to take a decompression stop here.
Summary: I can't see how this would happen at all in a foreign table that
correctly represented the remote data. Even then, it only happens if the
table is analyzed in a specific middle step of remediating the remote data,
and during that time the impact is low (no data loss, no incorrect
queries), and would resolve itself after the expected remediation. If this
needs a fix at all, it's probably not unique to postgres_fdw, so I
speculate that we'd want to teach do_analyze_rel() to know that a table
that analyzed as empty, but had an attempted remote stats fetch will need
to be zeroed out even if it already looks zeroed out.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PG19][PATCH] Make postgres_fdw statistics import atomic
2026-09-15 09:36 [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-16 20:45 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
@ 2026-09-16 23:24 ` Nikolay Samokhvalov <nik@postgres.ai>
2026-09-17 11:12 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Etsuro Fujita <etsuro.fujita@gmail.com>
1 sibling, 1 reply; 12+ messages in thread
From: Nikolay Samokhvalov @ 2026-09-16 23:24 UTC (permalink / raw)
To: Corey Huinker <corey.huinker@gmail.com>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>; etsuro.fujita@gmail.com; ashutosh.bapat.oss@gmail.com
Thanks Corey. Shouldn't an empty fallback preserve the old
pg_statistic rows, as analyze normally does? Here a={11}, b={21}
becomes a={111}, b=NULL; deleting the processed rows would not restore
the old state. Nik
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PG19][PATCH] Make postgres_fdw statistics import atomic
2026-09-15 09:36 [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-16 20:45 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
2026-09-16 23:24 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
@ 2026-09-17 11:12 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-09-17 15:34 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
0 siblings, 1 reply; 12+ messages in thread
From: Etsuro Fujita @ 2026-09-17 11:12 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; +Cc: Corey Huinker <corey.huinker@gmail.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; ashutosh.bapat.oss@gmail.com
On Thu, Sep 17, 2026 at 8:24 AM Nikolay Samokhvalov <nik@postgres.ai> wrote:
> Thanks Corey. Shouldn't an empty fallback preserve the old
> pg_statistic rows, as analyze normally does?
I don't think so, because if the fallback sample is empty, we have
reltuples=0 in pg_class, meaning that any attribute stats are
effectively ignored in planning.
Also, I think the scenario you showed upthread is not supported, or at
least not recommended:
* You declared the type of a column of the foreign table differently
from the remote table, but that isn't recommended, as noted in the
documentation: "It is generally recommended that the columns of a
foreign table be declared with exactly the same data types, and
collations if applicable, as the referenced columns of the remote
table..."
* You imported remote stats without re-analyzing the remote table
after the delete operation, but that isn't supported, as noted in the
documentation; "When using this option, it is the user's
responsibility to ensure that the existing statistics for the remote
table are up-to-date."
And I think any surprising behavior arising from such a use is the
user's fault rather than the system's fault.
Thanks for the testing!
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PG19][PATCH] Make postgres_fdw statistics import atomic
2026-09-15 09:36 [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-16 20:45 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
2026-09-16 23:24 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-17 11:12 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Etsuro Fujita <etsuro.fujita@gmail.com>
@ 2026-09-17 15:34 ` Nikolay Samokhvalov <nik@postgres.ai>
2026-09-18 09:15 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Nikolay Samokhvalov @ 2026-09-17 15:34 UTC (permalink / raw)
To: Etsuro Fujita <etsuro.fujita@gmail.com>; +Cc: Corey Huinker <corey.huinker@gmail.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; ashutosh.bapat.oss@gmail.com
On Thu, Sep 17, 2026, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> * You imported remote stats without re-analyzing the remote table
> after the delete operation, but that isn't supported, as noted in the
> documentation; "When using this option, it is the user's
> responsibility to ensure that the existing statistics for the remote
> table are up-to-date."
Thanks Etsuro. Agreed, my reproducer violates that requirement.
I haven't reproduced this with matching types and current remote
stats, so I'll withdraw the patch for now.
Attribute stats can still affect join estimates when reltuples=0;
I tested that. But that doesn't show this needs fixing.
Nik
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PG19][PATCH] Make postgres_fdw statistics import atomic
2026-09-15 09:36 [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-16 20:45 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
2026-09-16 23:24 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-17 11:12 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Etsuro Fujita <etsuro.fujita@gmail.com>
2026-09-17 15:34 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
@ 2026-09-18 09:15 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-09-23 10:23 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Etsuro Fujita @ 2026-09-18 09:15 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; +Cc: Corey Huinker <corey.huinker@gmail.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; ashutosh.bapat.oss@gmail.com
On Fri, Sep 18, 2026 at 12:35 AM Nikolay Samokhvalov <nik@postgres.ai> wrote:
> On Thu, Sep 17, 2026, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > * You imported remote stats without re-analyzing the remote table
> > after the delete operation, but that isn't supported, as noted in the
> > documentation; "When using this option, it is the user's
> > responsibility to ensure that the existing statistics for the remote
> > table are up-to-date."
>
> Thanks Etsuro. Agreed, my reproducer violates that requirement.
> I haven't reproduced this with matching types and current remote
> stats, so I'll withdraw the patch for now.
>
> Attribute stats can still affect join estimates when reltuples=0;
> I tested that. But that doesn't show this needs fixing.
Ok, I will move this open item to Non-bugs if no objections.
Thanks for checking!
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PG19][PATCH] Make postgres_fdw statistics import atomic
2026-09-15 09:36 [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-16 20:45 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
2026-09-16 23:24 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-17 11:12 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Etsuro Fujita <etsuro.fujita@gmail.com>
2026-09-17 15:34 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-18 09:15 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Etsuro Fujita <etsuro.fujita@gmail.com>
@ 2026-09-23 10:23 ` Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 0 replies; 12+ messages in thread
From: Etsuro Fujita @ 2026-09-23 10:23 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; +Cc: Corey Huinker <corey.huinker@gmail.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; ashutosh.bapat.oss@gmail.com
On Fri, Sep 18, 2026 at 6:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> Ok, I will move this open item to Non-bugs if no objections.
Done.
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PG19][PATCH] Make postgres_fdw statistics import atomic
2026-09-15 09:36 [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-16 20:45 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
@ 2026-09-18 17:10 ` Andres Freund <andres@anarazel.de>
2026-09-19 05:00 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
1 sibling, 1 reply; 12+ messages in thread
From: Andres Freund @ 2026-09-18 17:10 UTC (permalink / raw)
To: Corey Huinker <corey.huinker@gmail.com>; +Cc: Nikolay Samokhvalov <nik@postgres.ai>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; etsuro.fujita@gmail.com; ashutosh.bapat.oss@gmail.com
Hi,
On 2026-09-16 16:45:26 -0400, Corey Huinker wrote:
> And how would the first foreign table handle this if we fixed the records
> in a way that didn't also leave the table empty?
>
> # UPDATE remote_t SET b = 4;
> UPDATE 2
> # analyze ft;
> WARNING: invalid input syntax for type integer: "bad-x"
> WARNING: could not import statistics for foreign table "public.ft" ---
> attribute statistics import failed for column "b" of this foreign table
What is the defense of making all these warnings rather than errors? It's one
thing to e.g. warn that analyze skipped a relation due to locks, but doing
some catalog updates but not doing everything that the catalog updates
depended on seems like a really bad idea. Transactions exist for a reason...
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PG19][PATCH] Make postgres_fdw statistics import atomic
2026-09-15 09:36 [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-16 20:45 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
2026-09-18 17:10 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Andres Freund <andres@anarazel.de>
@ 2026-09-19 05:00 ` Corey Huinker <corey.huinker@gmail.com>
2026-09-19 10:55 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Corey Huinker @ 2026-09-19 05:00 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Nikolay Samokhvalov <nik@postgres.ai>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; etsuro.fujita@gmail.com; ashutosh.bapat.oss@gmail.com
>
> What is the defense of making all these warnings rather than errors? It's
> one
> thing to e.g. warn that analyze skipped a relation due to locks, but doing
> some catalog updates but not doing everything that the catalog updates
> depended on seems like a really bad idea. Transactions exist for a
> reason...
>
This is more an explanation than a defense, but here it goes...
It is that way because that's what the existing statistics import functions
(relation and attribute) do: they treat certain errors (cannot find that
table/column, cannot modify stats while in recovery, etc) as regular
errors, but in the case of the statistical values parameters they attempt
to make sense of them, and if some of the stats do not make sense (example:
most common values provided but most common frequencies not _also_
provided) it issues a warning and bypasses that stat, while still trying to
set other stat values for that column.
That was the desired behavior for pg_upgrade/pg_restore, where we didn't
want to stop the whole operation because the stats values somehow didn't
line up with expectations of a newer version.
A bit of historical trivia, there were patches that had two different
functions that did treat everything as an error, those were the pg_set_*
functions, but those were discarded in the development process in favor of
just using the pg_restore_* functions.
Each restore function does, however, return whether or not the import of
stats went flawlessly or not, and this flag allows stats import for foreign
data wrapper tables to fall back to sampling if the stats were less than
immaculate.
In the normal run of things, these bad remote stats would be rejected, and
the bad remote data would have also failed, exposing that the schema of the
foreign table had violated first normal form as well as the recommended
practice of making the foreign table columns identical to the remote table
column. Had the remote table user done a remediation that left at least
some rows in the table, then the table sample would have overwritten the
stats rows that were written, but in this corner case no rows remained in
the table and do_analyze_rel() decides to leave the existing stats as-is,
oblivious to the changes already made.
That's the explanation, what follows is a recap of options of what we can
do in the future.
That decision of the analyze.c code to leave existing stats as-is when it
gets an empty table sample is curious to me, as I'm not sure how ANALYZE
could ever reflect when a foreign table is actually empty once it has been
populated at least once, and if that's genuinely the case then perhaps we
should address that, but that would have implications outside of this
feature, so I'm highly reluctant to do that.
I can foresee several possible courses of action if we choose to reopen
this item.
1. Do nothing, as this is a corner case resulting from a misdesigned
foreign table and a remote table in an explicitly unsupported state
(modified to empty but not analyzed), and the situation will resolve itself
when the remote table is repopulated, or analyzed, or the column data types
are brought into alignment, whichever comes first.
2. Consider whether do_analyze_rel should do something (like clear the
pg_statistic rows for the relation) in the case where numrows returned from
the acquirefunc is zero. If I'm reading it correctly, there's no way for
ANALYZE to set stats on a truly empty table, though I can see where it
would make practical sense to assume that a truly empty table is only
temporarily empty, and query plans for a table with rows in it when the
stats stay it's empty are much worse than the query plans for an empty
table that the stats say still has rows, and thus keep the known bad stats
around because they're likely to be fixed soonish.
3. Add in a subtransaction and rollback like Nikolai's patch did, though
oddly enough we wouldn't want the try/catch part of it, because the
decision to roll back lies entirely with the boolean result from the
function, and we would not want the actual ERROR-level errors to be caught.
4. Do a substransaction but inside analyze_rel(), as we'd want this
behavior for all FDWs that do stats import, not just postgres_fdw.
5. Instead of a subtransaction, we could instead opt to clear the
pg_statistics for the relation and zero out the relpages/reltuples,
basically presuming that the table is empty until told otherwise by the
next step, which regular sampling. If sampling succeeds, then we get all of
our relation/attribute stats back, and if it doesn't then there is
something preventing regular querying of the table, so the 0 rows reported
isn't that far off.
Option 1 is where we're at now. It leaves open the chance of inconsistent
stats data being shown for a table designed in a such a way that ordinary
queries can generate errors AND someone has taken steps to hinder its
remediation.
Option 2 requires more investigation to determine why we opt to leave stats
as is when we get a numrows = 0 sample. Is it because we assume that all
methods of emptying a table already wipe out stats (e.g. truncate)? Is it
because we're hoping the table gets repopulated soonish? The viability of
this option depends on the answers to those questions.
Option 3 has the most control as it allows us to start the subtransaction
only when we are at risk of actually wanting to roll back rows, and it
gives us the most chances to avoid the overhead of the subtransaction (we'd
only start it once we knew that the table had the option enabled and we
successfully fetched stats data for all the columns), but it's still
subtransaction overhead to cover a case that shouldn't happen and whose
negative consequences are minimal.
Option 4 feels cleaner than Option 3, but it currently lacks the controls
to see whether the table in question is configured to import stats, so we'd
be incurring subtransaction overhead for all tables from FDWs that support
the feature whether or not the table is using the feature.
Option 5 avoids the subtransaction, which removes the overhead from
properly configured tables and all other situations that don't match the
corner case. The only downside is that we have to document the change in
behavior.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PG19][PATCH] Make postgres_fdw statistics import atomic
2026-09-15 09:36 [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-16 20:45 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
2026-09-18 17:10 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Andres Freund <andres@anarazel.de>
2026-09-19 05:00 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
@ 2026-09-19 10:55 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-09-23 10:21 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Etsuro Fujita @ 2026-09-19 10:55 UTC (permalink / raw)
To: Corey Huinker <corey.huinker@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; Nikolay Samokhvalov <nik@postgres.ai>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; ashutosh.bapat.oss@gmail.com
On Sat, Sep 19, 2026 at 2:00 PM Corey Huinker <corey.huinker@gmail.com> wrote:
>>
>> What is the defense of making all these warnings rather than errors? It's one
>> thing to e.g. warn that analyze skipped a relation due to locks, but doing
>> some catalog updates but not doing everything that the catalog updates
>> depended on seems like a really bad idea. Transactions exist for a reason...
>
> This is more an explanation than a defense, but here it goes...
Thanks for the detailed explanation!
> That's the explanation, what follows is a recap of options of what we can do in the future.
>
> That decision of the analyze.c code to leave existing stats as-is when it gets an empty table sample is curious to me, as I'm not sure how ANALYZE could ever reflect when a foreign table is actually empty once it has been populated at least once, and if that's genuinely the case then perhaps we should address that, but that would have implications outside of this feature, so I'm highly reluctant to do that.
Me too. Users should observe the restrictions when using
postgres_fdw, not just this feature.
> I can foresee several possible courses of action if we choose to reopen this item.
>
> 1. Do nothing, as this is a corner case resulting from a misdesigned foreign table and a remote table in an explicitly unsupported state (modified to empty but not analyzed), and the situation will resolve itself when the remote table is repopulated, or analyzed, or the column data types are brought into alignment, whichever comes first.
+1
> 2. Consider whether do_analyze_rel should do something (like clear the pg_statistic rows for the relation) in the case where numrows returned from the acquirefunc is zero.
I also thought this option; it would make things logically clean, but
I'm not sure we really need to do so, because in that case we set
reltuples=0 in pg_class, which makes the planner effectively ignore
the remaining attribute stats. See set_baserel_size_estimates(); if
reltuples=0, we have rel->tuples=0, so whatever value
clauselist_selectivity() calculates/returns based on the attribute
stats, rel->rows (the estimated number of output tuples from the base
relation) is set to zero.
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [PG19][PATCH] Make postgres_fdw statistics import atomic
2026-09-15 09:36 [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-16 20:45 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
2026-09-18 17:10 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Andres Freund <andres@anarazel.de>
2026-09-19 05:00 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Corey Huinker <corey.huinker@gmail.com>
2026-09-19 10:55 ` Re: [PG19][PATCH] Make postgres_fdw statistics import atomic Etsuro Fujita <etsuro.fujita@gmail.com>
@ 2026-09-23 10:21 ` Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 0 replies; 12+ messages in thread
From: Etsuro Fujita @ 2026-09-23 10:21 UTC (permalink / raw)
To: Corey Huinker <corey.huinker@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; Nikolay Samokhvalov <nik@postgres.ai>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; ashutosh.bapat.oss@gmail.com
On Sat, Sep 19, 2026 at 7:55 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > 2. Consider whether do_analyze_rel should do something (like clear the pg_statistic rows for the relation) in the case where numrows returned from the acquirefunc is zero.
>
> I also thought this option; it would make things logically clean, but
> I'm not sure we really need to do so, because in that case we set
> reltuples=0 in pg_class, which makes the planner effectively ignore
> the remaining attribute stats. See set_baserel_size_estimates(); if
> reltuples=0, we have rel->tuples=0, so whatever value
> clauselist_selectivity() calculates/returns based on the attribute
> stats, rel->rows (the estimated number of output tuples from the base
> relation) is set to zero.
Correction: since clamp_row_est() forces a row-count estimate to be at
least one row, we have rel->rows=1, not rel->rows=0. Sorry.
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2026-09-23 10:23 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-09-15 09:36 [PG19][PATCH] Make postgres_fdw statistics import atomic Nikolay Samokhvalov <nik@postgres.ai>
2026-09-16 15:48 ` Nathan Bossart <nathandbossart@gmail.com>
2026-09-16 20:45 ` Corey Huinker <corey.huinker@gmail.com>
2026-09-16 23:24 ` Nikolay Samokhvalov <nik@postgres.ai>
2026-09-17 11:12 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-09-17 15:34 ` Nikolay Samokhvalov <nik@postgres.ai>
2026-09-18 09:15 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-09-23 10:23 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-09-18 17:10 ` Andres Freund <andres@anarazel.de>
2026-09-19 05:00 ` Corey Huinker <corey.huinker@gmail.com>
2026-09-19 10:55 ` Etsuro Fujita <etsuro.fujita@gmail.com>
2026-09-23 10:21 ` Etsuro Fujita <etsuro.fujita@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox