Hi,
here's two minor postgres_fdw patches, addressing the two issues
discussed last week.
1) 0001-Fix-stale-comment-in-postgres_fdw.patch
The first one cleans up the comment referencing the sample rate
adjustment. While doing that, I realized without the adjustment we
should never get sample_rate outside the valid range because we do:
if ((reltuples <= 0) || (targrows >= reltuples))
method = ANALYZE_SAMPLE_OFF;
So I removed the clamping, or rather replaced it with an assert.
2) 0002-WIP-check-relkind-in-FDW-analyze.patch
The second patch adds the relkind check, so that we only issue
TABLESAMPLE on valid relation types (tables, matviews). But I'm not sure
we actually need it - the example presented earlier was foreign table
pointing to a sequence. But that actually works fine even without this
patch, because fore sequences we have reltuples=1, which disables
sampling due to the check mentioned above (because targrows >= 1).
The other issue with this patch is that it seems wrong to check the
relkind value locally - instead we should check this on the remote
server, because that's where we'll run TABLESAMPLE. Currently there are
no differences between versions, but what if we implement TABLESAMPLE
for another relkind in the future? Then we'll either fail to use
sampling or (worse) we'll try using TABLESAMPLE for unsupported relkind,
depending on which of the servers has newer version.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 681b3569278bc55b4e2bbfcf129245702616ecd9 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.von...@postgresql.org>
Date: Thu, 5 Jan 2023 21:13:20 +0100
Subject: [PATCH 2/2] WIP: check relkind in FDW analyze
---
contrib/postgres_fdw/deparse.c | 2 +-
contrib/postgres_fdw/postgres_fdw.c | 19 +++++++++++++++----
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 4461fb06f02..9f39d792280 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -2383,7 +2383,7 @@ deparseAnalyzeTuplesSql(StringInfo buf, Relation rel)
initStringInfo(&relname);
deparseRelation(&relname, rel);
- appendStringInfoString(buf, "SELECT reltuples FROM pg_catalog.pg_class WHERE oid = ");
+ appendStringInfoString(buf, "SELECT reltuples, relkind FROM pg_catalog.pg_class WHERE oid = ");
deparseStringLiteral(buf, relname.data);
appendStringInfoString(buf, "::pg_catalog.regclass");
}
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 65822b8a9aa..50b03067152 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4978,7 +4978,7 @@ postgresAnalyzeForeignTable(Relation relation,
* Count tuples in foreign table (just get pg_class.reltuples).
*/
static double
-postgresCountTuplesForForeignTable(Relation relation)
+postgresCountTuplesForForeignTable(Relation relation, bool *can_sample)
{
ForeignTable *table;
UserMapping *user;
@@ -4986,6 +4986,10 @@ postgresCountTuplesForForeignTable(Relation relation)
StringInfoData sql;
PGresult *volatile res = NULL;
volatile double reltuples = -1;
+ volatile char relkind = 0;
+
+ /* assume the relkind can't be sampled */
+ *can_sample = false;
/*
* Get the connection to use. We do the remote access as the table's
@@ -5008,9 +5012,10 @@ postgresCountTuplesForForeignTable(Relation relation)
if (PQresultStatus(res) != PGRES_TUPLES_OK)
pgfdw_report_error(ERROR, res, conn, false, sql.data);
- if (PQntuples(res) != 1 || PQnfields(res) != 1)
+ if (PQntuples(res) != 1 || PQnfields(res) != 2)
elog(ERROR, "unexpected result from deparseAnalyzeTuplesSql query");
reltuples = strtod(PQgetvalue(res, 0, 0), NULL);
+ relkind = *(PQgetvalue(res, 0, 1));
}
PG_FINALLY();
{
@@ -5021,6 +5026,10 @@ postgresCountTuplesForForeignTable(Relation relation)
ReleaseConnection(conn);
+ *can_sample = (relkind == RELKIND_RELATION ||
+ relkind == RELKIND_MATVIEW ||
+ relkind == RELKIND_PARTITIONED_TABLE);
+
return reltuples;
}
@@ -5167,13 +5176,15 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
*/
if (method != ANALYZE_SAMPLE_OFF)
{
- reltuples = postgresCountTuplesForForeignTable(relation);
+ bool can_sample;
+
+ reltuples = postgresCountTuplesForForeignTable(relation, &can_sample);
/*
* Remote's reltuples could be 0 or -1 if the table has never been
* vacuumed/analyzed. In that case, disable sampling after all.
*/
- if ((reltuples <= 0) || (targrows >= reltuples))
+ if ((!can_sample) || (reltuples <= 0) || (targrows >= reltuples))
method = ANALYZE_SAMPLE_OFF;
else
{
--
2.39.0
From 020ba6afb87287d782477054949d77954377d347 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.von...@postgresql.org>
Date: Thu, 5 Jan 2023 20:11:49 +0100
Subject: [PATCH 1/2] Fix stale comment in postgres_fdw
A comment was left behind referencing a bit (sample rate adjustment)
removed from 8ad51b5f44. So clean that up, and while at it also remove
clamping of the sample rate which should not be necessary without the
clamping, and just check that with an assert.
---
contrib/postgres_fdw/postgres_fdw.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 332b4a5cdeb..65822b8a9aa 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -5204,10 +5204,11 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
sample_frac = targrows / reltuples;
/*
- * Ensure the sampling rate is between 0.0 and 1.0, even after the
- * 10% adjustment above. (Clamping to 0.0 is just paranoia.)
+ * We should never get sampling rate outside the valid range
+ * (between 0.0 and 1.0), because those cases should be covered
+ * by the previous branch that sets ANALYZE_SAMPLE_OFF.
*/
- sample_frac = Min(1.0, Max(0.0, sample_frac));
+ Assert(sample_frac == Min(1.0, Max(0.0, sample_frac)));
}
}
--
2.39.0
