database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2026-10-23 13:29:09 +03:00
Code Activity

postgres_fdw: Fix issues with statistics import.

Browse Source 
When analyzing a foreign table pointing to a partitioned or inherited
table on the remote server, this method just imported relpages and
reltuples for the parent table from the remote pg_class, causing a
discrepancy or an error in results compared to the sampling method.

* In the partitioning case, while the sampling method sets relpages=0,
  this method set relpages=-1.  (For reltuples, the results of both
  methods match.)
* In the inheritance case, while the sampling method sets
  reltuples=(# of rows in the parent and child tables), this method set
  reltuples=(# of rows in the parent table).  (For relpages, the results
  of both methods match.)

As relpages and reltuples are used for costing foreign paths, these
would lead to incorrect plans or plan changes depending on the method.
To fix, do the following:

* For the partitioning case, modify postgres_fdw to set relpages=0 to
  match the sampling method.
* For the inheritance case, we could also modify it to set the correct
  reltuples value by fetching reltuples for the child tables as well,
  but 1) the change isn't small, and 2) table inheritance hasn't been
  used that much these days, so fix by disabling this method.

While at it, adjust comments and docs slightly.

Oversight in commit 28972b6fc.

Author: Etsuro Fujita <etsuro.fujita@gmail.com>
Discussion: https://postgr.es/m/CAPmGK173b2Xvyoeq%3DY7L09H%2BajgMgxC2fYe%3D7CCunZz8tGTfFA%40mail.gmail.com
Backpatch-through: 19
This commit is contained in:
Etsuro Fujita
2026-09-04 18:40:01 +09:00
parent 10a84539b9
commit f4012672bb
5 changed files with 108 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
contrib/postgres_fdw/deparse.c
View File

@@ -2562,11 +2562,14 @@ deparseAnalyzeSizeSql(StringInfo buf, Relation rel)
/*
* Construct SELECT statement to acquire the number of pages, the number of
* rows, and the relkind of a relation.
* rows, and the relkind/relhassubclass of a relation.
*
* Note: we just return the remote server's reltuples value, which might
* be off a good deal, but it doesn't seem worth working harder. See
* Note: we just return the remote server's relpages/reltuples values, which
* might be off a good deal, but it doesn't seem worth working harder. See
* comments in postgresAcquireSampleRowsFunc.
*
* Note: in the stats-import case it is the user's responsibility to ensure
* that those stats values are up-to-date.
*/
void
deparseAnalyzeInfoSql(StringInfo buf, Relation rel)
@@ -2577,7 +2580,7 @@ deparseAnalyzeInfoSql(StringInfo buf, Relation rel)
initStringInfo(&relname);
deparseRelation(&relname, rel);
appendStringInfoString(buf, "SELECT relpages, reltuples, relkind FROM pg_catalog.pg_class WHERE oid = ");
appendStringInfoString(buf, "SELECT relpages, reltuples, relkind, relhassubclass FROM pg_catalog.pg_class WHERE oid = ");
deparseStringLiteral(buf, relname.data);
appendStringInfoString(buf, "::pg_catalog.regclass");
}

27
contrib/postgres_fdw/expected/postgres_fdw.out
View File

