public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 1/2] review
10+ messages / 5 participants
[nested] [flat]
* [PATCH 1/2] review
@ 2021-01-09 02:17 Tomas Vondra <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Tomas Vondra @ 2021-01-09 02:17 UTC (permalink / raw)
---
doc/src/sgml/mvcc.sgml | 20 +++++++++++---------
doc/src/sgml/ref/create_policy.sgml | 6 +++---
doc/src/sgml/ref/merge.sgml | 12 +++++++++---
src/backend/commands/explain.c | 2 +-
src/backend/executor/execMerge.c | 5 ++++-
src/backend/executor/nodeModifyTable.c | 3 ++-
src/backend/optimizer/prep/preptlist.c | 6 ++++++
src/backend/parser/parse_agg.c | 1 +
src/backend/replication/walsender.c | 4 ++--
src/backend/rewrite/rewriteHandler.c | 4 ++++
src/backend/utils/adt/varlena.c | 3 +++
src/include/nodes/parsenodes.h | 7 ++++---
12 files changed, 50 insertions(+), 23 deletions(-)
diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 9ec8474185..02c9f3fdea 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -429,22 +429,24 @@ COMMIT;
with both <command>INSERT</command> and <command>UPDATE</command>
subcommands looks similar to <command>INSERT</command> with an
<literal>ON CONFLICT DO UPDATE</literal> clause but does not guarantee
- that either <command>INSERT</command> and <command>UPDATE</command> will occur.
+ that either <command>INSERT</command> or <command>UPDATE</command> will occur.
- If MERGE attempts an UPDATE or DELETE and the row is concurrently updated
+ /* XXX This is a very long and hard to understand sentence :-( */
+ /* XXX Do we want to mention EvalPlanQual here? There's no explanation what it does in this file, so maybe elaborate what it does or leave that detail for a README? */
+ If MERGE attempts an <command>UPDATE</command> or <command>DELETE</command> and the row is concurrently updated
but the join condition still passes for the current target and the current
- source tuple, then MERGE will behave the same as the UPDATE or DELETE commands
+ source tuple, then <command>MERGE</command> will behave the same as the <command>UPDATE</command> or <command>DELETE</command> commands
and perform its action on the latest version of the row, using standard
- EvalPlanQual. MERGE actions can be conditional, so conditions must be
+ EvalPlanQual. <command>MERGE</command> actions can be conditional, so conditions must be
re-evaluated on the latest row, starting from the first action.
On the other hand, if the row is concurrently updated or deleted so that
- the join condition fails, then MERGE will execute a NOT MATCHED action, if it
- exists and the AND WHEN qual evaluates to true.
+ the join condition fails, then <command>MERGE</command> will execute a <literal>NOT MATCHED</literal> action, if it
+ exists and the <literal>AND WHEN</literal> qual evaluates to true.
- If MERGE attempts an INSERT and a unique index is present and a duplicate
- row is concurrently inserted then a uniqueness violation is raised. MERGE
- does not attempt to avoid the ERROR by attempting an UPDATE.
+ If <command>MERGE</command> attempts an <command>INSERT</command> and a unique index is present and a duplicate
+ row is concurrently inserted then a uniqueness violation is raised. <command>MERGE</command>
+ does not attempt to avoid the <literal>ERROR</literal> by attempting an <command>UPDATE</command>.
</para>
<para>
diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml
index ad20230c58..0a64674f2d 100644
--- a/doc/src/sgml/ref/create_policy.sgml
+++ b/doc/src/sgml/ref/create_policy.sgml
@@ -97,9 +97,9 @@ CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable
<para>
No separate policy exists for <command>MERGE</command>. Instead policies
- defined for <literal>SELECT</literal>, <literal>INSERT</literal>,
- <literal>UPDATE</literal> and <literal>DELETE</literal> are applied
- while executing MERGE, depending on the actions that are activated.
+ defined for <command>SELECT</command>, <command>INSERT</command>,
+ <command>UPDATE</command> and <command>DELETE</command> are applied
+ while executing <command>MERGE</command>, depending on the actions that are activated.
</para>
</refsect1>
diff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml
index b2a9f67cfa..c2901b0d58 100644
--- a/doc/src/sgml/ref/merge.sgml
+++ b/doc/src/sgml/ref/merge.sgml
@@ -98,7 +98,7 @@ DELETE
</para>
<para>
- There is no MERGE privilege.
+ There is no <literal>MERGE</literal> privilege.
You must have the <literal>UPDATE</literal> privilege on the column(s)
of the <replaceable class="parameter">target_table_name</replaceable>
referred to in the <literal>SET</literal> clause
@@ -217,6 +217,7 @@ DELETE
At least one <literal>WHEN</literal> clause is required.
</para>
<para>
+ /* XXX Do we need to repeat the same thing for WHEN MATCHED and WHEN NOT MATCHED? Could this say that it applies to both? */
If the <literal>WHEN</literal> clause specifies <literal>WHEN MATCHED</literal>
and the candidate change row matches a row in the
<replaceable class="parameter">target_table_name</replaceable>
@@ -242,6 +243,7 @@ DELETE
clause will be activated and the corresponding action will occur for
that row. The expression may not contain functions that possibly performs
writes to the database.
+ /* XXX Does it mean that it has to be marked as STABLE, or that a write crashes the DB? */
</para>
<para>
A condition on a <literal>WHEN MATCHED</literal> clause can refer to columns
@@ -379,6 +381,7 @@ DELETE
<para>
An expression to assign to the column. The expression can use the
old values of this and other columns in the table.
+ /* XXX Which table? Source or target, or both? What if it's NOT MATCHED? */
</para>
</listitem>
</varlistentry>
@@ -492,6 +495,7 @@ MERGE <replaceable class="parameter">total-count</replaceable>
In summary, statement triggers for an event type (say, INSERT) will
be fired whenever we <emphasis>specify</emphasis> an action of that kind. Row-level
triggers will fire only for the one event type <emphasis>activated</emphasis>.
+ /* XXX What does "activated" mean? Maybe "executed" would be better? */
So a <command>MERGE</command> might fire statement triggers for both
<command>UPDATE</command> and <command>INSERT</command>, even though only
<command>UPDATE</command> row triggers were fired.
@@ -504,6 +508,8 @@ MERGE <replaceable class="parameter">total-count</replaceable>
rows will be used to modify the target row, later attempts to modify will
cause an error. This can also occur if row triggers make changes to the
target table which are then subsequently modified by <command>MERGE</command>.
+ /* XXX Not sure the preceding sentence makes sense. What does the 'which' refer to? Row triggers, changes, or what? */
+ /* XXX It seems the rule is that INSERT actions are <literal> while INSERT statements (in SQL sense) are <command>. But this mixes that up? */
If the repeated action is an <command>INSERT</command> this will
cause a uniqueness violation while a repeated <command>UPDATE</command> or
<command>DELETE</command> will cause a cardinality violation; the latter behavior
@@ -554,7 +560,7 @@ MERGE <replaceable class="parameter">total-count</replaceable>
<title>Examples</title>
<para>
- Perform maintenance on CustomerAccounts based upon new Transactions.
+ Perform maintenance on <literal>CustomerAccounts</literal> based upon new <literal>Transactions</literal>.
<programlisting>
MERGE CustomerAccount CA
@@ -599,7 +605,7 @@ WHEN MATCHED THEN
DELETE;
</programlisting>
- The wine_stock_changes table might be, for example, a temporary table
+ The <literal>wine_stock_changes</literal> table might be, for example, a temporary table
recently loaded into the database.
</para>
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index fbaf50c258..f914a00e9f 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -3690,7 +3690,7 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors,
break;
case CMD_MERGE:
operation = "Merge";
- foperation = "Foreign Merge";
+ foperation = "Foreign Merge"; /* XXX Doesn't the commit message say foreign tables are not yet supported? */
break;
default:
operation = "???";
diff --git a/src/backend/executor/execMerge.c b/src/backend/executor/execMerge.c
index 0e245e1361..a7492a6c4b 100644
--- a/src/backend/executor/execMerge.c
+++ b/src/backend/executor/execMerge.c
@@ -105,7 +105,7 @@ ExecMerge(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
* A concurrent update can:
*
* 1. modify the target tuple so that it no longer satisfies the
- * additional quals attached to the current WHEN MATCHED action OR
+ * additional quals attached to the current WHEN MATCHED action
*
* In this case, we are still dealing with a WHEN MATCHED case, but we
* should recheck the list of WHEN MATCHED actions and choose the first
@@ -118,6 +118,9 @@ ExecMerge(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
* tuple, so we now instead find a qualifying WHEN NOT MATCHED action to
* execute.
*
+ * XXX Hmmm, what if the updated tuple would now match one that was
+ * considered NOT MATCHED so far?
+ *
* A concurrent delete, changes a WHEN MATCHED case to WHEN NOT MATCHED.
*
* ExecMergeMatched takes care of following the update chain and
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index c0a97eba91..9c14709e47 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2124,7 +2124,6 @@ ExecModifyTable(PlanState *pstate)
{
/* advance to next subplan if any */
node->mt_whichplan++;
-
if (node->mt_whichplan < node->mt_nplans)
{
resultRelInfo++;
@@ -2169,6 +2168,8 @@ ExecModifyTable(PlanState *pstate)
EvalPlanQualSetSlot(&node->mt_epqstate, planSlot);
slot = planSlot;
+ /* XXX Wouldn't it be "nicer" to handle MERGE in the switch, together
+ * with the other MT operations? This seems a bit out of place. */
if (operation == CMD_MERGE)
{
ExecMerge(node, resultRelInfo, estate, slot, junkfilter);
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index 8848e9a03f..b2543f6814 100644
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -117,6 +117,10 @@ preprocess_targetlist(PlannerInfo *root)
tlist = expand_targetlist(tlist, command_type,
result_relation, target_relation);
+ /*
+ * For MERGE we need to handle the target list for the target relation,
+ * and also target list for each action (only INSERT/UPDATE matter).
+ */
if (command_type == CMD_MERGE)
{
ListCell *l;
@@ -343,6 +347,8 @@ expand_targetlist(List *tlist, int command_type,
* generate a NULL for dropped columns (we want to drop any old
* values).
*
+ * XXX Should this explain why MERGE has the same logic as UPDATE?
+ *
* When generating a NULL constant for a dropped column, we label
* it INT4 (any other guaranteed-to-exist datatype would do as
* well). We can't label it with the dropped column's datatype
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index f7c152beb4..5d49c8c37e 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -436,6 +436,7 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
break;
case EXPR_KIND_MERGE_WHEN_AND:
if (isAgg)
+ /* XXX "WHEN AND" seems rather strange. ... */
err = _("aggregate functions are not allowed in WHEN AND conditions");
else
err = _("grouping operations are not allowed in WHEN AND conditions");
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index d59f4fde95..d50543b1ba 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1198,6 +1198,7 @@ StartLogicalReplication(StartReplicationCmd *cmd)
/* Also update the sent position status in shared memory */
SpinLockAcquire(&MyWalSnd->mutex);
+ /* XXX Huh? Why does MERGE patch change walsender? */
MyWalSnd->sentPtr = MyReplicationSlot->data.confirmed_flush;
SpinLockRelease(&MyWalSnd->mutex);
@@ -2930,8 +2931,7 @@ WalSndDone(WalSndSendDataCallback send_data)
replicatedPtr = XLogRecPtrIsInvalid(MyWalSnd->flush) ?
MyWalSnd->write : MyWalSnd->flush;
- if (WalSndCaughtUp &&
- sentPtr == replicatedPtr &&
+ if (WalSndCaughtUp && sentPtr == replicatedPtr &&
!pq_is_send_pending())
{
QueryCompletion qc;
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index e2706c181c..80ae946d17 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1774,6 +1774,7 @@ fill_extraUpdatedCols(RangeTblEntry *target_rte, Relation target_relation)
}
}
+
/*
* matchLocks -
* match the list of locks and returns the matching rules
@@ -3946,6 +3947,9 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
/*
* XXX MERGE doesn't support write rules because they would violate
* the SQL Standard spec and would be unclear how they should work.
+ *
+ * XXX So does't support means 'ignores'? Should that either fail
+ * or at least print some warning?
*/
if (event == CMD_MERGE)
product_queries = NIL;
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 0281351dcf..7a768a7b5b 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -2312,6 +2312,7 @@ varstrfastcmp_locale(char *a1p, int len1, char *a2p, int len2, SortSupport ssup)
int result;
bool arg1_match;
+ /* XXX Huh? Why is this in MERGE patch? */
if (len1 < 0 || len2 < 0)
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
@@ -2505,6 +2506,7 @@ varstr_abbrev_convert(Datum original, SortSupport ssup)
memset(pres, 0, sizeof(Datum));
len = VARSIZE_ANY_EXHDR(authoritative);
+ /* XXX Huh? Why is this in MERGE patch? */
if (len < 0)
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
@@ -3260,6 +3262,7 @@ bytea_catenate(bytea *t1, bytea *t2)
len1 = VARSIZE_ANY_EXHDR(t1);
len2 = VARSIZE_ANY_EXHDR(t2);
+ /* XXX Huh? Why is this in MERGE patch? */
if (len1 < 0 || len2 < 0)
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 3cba38044e..08fea9b8f7 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -172,6 +172,7 @@ typedef struct Query
List *withCheckOptions; /* a list of WithCheckOption's (added
* during rewrite) */
+ /* XXX Why not mergeTragetRelation? */
int mergeTarget_relation;
List *mergeSourceTargetList;
List *mergeActionList; /* list of actions for MERGE (only) */
@@ -1581,11 +1582,11 @@ typedef struct UpdateStmt
typedef struct MergeStmt
{
NodeTag type;
- RangeVar *relation; /* target relation to merge into */
+ RangeVar *relation; /* target relation to merge into */
Node *source_relation; /* source relation */
- Node *join_condition; /* join condition between source and target */
+ Node *join_condition; /* join condition between source and target */
List *mergeWhenClauses; /* list of MergeWhenClause(es) */
- WithClause *withClause; /* WITH clause */
+ WithClause *withClause; /* WITH clause */
} MergeStmt;
typedef struct MergeWhenClause
--
2.26.2
--------------B1AA4DC5EAE9BAF8B146244A
Content-Type: text/plain; charset=UTF-8;
name="0002-fix-valgrind-failure.txt"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="0002-fix-valgrind-failure.txt"
^ permalink raw reply [nested|flat] 10+ messages in thread
* postgres_fdw: using TABLESAMPLE to collect remote sample
@ 2022-02-11 17:02 Tomas Vondra <[email protected]>
0 siblings, 2 replies; 10+ messages in thread
From: Tomas Vondra @ 2022-02-11 17:02 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
Hi,
here's a small patch modifying postgres_fdw to use TABLESAMPLE to
collect sample when running ANALYZE on a foreign table. Currently the
sampling happens locally, i.e. we fetch all data from the remote server
and then pick rows. But that's hugely expensive for large relations
and/or with limited network bandwidth, of course.
Alexander Lepikhov mentioned this in [1], but it was actually proposed
by Stephen in 2020 [2] but no patch even materialized.
So here we go. The patch does a very simple thing - it uses TABLESAMPLE
to collect/transfer just a small sample from the remote node, saving
both CPU and network.
And indeed, that helps quite a bit:
---------------------------------------------------------------------
create table t (a int);
Time: 10.223 ms
insert into t select i from generate_series(1,10000000) s(i);
Time: 552.207 ms
analyze t;
Time: 310.445 ms
CREATE FOREIGN TABLE ft (a int)
SERVER foreign_server
OPTIONS (schema_name 'public', table_name 't');
Time: 4.838 ms
analyze ft;
WARNING: SQL: DECLARE c1 CURSOR FOR SELECT a FROM public.t TABLESAMPLE
SYSTEM(0.375001)
Time: 44.632 ms
alter foreign table ft options (sample 'false');
Time: 4.821 ms
analyze ft;
WARNING: SQL: DECLARE c1 CURSOR FOR SELECT a FROM public.t
Time: 6690.425 ms (00:06.690)
---------------------------------------------------------------------
6690ms without sampling, and 44ms with sampling - quite an improvement.
Of course, the difference may be much smaller/larger in other cases.
Now, there's a couple issues ...
Firstly, the FDW API forces a bit strange division of work between
different methods and duplicating some of it (and it's already mentioned
in postgresAnalyzeForeignTable). But that's minor and can be fixed.
The other issue is which sampling method to use - we have SYSTEM and
BERNOULLI built in, and the tsm_system_rows as an extension (and _time,
but that's not very useful here). I guess we'd use one of the built-in
ones, because that'll work on more systems out of the box.
But that leads to the main issue - determining the fraction of rows to
sample. We know how many rows we want to sample, but we have no idea how
many rows there are in total. We can look at reltuples, but we can't be
sure how accurate / up-to-date that value is.
The patch just trusts it unless it's obviously bogus (-1, 0, etc.) and
applies some simple sanity checks, but I wonder if we need to do more
(e.g. look at relation size and adjust reltuples by current/relpages).
FWIW this is yet a bit more convoluted when analyzing partitioned table
with foreign partitions, because we only ever look at relation sizes to
determine how many rows to sample from each partition.
regards
[1]
https://www.postgresql.org/message-id/bdb0bea2-a0da-1f1d-5c92-96ff90c198eb%40postgrespro.ru
[2]
https://www.postgresql.org/message-id/20200829162231.GE29590%40tamriel.snowman.net
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[text/x-patch] postgres-fdw-analyze-sample-20220211.patch (9.6K, ../../[email protected]/2-postgres-fdw-analyze-sample-20220211.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index bf12eac0288..68f875c47a6 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -2267,6 +2267,26 @@ deparseAnalyzeSizeSql(StringInfo buf, Relation rel)
appendStringInfo(buf, "::pg_catalog.regclass) / %d", BLCKSZ);
}
+/*
+ * Construct SELECT statement to acquire numbe of rows of given relation.
+ *
+ * Note: Maybe this should compare relpages and current relation size
+ * and adjust reltuples accordingly?
+ */
+void
+deparseAnalyzeTuplesSql(StringInfo buf, Relation rel)
+{
+ StringInfoData relname;
+
+ /* We'll need the remote relation name as a literal. */
+ initStringInfo(&relname);
+ deparseRelation(&relname, rel);
+
+ appendStringInfoString(buf, "SELECT reltuples FROM pg_catalog.pg_class WHERE oid = ");
+ deparseStringLiteral(buf, relname.data);
+ appendStringInfoString(buf, "::pg_catalog.regclass");
+}
+
/*
* Construct SELECT statement to acquire sample rows of given relation.
*
@@ -2328,6 +2348,72 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs)
deparseRelation(buf, rel);
}
+/*
+ * Construct SELECT statement to acquire sample rows of given relation,
+ * by sampling a fraction of the table using TABLESAMPLE SYSTEM.
+ *
+ * SELECT command is appended to buf, and list of columns retrieved
+ * is returned to *retrieved_attrs.
+ *
+ * Note: We could allow selecting system/bernoulli, and maybe even the
+ * optional TSM modules (especially tsm_system_rows would help).
+ */
+void
+deparseAnalyzeSampleSql(StringInfo buf, Relation rel, List **retrieved_attrs, double sample_frac)
+{
+ Oid relid = RelationGetRelid(rel);
+ TupleDesc tupdesc = RelationGetDescr(rel);
+ int i;
+ char *colname;
+ List *options;
+ ListCell *lc;
+ bool first = true;
+
+ *retrieved_attrs = NIL;
+
+ appendStringInfoString(buf, "SELECT ");
+ for (i = 0; i < tupdesc->natts; i++)
+ {
+ /* Ignore dropped columns. */
+ if (TupleDescAttr(tupdesc, i)->attisdropped)
+ continue;
+
+ if (!first)
+ appendStringInfoString(buf, ", ");
+ first = false;
+
+ /* Use attribute name or column_name option. */
+ colname = NameStr(TupleDescAttr(tupdesc, i)->attname);
+ options = GetForeignColumnOptions(relid, i + 1);
+
+ foreach(lc, options)
+ {
+ DefElem *def = (DefElem *) lfirst(lc);
+
+ if (strcmp(def->defname, "column_name") == 0)
+ {
+ colname = defGetString(def);
+ break;
+ }
+ }
+
+ appendStringInfoString(buf, quote_identifier(colname));
+
+ *retrieved_attrs = lappend_int(*retrieved_attrs, i + 1);
+ }
+
+ /* Don't generate bad syntax for zero-column relation. */
+ if (first)
+ appendStringInfoString(buf, "NULL");
+
+ /*
+ * Construct FROM clause
+ */
+ appendStringInfoString(buf, " FROM ");
+ deparseRelation(buf, rel);
+ appendStringInfo(buf, " TABLESAMPLE SYSTEM(%f)", (100.0 * sample_frac));
+}
+
/*
* Construct a simple "TRUNCATE rel" statement
*/
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index fc3ce6a53a2..02be48ec82c 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -121,7 +121,8 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
strcmp(def->defname, "updatable") == 0 ||
strcmp(def->defname, "truncatable") == 0 ||
strcmp(def->defname, "async_capable") == 0 ||
- strcmp(def->defname, "keep_connections") == 0)
+ strcmp(def->defname, "keep_connections") == 0 ||
+ strcmp(def->defname, "sample") == 0)
{
/* these accept only boolean values */
(void) defGetBoolean(def);
@@ -252,6 +253,10 @@ InitPgFdwOptions(void)
{"keep_connections", ForeignServerRelationId, false},
{"password_required", UserMappingRelationId, false},
+ /* sampling is available on both server and table */
+ {"sample", ForeignServerRelationId, false},
+ {"sample", ForeignTableRelationId, false},
+
/*
* sslcert and sslkey are in fact libpq options, but we repeat them
* here to allow them to appear in both foreign server context (when
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 56654844e8f..be04ceb01ef 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4961,6 +4961,68 @@ postgresAnalyzeForeignTable(Relation relation,
return true;
}
+/*
+ * postgresCountTuplesForForeignTable
+ * Count tuples in foreign table (just get pg_class.reltuples).
+ *
+ * Note: It's unclear how accurate reltuples is, maybe size that using
+ * relpages and simple assumptions (1 tuples per page, ...)? Using
+ * tsm_system_rows wold make this somewhat unnecessary.
+ */
+static double
+postgresCountTuplesForForeignTable(Relation relation)
+{
+ ForeignTable *table;
+ UserMapping *user;
+ PGconn *conn;
+ StringInfoData sql;
+ PGresult *volatile res = NULL;
+ double reltuples = -1;
+
+ /*
+ * Now we have to get the number of pages. It's annoying that the ANALYZE
+ * API requires us to return that now, because it forces some duplication
+ * of effort between this routine and postgresAcquireSampleRowsFunc. But
+ * it's probably not worth redefining that API at this point.
+ */
+
+ /*
+ * Get the connection to use. We do the remote access as the table's
+ * owner, even if the ANALYZE was started by some other user.
+ */
+ table = GetForeignTable(RelationGetRelid(relation));
+ user = GetUserMapping(relation->rd_rel->relowner, table->serverid);
+ conn = GetConnection(user, false, NULL);
+
+ /*
+ * Construct command to get page count for relation.
+ */
+ initStringInfo(&sql);
+ deparseAnalyzeTuplesSql(&sql, relation);
+
+ /* In what follows, do not risk leaking any PGresults. */
+ PG_TRY();
+ {
+ res = pgfdw_exec_query(conn, sql.data, NULL);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ pgfdw_report_error(ERROR, res, conn, false, sql.data);
+
+ if (PQntuples(res) != 1 || PQnfields(res) != 1)
+ elog(ERROR, "unexpected result from deparseAnalyzeSizeSql query");
+ reltuples = strtod(PQgetvalue(res, 0, 0), NULL);
+ }
+ PG_FINALLY();
+ {
+ if (res)
+ PQclear(res);
+ }
+ PG_END_TRY();
+
+ ReleaseConnection(conn);
+
+ return reltuples;
+}
+
/*
* Acquire a random sample of rows from foreign table managed by postgres_fdw.
*
@@ -4991,6 +5053,12 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
unsigned int cursor_number;
StringInfoData sql;
PGresult *volatile res = NULL;
+ ListCell *lc;
+
+ /* sampling enabled by default */
+ bool do_sample = true;
+ double sample_frac = -1.0;
+ double reltuples;
/* Initialize workspace state */
astate.rel = relation;
@@ -5018,20 +5086,74 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
user = GetUserMapping(relation->rd_rel->relowner, table->serverid);
conn = GetConnection(user, false, NULL);
+ /*
+ * Should we use TABLESAMPLE to collect the remote sample?
+ */
+ foreach(lc, server->options)
+ {
+ DefElem *def = (DefElem *) lfirst(lc);
+
+ if (strcmp(def->defname, "sample") == 0)
+ {
+ do_sample = defGetBoolean(def);
+ break;
+ }
+ }
+ foreach(lc, table->options)
+ {
+ DefElem *def = (DefElem *) lfirst(lc);
+
+ if (strcmp(def->defname, "sample") == 0)
+ {
+ do_sample = defGetBoolean(def);
+ break;
+ }
+ }
+
+ if (do_sample)
+ {
+ reltuples = postgresCountTuplesForForeignTable(relation);
+
+ if ((reltuples <= 0) || (targrows >= reltuples))
+ do_sample = false;
+
+ sample_frac = targrows / reltuples;
+
+ /* Let's sample a bit more, we'll reduce the sample locally. */
+ sample_frac *= 1.25;
+
+ /* Sanity checks. */
+ sample_frac = Min(1.0, Max(0.0, sample_frac));
+
+ /*
+ * When sampling too large fraction, just read everything.
+ *
+ * XXX It's not clear where exactly the threshold is, with slow
+ * network it may be cheaper to sample even 90%.
+ */
+ if (sample_frac > 0.5)
+ do_sample = false;
+ }
+
/*
* Construct cursor that retrieves whole rows from remote.
*/
cursor_number = GetCursorNumber(conn);
initStringInfo(&sql);
appendStringInfo(&sql, "DECLARE c%u CURSOR FOR ", cursor_number);
- deparseAnalyzeSql(&sql, relation, &astate.retrieved_attrs);
+
+ if (do_sample)
+ deparseAnalyzeSampleSql(&sql, relation, &astate.retrieved_attrs, sample_frac);
+ else
+ deparseAnalyzeSql(&sql, relation, &astate.retrieved_attrs);
+
+ elog(WARNING, "SQL: %s", sql.data);
/* In what follows, do not risk leaking any PGresults. */
PG_TRY();
{
char fetch_sql[64];
int fetch_size;
- ListCell *lc;
res = pgfdw_exec_query(conn, sql.data, NULL);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
@@ -5120,7 +5242,10 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
*totaldeadrows = 0.0;
/* We've retrieved all living tuples from foreign server. */
- *totalrows = astate.samplerows;
+ if (do_sample)
+ *totalrows = reltuples;
+ else
+ *totalrows = astate.samplerows;
/*
* Emit some interesting relation info
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 8ae79e97e44..5e4f732e563 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -208,8 +208,12 @@ extern void deparseDirectDeleteSql(StringInfo buf, PlannerInfo *root,
List *returningList,
List **retrieved_attrs);
extern void deparseAnalyzeSizeSql(StringInfo buf, Relation rel);
+extern void deparseAnalyzeTuplesSql(StringInfo buf, Relation rel);
extern void deparseAnalyzeSql(StringInfo buf, Relation rel,
List **retrieved_attrs);
+extern void deparseAnalyzeSampleSql(StringInfo buf, Relation rel,
+ List **retrieved_attrs,
+ double sample_frac);
extern void deparseTruncateSql(StringInfo buf,
List *rels,
DropBehavior behavior,
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: postgres_fdw: using TABLESAMPLE to collect remote sample
@ 2022-02-16 12:56 Sofia Kopikova <[email protected]>
parent: Tomas Vondra <[email protected]>
1 sibling, 0 replies; 10+ messages in thread
From: Sofia Kopikova @ 2022-02-16 12:56 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; pgsql-hackers <[email protected]>
Hello,
I find it a great idea to use TABLESAMPLE in postgres_fdw ANALYZE.
Let me offer you some ideas how to resolve you problems.
Tomas Vondra < [email protected] > writes:
> The other issue is which sampling method to use - we have SYSTEM and
> BERNOULLI built in, and the tsm_system_rows as an extension (and _time,
> but that's not very useful here). I guess we'd use one of the built-in
> ones, because that'll work on more systems out of the box.
It’s hard to choose between speed and quality, but I think we need
SYSTEM method here. This patch is for speeding-up ANALYZE,
and SYSTEM method will faster than BERNOULLI on fraction
values to 50%.
> But that leads to the main issue - determining the fraction of rows to
> sample. We know how many rows we want to sample, but we have no idea how
> many rows there are in total. We can look at reltuples, but we can't be
> sure how accurate / up-to-date that value is.
>
> The patch just trusts it unless it's obviously bogus (-1, 0, etc.) and
> applies some simple sanity checks, but I wonder if we need to do more
> (e.g. look at relation size and adjust reltuples by current/relpages).
I found a query on Stackoverflow (it does similar thing to what
estimate_rel_size does) that may help with it. So that makes
tsm_system_rows unnecessary.
----------------------------------------------------------------------
SELECT
(CASE WHEN relpages = 0 THEN float8 '0'
ELSE reltuples / relpages END
* (pg_relation_size(oid) / pg_catalog.current_setting('block_size')::int)
)::bigint
FROM pg_class c
WHERE c.oid = 'tablename'::regclass;
----------------------------------------------------------------------
And one more refactoring note. New function deparseAnalyzeSampleSql
duplicates function deparseAnalyzeSql (is previous in file deparse.c)
except for the new last line. I guess it would be better to add new
parameter — double sample_frac — to existing function
deparseAnalyzeSql and use it as a condition for adding
"TABLESAMPLE SYSTEM..." to SQL query (set it to zero when
do_sample is false). Or you may also add do_sample as a parameter to
deparseAnalyzeSql, but as for me that’s redundantly.
Stackoverflow: https://stackoverflow.com/questions/7943233/fast-way-to-discover-the-row-count-of-a-table-in-postgre...
--
Sofia Kopikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: postgres_fdw: using TABLESAMPLE to collect remote sample
@ 2022-12-15 16:34 James Finnerty <[email protected]>
parent: Tomas Vondra <[email protected]>
1 sibling, 1 reply; 10+ messages in thread
From: James Finnerty @ 2022-12-15 16:34 UTC (permalink / raw)
To: [email protected]; +Cc: Tomas Vondra <[email protected]>
This patch looks good to me. I have two very minor nits: The inflation of the sample size by 10% is arbitrary but it doesn't seem unreasonable or concerning. It just makes me curious if there are any known cases that motivated adding this logic. Secondly, if the earliest non-deprecated version of PostgreSQL supports sampling, then you could optionally remove the logic that tests for that. The affected lines should be unreachable.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: postgres_fdw: using TABLESAMPLE to collect remote sample
@ 2022-12-15 16:46 Tom Lane <[email protected]>
parent: James Finnerty <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Tom Lane @ 2022-12-15 16:46 UTC (permalink / raw)
To: James Finnerty <[email protected]>; +Cc: [email protected]; Tomas Vondra <[email protected]>
James Finnerty <[email protected]> writes:
> This patch looks good to me. I have two very minor nits: The inflation
> of the sample size by 10% is arbitrary but it doesn't seem unreasonable
> or concerning. It just makes me curious if there are any known cases
> that motivated adding this logic.
I wondered why, too.
> Secondly, if the earliest non-deprecated version of PostgreSQL supports
> sampling, then you could optionally remove the logic that tests for
> that. The affected lines should be unreachable.
We've tried to keep postgres_fdw compatible with quite ancient remote
servers (I think the manual claims back to 8.3, though it's unlikely
anyone's tested that far back lately). This patch should not move those
goalposts, especially if it's easy not to.
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: postgres_fdw: using TABLESAMPLE to collect remote sample
@ 2022-12-21 12:08 Tomas Vondra <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Tomas Vondra @ 2022-12-21 12:08 UTC (permalink / raw)
To: Tom Lane <[email protected]>; James Finnerty <[email protected]>; +Cc: [email protected]; Tomas Vondra <[email protected]>
On 12/15/22 17:46, Tom Lane wrote:
> James Finnerty <[email protected]> writes:
>> This patch looks good to me. I have two very minor nits: The inflation
>> of the sample size by 10% is arbitrary but it doesn't seem unreasonable
>> or concerning. It just makes me curious if there are any known cases
>> that motivated adding this logic.
>
> I wondered why, too.
>
Not sure about known cases, but the motivation is explained in the
comment before the 10% is applied.
The repluples value is never going to be spot on, and we use that to
determine what fraction of the table to sample (because all built-in
TABLESAMPLE methods only accept fraction to sample, not expected number
of rows).
If pg_class.reltuples is lower (than the actual row count), we'll end up
with sample fraction too high. That's mostly harmless, as we'll then
discard some of the rows locally.
But if the pg_class.reltuples is higher, we'll end up sampling too few
rows (less than targrows). This can happen e.g. after a bulk delete.
Yes, the 10% value is mostly arbitrary, and maybe it's pointless. How
much may the stats change with 10% larger sample? Probably not much, so
is it really solving anything?
Also, maybe it's fixing the issue at the wrong level - if stale
reltuples are an issue, maybe the right fix is making it more accurate
on the source instance. Decrease autovacuum_vacuum_scale_factor, or
maybe also look at pg_stat_all_tables or something.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: postgres_fdw: using TABLESAMPLE to collect remote sample
@ 2022-12-30 23:03 Tomas Vondra <[email protected]>
parent: Tomas Vondra <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Tomas Vondra @ 2022-12-30 23:03 UTC (permalink / raw)
To: Tom Lane <[email protected]>; James Finnerty <[email protected]>; +Cc: [email protected]; Tomas Vondra <[email protected]>
Pushed.
After thinking about it a bit more I decided to rip out the 10% sampling
rate inflation. While stale reltuples may be an issue, but I couldn't
convince myself this would be an improvement overall, or that this is
the right place to deal with it.
Firstly, if reltuples is too low (i.e. when the table grew), everything
is "fine" - we sample more than targrows and then remove some of that
locally. We end up with same sample size as without this patch - which
is not necessarily the right sample size (more about that in a minute),
but if it was good enough for now ... improving this bit was not the
ambition of this patch.
If reltuples is too high (e.g. right after bulk DELETE) we may end up
sampling too few rows. If it's 1/2 what it should be, we'll end up
sampling only 15k rows instead of 30k rows. But it's unclear how much
should we adjust the value - 10% as was in the patch (kinda based on
autovacuum_analyze_scale_factor being 10%)?
I'd argue that's too low to make any difference - cases that are only
~10% off should work fine (we don't have perfect stats anyway). And for
the really bad cases 10% is not going to make a difference.
And if the reltuples is that off, it'll probably affect even queries
planned on the remote node (directly or pushed down), so I guess the
right fix should be making sure reltuples is not actually off - make
autovacuum more aggressive or whatever.
A thing that bothers me is that calculating sample size for partitioned
tables is a bit cyclic - we look at number of blocks, and divide the
whole sample based on that. And that's generally correlated with the
reltuples, but OTOH it may also be wrong - and not necessarily in the
same way. For example you might delete 90% of a table, which will make
the table overrepresented in the sample. But then reltuples may be quite
accurate thanks to recent vacuum - so fixing this by blindly adjusting
the sampling rate seems misguided. If this turns out to be an issue in
practice, we need to rethink acquire_inherited_sample_rows as a whole
(not just for foreign tables).
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: postgres_fdw: using TABLESAMPLE to collect remote sample
@ 2022-12-31 04:42 Tom Lane <[email protected]>
parent: Tomas Vondra <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Tom Lane @ 2022-12-31 04:42 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: James Finnerty <[email protected]>; [email protected]
Tomas Vondra <[email protected]> writes:
> After thinking about it a bit more I decided to rip out the 10% sampling
> rate inflation.
+1. I'm not sure if there's anything more we need to do there, but
that didn't seem like that was it.
I notice that the committed patch still has a reference to that hack
though:
+ * Ensure the sampling rate is between 0.0 and 1.0, even after the
+ * 10% adjustment above. (Clamping to 0.0 is just paranoia.)
Clamping still seems like a wise idea, but the comment is just
confusing now.
Also, I wonder if there is any possibility of ANALYZE failing
with
ERROR: TABLESAMPLE clause can only be applied to tables and materialized views
I think the patch avoids that, but only accidentally, because
reltuples will be 0 or -1 for a view. Maybe it'd be a good
idea to pull back relkind along with reltuples, and check
that too?
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: postgres_fdw: using TABLESAMPLE to collect remote sample
@ 2022-12-31 14:16 Tomas Vondra <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Tomas Vondra @ 2022-12-31 14:16 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: James Finnerty <[email protected]>; [email protected]
On 12/31/22 05:42, Tom Lane wrote:
> Tomas Vondra <[email protected]> writes:
>> After thinking about it a bit more I decided to rip out the 10% sampling
>> rate inflation.
>
> +1. I'm not sure if there's anything more we need to do there, but
> that didn't seem like that was it.
>
> I notice that the committed patch still has a reference to that hack
> though:
>
> + * Ensure the sampling rate is between 0.0 and 1.0, even after the
> + * 10% adjustment above. (Clamping to 0.0 is just paranoia.)
>
> Clamping still seems like a wise idea, but the comment is just
> confusing now.
>
Yeah, I missed that reference. Will fix.
> Also, I wonder if there is any possibility of ANALYZE failing
> with
>
> ERROR: TABLESAMPLE clause can only be applied to tables and materialized views
>
> I think the patch avoids that, but only accidentally, because
> reltuples will be 0 or -1 for a view. Maybe it'd be a good
> idea to pull back relkind along with reltuples, and check
> that too?
Not sure. I guess we can rely on reltuples being 0 or -1 in such cases,
but maybe it'd be good to at least mention that in a comment? We're not
going to use other reltuples values for views etc.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: postgres_fdw: using TABLESAMPLE to collect remote sample
@ 2022-12-31 17:23 Tom Lane <[email protected]>
parent: Tomas Vondra <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Tom Lane @ 2022-12-31 17:23 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: James Finnerty <[email protected]>; [email protected]
Tomas Vondra <[email protected]> writes:
> On 12/31/22 05:42, Tom Lane wrote:
>> ERROR: TABLESAMPLE clause can only be applied to tables and materialized views
>> I think the patch avoids that, but only accidentally, because
>> reltuples will be 0 or -1 for a view. Maybe it'd be a good
>> idea to pull back relkind along with reltuples, and check
>> that too?
> Not sure. I guess we can rely on reltuples being 0 or -1 in such cases,
> but maybe it'd be good to at least mention that in a comment? We're not
> going to use other reltuples values for views etc.
Would anyone ever point a foreign table at a sequence? I guess it
would be a weird use-case, but it's possible, or was till now.
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2022-12-31 17:23 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-09 02:17 [PATCH 1/2] review Tomas Vondra <[email protected]>
2022-02-11 17:02 postgres_fdw: using TABLESAMPLE to collect remote sample Tomas Vondra <[email protected]>
2022-02-16 12:56 ` Re: postgres_fdw: using TABLESAMPLE to collect remote sample Sofia Kopikova <[email protected]>
2022-12-15 16:34 ` Re: postgres_fdw: using TABLESAMPLE to collect remote sample James Finnerty <[email protected]>
2022-12-15 16:46 ` Re: postgres_fdw: using TABLESAMPLE to collect remote sample Tom Lane <[email protected]>
2022-12-21 12:08 ` Re: postgres_fdw: using TABLESAMPLE to collect remote sample Tomas Vondra <[email protected]>
2022-12-30 23:03 ` Re: postgres_fdw: using TABLESAMPLE to collect remote sample Tomas Vondra <[email protected]>
2022-12-31 04:42 ` Re: postgres_fdw: using TABLESAMPLE to collect remote sample Tom Lane <[email protected]>
2022-12-31 14:16 ` Re: postgres_fdw: using TABLESAMPLE to collect remote sample Tomas Vondra <[email protected]>
2022-12-31 17:23 ` Re: postgres_fdw: using TABLESAMPLE to collect remote sample Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox