Release SPI plans for referential integrity with DISCARD ALL
Hello,
We found problem that a huge amount of memory was consumed when
we created a foreign key on a partitioned table including a lots partitions
and accessed them, as discussed in [1]/messages/by-id/cab4b85d-9292-967d-adf2-be0d803c3e23@nttcom.co.jp_1. Kuroda-san's idea proposed in
this thread is reducing cached SPI plans by combining several plans into one.
But we are also considering another option to solve this problem, which
makes users to release cached SPI plans for referential integrity as well as
plain cached plans with DISCARD ALL. To do that, we added a new
function, RI_DropAllPreparedPlan() which deletes all plans from
ri_query_cache and
modified DISCARD ALL to execute that function.
I tested using a test case yamada-san attached in [2]/messages/by-id/cab4b85d-9292-967d-adf2-be0d803c3e23@nttcom.co.jp_1 as follows:
[Before DISCARD ALL]
=# SELECT name, sum(used_bytes) as bytes,
pg_size_pretty(sum(used_bytes)) FROM pg_backend_memory_contexts WHERE
name LIKE 'Cached%' GROUP BY name;
name | bytes | pg_size_pretty
------------------+-----------+----------------
CachedPlanQuery | 1326280 | 1295 kB
CachedPlanSource | 1474616 | 1440 kB
CachedPlan | 744009168 | 710 MB
(3 rows)
[After DISCARD ALL]
=# DISCARD ALL;
DISCARD ALL
=# SELECT name, sum(used_bytes) as bytes,
pg_size_pretty(sum(used_bytes)) FROM pg_backend_memory_contexts WHERE
name LIKE 'Cached%' GROUP BY name;
name | bytes | pg_size_pretty
------------------+-------+----------------
CachedPlanQuery | 10280 | 10 kB
CachedPlanSource | 14616 | 14 kB
CachedPlan | 13168 | 13 kB
(3 rows)
In addition to that, a following case would be solved with this approach:
When many processes are referencing many tables defined foreign key
constraints thoroughly, a huge amount of memory will be consumed
regardless of whether referenced tables are partitioned or not.
Attached the patch. Any thoughts?
[1]: /messages/by-id/cab4b85d-9292-967d-adf2-be0d803c3e23@nttcom.co.jp_1
[2]: /messages/by-id/cab4b85d-9292-967d-adf2-be0d803c3e23@nttcom.co.jp_1
--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center
Attachments:
v1_discard_ri_spi_plans.patchapplication/octet-stream; name=v1_discard_ri_spi_plans.patchDownload
diff --git a/src/backend/commands/discard.c b/src/backend/commands/discard.c
index 57d3d7dd9b..3fc5106529 100644
--- a/src/backend/commands/discard.c
+++ b/src/backend/commands/discard.c
@@ -19,6 +19,7 @@
#include "commands/discard.h"
#include "commands/prepare.h"
#include "commands/sequence.h"
+#include "commands/trigger.h"
#include "utils/guc.h"
#include "utils/portal.h"
@@ -73,6 +74,7 @@ DiscardAll(bool isTopLevel)
Async_UnlistenAll();
LockReleaseAll(USER_LOCKMETHOD, true);
ResetPlanCache();
+ RI_DropAllPreparedPlan();
ResetTempTableNamespace();
ResetSequenceCaches();
}
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 6e3a41062f..bf61a27ca3 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2646,7 +2646,6 @@ ri_HashPreparedPlan(RI_QueryKey *key, SPIPlanPtr plan)
entry->plan = plan;
}
-
/*
* ri_KeysEqual -
*
@@ -2887,3 +2886,30 @@ RI_FKey_trigger_type(Oid tgfoid)
return RI_TRIGGER_NONE;
}
+
+/*
+ * RI_DropAllPreparedPlan -
+ *
+ * Delete all plans from our private SPI query plan hashtable.
+ */
+void
+RI_DropAllPreparedPlan(void)
+{
+ HASH_SEQ_STATUS seq;
+ RI_QueryHashEntry *entry;
+
+ /* nothing cached */
+ if(!ri_query_cache)
+ return;
+
+ /* walk over cache */
+ hash_seq_init(&seq, ri_query_cache);
+ while((entry = hash_seq_search(&seq)) != NULL)
+ {
+ /* Relase the plancache entry */
+ SPI_freeplan(entry->plan);
+
+ /* Now we can remove the hash table entry */
+ hash_search(ri_query_cache, &entry->key, HASH_REMOVE, NULL);
+ }
+}
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 9e557cfbce..b33bb31963 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -250,6 +250,7 @@ extern bool AfterTriggerPendingOnRel(Oid relid);
/*
* in utils/adt/ri_triggers.c
*/
+extern void RI_DropAllPreparedPlan(void);
extern bool RI_FKey_pk_upd_check_required(Trigger *trigger, Relation pk_rel,
TupleTableSlot *old_slot, TupleTableSlot *new_slot);
extern bool RI_FKey_fk_upd_check_required(Trigger *trigger, Relation fk_rel,
In addition to that, a following case would be solved with this approach:
When many processes are referencing many tables defined foreign key
constraints thoroughly, a huge amount of memory will be consumed
regardless of whether referenced tables are partitioned or not.Attached the patch. Any thoughts?
Amit Langote has done some great work at eliminating SPI from INSERT/UPDATE
triggers entirely, thus reducing the number of cached plans considerably.
I think he was hoping to have a patch formalized this week, if time allowed.
It doesn't have DELETE triggers in it, so this patch might still have good
value for deletes on a commonly used enumeration table.
However, our efforts might be better focused on eliminating SPI from delete
triggers as well, an admittedly harder task.
On Wed, Jan 13, 2021 at 1:03 PM Corey Huinker <corey.huinker@gmail.com>
wrote:
In addition to that, a following case would be solved with this approach:
When many processes are referencing many tables defined foreign key
constraints thoroughly, a huge amount of memory will be consumed
regardless of whether referenced tables are partitioned or not.Attached the patch. Any thoughts?
Amit Langote has done some great work at eliminating SPI from
INSERT/UPDATE triggers entirely, thus reducing the number of cached plans
considerably.I think he was hoping to have a patch formalized this week, if time
allowed.It doesn't have DELETE triggers in it, so this patch might still have good
value for deletes on a commonly used enumeration table.However, our efforts might be better focused on eliminating SPI from
delete triggers as well, an admittedly harder task.
Amit's patch is now available in this thread [1]/messages/by-id/CA+HiwqGkfJfYdeq5vHPh6eqPKjSbfpDDY+j-kXYFePQedtSLeg@mail.gmail.com. I'm curious if it has any
effect on your memory pressure issue.
[1]: /messages/by-id/CA+HiwqGkfJfYdeq5vHPh6eqPKjSbfpDDY+j-kXYFePQedtSLeg@mail.gmail.com
/messages/by-id/CA+HiwqGkfJfYdeq5vHPh6eqPKjSbfpDDY+j-kXYFePQedtSLeg@mail.gmail.com
Hi Corey,
Thank you for sharing.
Amit's patch is now available in this thread [1]. I'm curious if it has any effect on your memory pressure issue.
I just found that thread. I'll check the patch.
--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center
On 2021-01-13 09:47, yuzuko wrote:
But we are also considering another option to solve this problem, which
makes users to release cached SPI plans for referential integrity as well as
plain cached plans with DISCARD ALL. To do that, we added a new
function, RI_DropAllPreparedPlan() which deletes all plans from
ri_query_cache and
modified DISCARD ALL to execute that function.
I don't have a comment on the memory management issue, but I think the
solution of dropping all cached plans as part of DISCARD ALL seems a bit
too extreme of a solution. In the context of connection pooling,
getting a new session with pre-cached plans seems like a good thing, and
this change could potentially have a performance impact without a
practical way to opt out.
Hello,
Thank you for your comments.
Following Corey's advice, I applied Amit's patches proposed in this
email [1]/messages/by-id/CA+HiwqG1qQuBwApueaUfA855UJ4TiSgFkPF34hQDWx3tOChV5w@mail.gmail.com, and confirmed our memory pressure problem was solved.
So dropping cached plan with DISCARD is not necessary anymore.
[1]: /messages/by-id/CA+HiwqG1qQuBwApueaUfA855UJ4TiSgFkPF34hQDWx3tOChV5w@mail.gmail.com
--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center
Hello,
I thought about this suggestion again.
Amit's patch suggested in the thread [1]/messages/by-id/CA+HiwqGkfJfYdeq5vHPh6eqPKjSbfpDDY+j-kXYFePQedtSLeg@mail.gmail.com can eliminate SPI plans from
INSERT/UPDATE triggers, so our memory pressure issue would be solved.
But as far as I can see that thread, Amit's patch doesn't cover DELETE case.
It is not a common case, but there is a risk of pressing memory
capacity extremely.
Considering that, this suggestion might still have good value as Corey
said before.
I improved a patch according to Peter's following comment :
but I think the
solution of dropping all cached plans as part of DISCARD ALL seems a bit
too extreme of a solution. In the context of connection pooling,
getting a new session with pre-cached plans seems like a good thing, and
this change could potentially have a performance impact without a
practical way to opt out.
In a new patch, I separated discarding SPI Plan caches for RI from DISCARD ALL
and added a new option "RIPLANS" to DISCARD command to do that. Also a new
function, ResetRIPlanCache() which clears SPI plan caches is called by
DISCARD ALL
or DISCARD RIPLANS. The amount of modification is small and this option can be
removed instantly when it is no longer needed, so I think we can use
this patch as
a provisional solution.
Any thoughts?
[1]: /messages/by-id/CA+HiwqGkfJfYdeq5vHPh6eqPKjSbfpDDY+j-kXYFePQedtSLeg@mail.gmail.com
--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center
Attachments:
v2_discard_ri_spi_plans.patchapplication/octet-stream; name=v2_discard_ri_spi_plans.patchDownload
diff --git a/src/backend/commands/discard.c b/src/backend/commands/discard.c
index 57d3d7dd9b..e376573612 100644
--- a/src/backend/commands/discard.c
+++ b/src/backend/commands/discard.c
@@ -19,13 +19,14 @@
#include "commands/discard.h"
#include "commands/prepare.h"
#include "commands/sequence.h"
+#include "commands/trigger.h"
#include "utils/guc.h"
#include "utils/portal.h"
static void DiscardAll(bool isTopLevel);
/*
- * DISCARD { ALL | SEQUENCES | TEMP | PLANS }
+ * DISCARD { ALL | SEQUENCES | TEMP | PLANS | RIPLANS }
*/
void
DiscardCommand(DiscardStmt *stmt, bool isTopLevel)
@@ -48,6 +49,10 @@ DiscardCommand(DiscardStmt *stmt, bool isTopLevel)
ResetTempTableNamespace();
break;
+ case DISCARD_RIPLANS:
+ ResetRIPlanCache();
+ break;
+
default:
elog(ERROR, "unrecognized DISCARD target: %d", stmt->target);
}
@@ -73,6 +78,7 @@ DiscardAll(bool isTopLevel)
Async_UnlistenAll();
LockReleaseAll(USER_LOCKMETHOD, true);
ResetPlanCache();
+ ResetRIPlanCache();
ResetTempTableNamespace();
ResetSequenceCaches();
}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 652be0b96d..bede6c0b85 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -685,7 +685,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
RANGE READ REAL REASSIGN RECHECK RECURSIVE REF REFERENCES REFERENCING
REFRESH REINDEX RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA
- RESET RESTART RESTRICT RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROLLUP
+ RESET RESTART RESTRICT RETURNING RETURNS REVOKE RIGHT RIPLANS ROLE ROLLBACK ROLLUP
ROUTINE ROUTINES ROW ROWS RULE
SAVEPOINT SCHEMA SCHEMAS SCROLL SEARCH SECOND_P SECURITY SELECT SEQUENCE SEQUENCES
@@ -1830,7 +1830,7 @@ CheckPointStmt:
/*****************************************************************************
*
- * DISCARD { ALL | TEMP | PLANS | SEQUENCES }
+ * DISCARD { ALL | TEMP | PLANS | SEQUENCES | RIPLANS }
*
*****************************************************************************/
@@ -1865,7 +1865,12 @@ DiscardStmt:
n->target = DISCARD_SEQUENCES;
$$ = (Node *) n;
}
-
+ | DISCARD RIPLANS
+ {
+ DiscardStmt *n = makeNode(DiscardStmt);
+ n->target = DISCARD_RIPLANS;
+ $$ = (Node *) n;
+ }
;
@@ -15656,6 +15661,7 @@ type_func_name_keyword:
| OUTER_P
| OVERLAPS
| RIGHT
+ | RIPLANS
| SIMILAR
| TABLESAMPLE
| VERBOSE
@@ -16053,6 +16059,7 @@ bare_label_keyword:
| RETURNS
| REVOKE
| RIGHT
+ | RIPLANS
| ROLE
| ROLLBACK
| ROLLUP
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 05bb698cf4..48e087e28b 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -2822,6 +2822,9 @@ CreateCommandTag(Node *parsetree)
case DISCARD_SEQUENCES:
tag = CMDTAG_DISCARD_SEQUENCES;
break;
+ case DISCARD_RIPLANS:
+ tag = CMDTAG_DISCARD_RIPLANS;
+ break;
default:
tag = CMDTAG_UNKNOWN;
}
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 6e3a41062f..daa1dac088 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2646,7 +2646,6 @@ ri_HashPreparedPlan(RI_QueryKey *key, SPIPlanPtr plan)
entry->plan = plan;
}
-
/*
* ri_KeysEqual -
*
@@ -2887,3 +2886,30 @@ RI_FKey_trigger_type(Oid tgfoid)
return RI_TRIGGER_NONE;
}
+
+/*
+ * ResetRIPlanCache -
+ *
+ * Delete all plans from our private SPI query plan hashtable.
+ */
+void
+ResetRIPlanCache(void)
+{
+ HASH_SEQ_STATUS seq;
+ RI_QueryHashEntry *entry;
+
+ /* nothing cached */
+ if(!ri_query_cache)
+ return;
+
+ /* walk over cache */
+ hash_seq_init(&seq, ri_query_cache);
+ while((entry = hash_seq_search(&seq)) != NULL)
+ {
+ /* Relase the plancache entry */
+ SPI_freeplan(entry->plan);
+
+ /* Now we can remove the hash table entry */
+ hash_search(ri_query_cache, &entry->key, HASH_REMOVE, NULL);
+ }
+}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 9f0208ac49..ad05d2bc34 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3097,7 +3097,7 @@ psql_completion(const char *text, int start, int end)
/* DISCARD */
else if (Matches("DISCARD"))
- COMPLETE_WITH("ALL", "PLANS", "SEQUENCES", "TEMP");
+ COMPLETE_WITH("ALL", "PLANS", "SEQUENCES", "TEMP", "RIPLANS");
/* DO */
else if (Matches("DO"))
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 9e557cfbce..76ec9bf57b 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -250,6 +250,7 @@ extern bool AfterTriggerPendingOnRel(Oid relid);
/*
* in utils/adt/ri_triggers.c
*/
+extern void ResetRIPlanCache(void);
extern bool RI_FKey_pk_upd_check_required(Trigger *trigger, Relation pk_rel,
TupleTableSlot *old_slot, TupleTableSlot *new_slot);
extern bool RI_FKey_fk_upd_check_required(Trigger *trigger, Relation fk_rel,
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 236832a2ca..616a40673d 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3349,7 +3349,8 @@ typedef enum DiscardMode
DISCARD_ALL,
DISCARD_PLANS,
DISCARD_SEQUENCES,
- DISCARD_TEMP
+ DISCARD_TEMP,
+ DISCARD_RIPLANS
} DiscardMode;
typedef struct DiscardStmt
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index 28083aaac9..1a6b5d0671 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -350,6 +350,7 @@ PG_KEYWORD("returning", RETURNING, RESERVED_KEYWORD, AS_LABEL)
PG_KEYWORD("returns", RETURNS, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("revoke", REVOKE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("right", RIGHT, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL)
+PG_KEYWORD("riplans", RIPLANS, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL)
PG_KEYWORD("role", ROLE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("rollback", ROLLBACK, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("rollup", ROLLUP, UNRESERVED_KEYWORD, BARE_LABEL)
diff --git a/src/include/tcop/cmdtaglist.h b/src/include/tcop/cmdtaglist.h
index 9ba24d4ca9..391db5af7e 100644
--- a/src/include/tcop/cmdtaglist.h
+++ b/src/include/tcop/cmdtaglist.h
@@ -133,6 +133,7 @@ PG_CMDTAG(CMDTAG_DISCARD_ALL, "DISCARD ALL", false, false, false)
PG_CMDTAG(CMDTAG_DISCARD_PLANS, "DISCARD PLANS", false, false, false)
PG_CMDTAG(CMDTAG_DISCARD_SEQUENCES, "DISCARD SEQUENCES", false, false, false)
PG_CMDTAG(CMDTAG_DISCARD_TEMP, "DISCARD TEMP", false, false, false)
+PG_CMDTAG(CMDTAG_DISCARD_RIPLANS, "DISCARD RIPLANS", false, false, false)
PG_CMDTAG(CMDTAG_DO, "DO", false, false, false)
PG_CMDTAG(CMDTAG_DROP_ACCESS_METHOD, "DROP ACCESS METHOD", true, false, false)
PG_CMDTAG(CMDTAG_DROP_AGGREGATE, "DROP AGGREGATE", true, false, false)
On Wed, Mar 10, 2021 at 1:49 PM yuzuko <yuzukohosoya@gmail.com> wrote:
Hello,
I thought about this suggestion again.
Amit's patch suggested in the thread [1] can eliminate SPI plans from
INSERT/UPDATE triggers, so our memory pressure issue would be solved.
But as far as I can see that thread, Amit's patch doesn't cover DELETE case.
It is not a common case, but there is a risk of pressing memory
capacity extremely.
Considering that, this suggestion might still have good value as Corey
said before.I improved a patch according to Peter's following comment :
but I think the
solution of dropping all cached plans as part of DISCARD ALL seems a bit
too extreme of a solution. In the context of connection pooling,
getting a new session with pre-cached plans seems like a good thing, and
this change could potentially have a performance impact without a
practical way to opt out.In a new patch, I separated discarding SPI Plan caches for RI from DISCARD ALL
and added a new option "RIPLANS" to DISCARD command to do that. Also a new
function, ResetRIPlanCache() which clears SPI plan caches is called by
DISCARD ALL
or DISCARD RIPLANS. The amount of modification is small and this option can be
removed instantly when it is no longer needed, so I think we can use
this patch as
a provisional solution.Any thoughts?
The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".
Regards,
Vignesh
yuzuko <yuzukohosoya@gmail.com> writes:
I improved a patch according to Peter's following comment :
but I think the
solution of dropping all cached plans as part of DISCARD ALL seems a bit
too extreme of a solution. In the context of connection pooling,
getting a new session with pre-cached plans seems like a good thing, and
this change could potentially have a performance impact without a
practical way to opt out.
In a new patch, I separated discarding SPI Plan caches for RI from DISCARD ALL
and added a new option "RIPLANS" to DISCARD command to do that.
I'm not really comfortable with exposing this as a user-visible behavior.
In particular ...
The amount of modification is small and this option can be
removed instantly when it is no longer needed, so I think we can use
this patch as a provisional solution.
... this claim seems like nonsense. Once there is user-accessible syntax
for something, we basically can't ever take that out, because it'd break
applications that call that command.
It seems like the real problem with the ri_query_cache cache is that
it will grow without limit. I wonder whether it wouldn't be better
to put some upper bound on it, along with a simple least-recently-used
rule to choose which entry to discard. (Perhaps the same goes for
ri_constraint_cache and/or ri_compare_cache, although their entry sizes
are smaller.)
regards, tom lane