@@ -13198,6 +13198,31 @@ CREATE FOREIGN TABLE simport_fview (c1 int, c2 text)
ALTER FOREIGN TABLE simport_fview OPTIONS (ADD import_stats 'true');
ANALYZE simport_fview; -- should fail
WARNING: could not import statistics for foreign table "public.simport_fview" --- remote table "public.simport_view" is of relkind "v" which cannot have statistics
CREATE TABLE simport_pt (c1 int not null, c2 text) PARTITION BY LIST (c1);
CREATE TABLE simport_p1 PARTITION OF simport_pt FOR VALUES IN (1);
CREATE TABLE simport_p2 PARTITION OF simport_pt FOR VALUES IN (2);
CREATE FOREIGN TABLE simport_fpt (c1 int not null, c2 text)
SERVER loopback OPTIONS (table_name 'simport_pt');
INSERT INTO simport_pt VALUES (1, 'foo'), (1, 'foo'), (2, 'bar'), (2, 'bar');
-- Check that we have relpages = 0 for simport_fpt regardless of the method
ANALYZE simport_fpt;
SELECT relpages FROM pg_class WHERE oid = 'public.simport_fpt'::regclass;
relpages
----------
0
(1 row)
ALTER FOREIGN TABLE simport_fpt OPTIONS (ADD import_stats 'true');
ANALYZE simport_pt;
ANALYZE VERBOSE simport_fpt; -- should work
INFO: importing statistics for foreign table "public.simport_fpt"
INFO: finished importing statistics for foreign table "public.simport_fpt"
SELECT relpages FROM pg_class WHERE oid = 'public.simport_fpt'::regclass;
relpages
----------
0
(1 row)
-- This tests build_remattrmap()'s deparsing of column names that include
-- single quotes or backslashes
CREATE TABLE dtest_table ("col'quote" int, "col\backslash" int);
@@ -13250,6 +13275,8 @@ DROP FOREIGN TABLE simport_ftable;
DROP FOREIGN TABLE simport_fview;
DROP VIEW simport_view;
DROP TABLE simport_table;
DROP FOREIGN TABLE simport_fpt;
DROP TABLE simport_pt;
DROP FOREIGN TABLE dtest_ftable;
DROP TABLE dtest_table;
-- ===================================================================

100
contrib/postgres_fdw/postgres_fdw.c
View File

@@ -329,6 +329,7 @@ enum RelStatsColumns
RELSTATS_RELPAGES = 0,
RELSTATS_RELTUPLES,
RELSTATS_RELKIND,
RELSTATS_RELHASSUBCLASS,
RELSTATS_NUM_FIELDS,
};
@@ -352,14 +353,14 @@ enum AttStatsColumns
ATTSTATS_NUM_FIELDS,
};
/* Result sets that are returned from a foreign statistics scan */
/* Results that are returned from a foreign statistics scan */
typedef struct
{
int version; /* version of remote server */
BlockNumber relpages; /* # of pages in remote table */
double reltuples; /* # of tuples in remote table */
PGresult *rel; /* result for relation stats query */
PGresult *att; /* result for attribute stats query */
double livetuples; /* livetuples estimates, for pgstat report */
double deadtuples; /* deadtuples estimates, for pgstat report */
int version; /* version of remote server */
} RemoteStatsResults;
/* Pairs of remote columns with local columns */
@@ -597,7 +598,6 @@ static bool import_fetched_statistics(Relation relation,
static char *get_opt_value(PGresult *res, int row, int col);
static void set_text_arg(NullableDatum *arg, const char *s);
static void set_int32_arg(NullableDatum *arg, const char *s);
static void set_uint32_arg(NullableDatum *arg, const char *s);
static void set_float_arg(NullableDatum *arg, const char *s);
static void set_floatarr_arg(NullableDatum *arg, const char *s);
static void produce_tuple_asynchronously(AsyncRequest *areq, bool fetch);
@@ -5065,7 +5065,7 @@ postgresGetAnalyzeInfoForForeignTable(Relation relation, bool *can_tablesample)
if (PQntuples(res) != 1 || PQnfields(res) != RELSTATS_NUM_FIELDS)
elog(ERROR, "unexpected result from deparseAnalyzeInfoSql query");
/* We don't use relpages here */
/* We don't use relpages/relhassubclass here */
reltuples = strtod(PQgetvalue(res, 0, RELSTATS_RELTUPLES), NULL);
relkind = *(PQgetvalue(res, 0, RELSTATS_RELKIND));
PQclear(res);
@@ -5542,8 +5542,7 @@ postgresImportForeignStatistics(Relation relation, List *va_cols, int elevel)
if (ok)
{
pgstat_report_analyze(relation,
remstats.livetuples, remstats.deadtuples,
pgstat_report_analyze(relation, remstats.reltuples, 0,
(va_cols == NIL), starttime);
ereport(elevel,
@@ -5637,6 +5636,23 @@ fetch_remote_statistics(Relation relation,
goto fetch_cleanup;
}
/*
* If the remote table is inherited, relpages/reltuples in pg_class for it
* provide stats for the parent table, not for the inheritance set. We
* could calculate stats for the set by fetching the relation stats for
* child tables as well; but for now, just fallback to sampling.
*/
if ((relkind == RELKIND_RELATION || relkind == RELKIND_FOREIGN_TABLE) &&
strcmp(PQgetvalue(relstats, 0, RELSTATS_RELHASSUBCLASS), "t") == 0)
{
ereport(WARNING,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("could not import statistics for foreign table \"%s.%s\" --- remote table \"%s.%s\" is inherited",
local_schemaname, local_relname,
remote_schemaname, remote_relname));
goto fetch_cleanup;
}
/*
* If the reltuples value > 0, then we can expect to find attribute stats
* for the remote table.
@@ -5650,7 +5666,8 @@ fetch_remote_statistics(Relation relation,
* that the table had never been analyzed, or that it was empty. Assuming
* the former, fallback to sampling.
*/
reltuples = strtod(PQgetvalue(relstats, 0, RELSTATS_RELTUPLES), NULL);
remstats->reltuples = reltuples =
strtod(PQgetvalue(relstats, 0, RELSTATS_RELTUPLES), NULL);
if (reltuples > 0)
{
RemoteAttributeMapping *remattrmap;
@@ -5690,9 +5707,15 @@ fetch_remote_statistics(Relation relation,
goto fetch_cleanup;
}
/* We assume that we have no dead tuple. */
remstats->deadtuples = 0.0;
remstats->livetuples = reltuples;
/*
* If the remote table is partitioned, import relpages = 0, to match the
* sampling case.
*/
if (relkind == RELKIND_PARTITIONED_TABLE)
remstats->relpages = 0;
else
remstats->relpages =
strtoul(PQgetvalue(relstats, 0, RELSTATS_RELPAGES), NULL, 10);
ok = true;
@@ -5776,7 +5799,13 @@ fetch_attstats(PGconn *conn, int server_version_num,
" AND attname = ANY(%s)",
column_list);
/* inherited is supported since Postgres 9.0 */
/*
* inherited is supported since Postgres 9.0
*
* Note that this is okay because for now, we support the case where the
* remote table is partitioned, but not the case where it is inherited
* (see fetch_remote_statistics()).
*/
if (server_version_num >= 90000)
appendStringInfoString(&sql,
" ORDER BY attname COLLATE \"C\", inherited DESC");
@@ -5826,7 +5855,10 @@ build_remattrmap(Relation relation, List *va_cols,
if (!attribute_is_analyzable(relation, attnum, attr, NULL))
continue;
/* If the column_name option is not specified, go with attname. */
/*
* Assume the remote column names are the same as the local name
* unless the foreign column's options tell us otherwise.
*/
colname = attname;
fc_options = GetForeignColumnOptions(RelationGetRelid(relation), attnum);
foreach(lc, fc_options)
@@ -5921,7 +5953,7 @@ remattrmap_cmp(const void *v1, const void *v2)
* As the result set consists of the attribute stats for some/all of distinct
* mapped remote columns in the RemoteAttributeMapping, every entry in it
* should have at most one match in the result set; which is also ordered by
* attname, so we find such pairs by doing a merge join.
* remote_attname, so we find such pairs by doing a merge join.
*
* Returns true if every entry in it has a match, and false if not.
*/
@@ -6023,7 +6055,6 @@ import_fetched_statistics(Relation relation,
const RemoteAttributeMapping *remattrmap,
int attrcnt)
{
PGresult *res;
NullableDatum args[ATTSTATS_NUM_FIELDS];
/* Set the 'version' parameter, which is common to both statistics. */
@@ -6035,9 +6066,10 @@ import_fetched_statistics(Relation relation,
* prone to errors. This avoids making a modification of pg_class that
* will just get rolled back by a failed attribute import.
*/
res = remstats->att;
if (res != NULL)
if (remstats->att != NULL)
{
PGresult *res = remstats->att;
Assert(PQnfields(res) == ATTSTATS_NUM_FIELDS);
Assert(PQntuples(res) >= 1);
@@ -6103,16 +6135,12 @@ import_fetched_statistics(Relation relation,
/*
* Import relation statistics.
*/
res = remstats->rel;
Assert(res != NULL);
Assert(PQnfields(res) == RELSTATS_NUM_FIELDS);
Assert(PQntuples(res) == 1);
/* Set the remaining parameters. */
set_uint32_arg(&args[1], get_opt_value(res, 0, RELSTATS_RELPAGES));
Assert(!args[1].isnull);
set_float_arg(&args[2], get_opt_value(res, 0, RELSTATS_RELTUPLES));
Assert(!args[2].isnull);
args[1].value = UInt32GetDatum(remstats->relpages);
args[1].isnull = false;
args[2].value = Float4GetDatum(remstats->reltuples);
args[2].isnull = false;
/* We don't import relallvisible/relallfrozen. */
args[3].value = (Datum) 0;
args[3].isnull = true;
@@ -6181,26 +6209,6 @@ set_int32_arg(NullableDatum *arg, const char *s)
}
}
/*
* Convenience routine for setting optional uint32 arguments
*/
static void
set_uint32_arg(NullableDatum *arg, const char *s)
{
if (s)
{
uint32 val = uint32in_subr(s, NULL, "uint32", NULL);
arg->value = UInt32GetDatum(val);
arg->isnull = false;
}
else
{
arg->value = (Datum) 0;
arg->isnull = true;
}
}
/*
* Convenience routine for setting optional float arguments
*/

17
contrib/postgres_fdw/sql/postgres_fdw.sql
View File

@@ -4686,6 +4686,21 @@ ALTER FOREIGN TABLE simport_fview OPTIONS (ADD import_stats 'true');
ANALYZE simport_fview; -- should fail
CREATE TABLE simport_pt (c1 int not null, c2 text) PARTITION BY LIST (c1);
CREATE TABLE simport_p1 PARTITION OF simport_pt FOR VALUES IN (1);
CREATE TABLE simport_p2 PARTITION OF simport_pt FOR VALUES IN (2);
CREATE FOREIGN TABLE simport_fpt (c1 int not null, c2 text)
SERVER loopback OPTIONS (table_name 'simport_pt');
INSERT INTO simport_pt VALUES (1, 'foo'), (1, 'foo'), (2, 'bar'), (2, 'bar');
-- Check that we have relpages = 0 for simport_fpt regardless of the method
ANALYZE simport_fpt;
SELECT relpages FROM pg_class WHERE oid = 'public.simport_fpt'::regclass;
ALTER FOREIGN TABLE simport_fpt OPTIONS (ADD import_stats 'true');
ANALYZE simport_pt;
ANALYZE VERBOSE simport_fpt; -- should work
SELECT relpages FROM pg_class WHERE oid = 'public.simport_fpt'::regclass;
-- This tests build_remattrmap()'s deparsing of column names that include
-- single quotes or backslashes
CREATE TABLE dtest_table ("col'quote" int, "col\backslash" int);
@@ -4730,6 +4745,8 @@ DROP FOREIGN TABLE simport_ftable;
DROP FOREIGN TABLE simport_fview;
DROP VIEW simport_view;
DROP TABLE simport_table;
DROP FOREIGN TABLE simport_fpt;
DROP TABLE simport_pt;
DROP FOREIGN TABLE dtest_ftable;
DROP TABLE dtest_table;

5
doc/src/sgml/postgres-fdw.sgml
View File

@@ -375,7 +375,7 @@ OPTIONS (ADD password_required 'false');
the local server. If the attempt failed, statistics are collected by
row sampling on the foreign table.
This option is only useful if the remote table is one that can have
regular statistics (tables and materialized views).
regular statistics (tables, not inherited, and materialized views).
When using this option, <emphasis>it is the user's responsibility
</emphasis> to ensure that the existing statistics for the foreign
table are up-to-date.
@@ -386,7 +386,8 @@ OPTIONS (ADD password_required 'false');
If the foreign table is a partition of a partitioned table, analyzing
the partitioned table will still result in row sampling on the foreign
table regardless of this setting, though direct analysis of the foreign
table would have attempted to fetch and import remote statistics first.
table would have attempted to fetch and import the remote statistics
first if this option was enabled.
</para>
</listitem>
</varlistentry>