public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2] Do not use RelationNeedsWAL to identify relation persistence
4+ messages / 3 participants
[nested] [flat]
* [PATCH v2] Do not use RelationNeedsWAL to identify relation persistence
@ 2021-01-18 05:47 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Kyotaro Horiguchi @ 2021-01-18 05:47 UTC (permalink / raw)
RelationNeedsWAL() may return false for a permanent relation when
wal_level=minimal and the relation is created or truncated in the
current transaction. Directly examine relpersistence instead of using
the function to know relation persistence.
---
src/backend/catalog/pg_publication.c | 2 +-
src/backend/optimizer/util/plancat.c | 3 ++-
src/include/utils/rel.h | 9 ++++++++-
src/include/utils/snapmgr.h | 2 +-
src/test/subscription/t/001_rep_changes.pl | 20 +++++++++++++++++++-
5 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 5f8e1c64e1..84d2efcfd2 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -67,7 +67,7 @@ check_publication_add_relation(Relation targetrel)
errdetail("System tables cannot be added to publications.")));
/* UNLOGGED and TEMP relations cannot be part of publication. */
- if (!RelationNeedsWAL(targetrel))
+ if (targetrel->rd_rel->relpersistence != RELPERSISTENCE_PERMANENT)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("table \"%s\" cannot be replicated",
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index da322b453e..177e6e336a 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -126,7 +126,8 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
relation = table_open(relationObjectId, NoLock);
/* Temporary and unlogged relations are inaccessible during recovery. */
- if (!RelationNeedsWAL(relation) && RecoveryInProgress())
+ if (relation->rd_rel->relpersistence != RELPERSISTENCE_PERMANENT &&
+ RecoveryInProgress())
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot access temporary or unlogged relations during recovery")));
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 10b63982c0..1e2c11fdd3 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -552,9 +552,16 @@ typedef struct ViewOptions
(relation)->rd_smgr->smgr_targblock = (targblock); \
} while (0)
+/*
+ * RelationIsPermanent
+ * True if relation is WAL-logged.
+ */
+#define RelationIsWalLogged(relation) \
+ ((relation)->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT)
+
/*
* RelationNeedsWAL
- * True if relation needs WAL.
+ * True if relation needs WAL at the time.
*
* Returns false if wal_level = minimal and this relation is created or
* truncated in the current transaction. See "Skipping WAL for New
diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index 579be352c5..35acccb29c 100644
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -37,7 +37,7 @@
*/
#define RelationAllowsEarlyPruning(rel) \
( \
- RelationNeedsWAL(rel) \
+ rel->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT \
&& !IsCatalogRelation(rel) \
&& !RelationIsAccessibleInLogicalDecoding(rel) \
)
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 0680f44a1a..ed9b48e8bc 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -3,7 +3,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 23;
+use Test::More tests => 24;
# Initialize publisher node
my $node_publisher = get_new_node('publisher');
@@ -358,3 +358,21 @@ is($result, qq(0), 'check replication origin was dropped on subscriber');
$node_subscriber->stop('fast');
$node_publisher->stop('fast');
+
+#
+# CREATE PUBLICATION while wal_level=mimal should succeed, with a WARNING
+$node_publisher->append_conf(
+ 'postgresql.conf', qq(
+wal_level=minimal
+max_wal_senders=0
+));
+$node_publisher->start;
+($result, my $retout, my $reterr) = $node_publisher->psql(
+ 'postgres', qq{
+BEGIN;
+CREATE TABLE skip_wal();
+CREATE PUBLICATION tap_pub2 FOR TABLE skip_wal;
+ROLLBACK;
+});
+ok($reterr =~ m/WARNING: wal_level is insufficient to publish logical changes/,
+ 'test CREATE PUBLICATION can be done while wal_leve=minimal');
--
2.27.0
----Next_Part(Mon_Jan_18_15_08_38_2021_069)----
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Memory leak in incremental sort re-scan
@ 2023-06-15 13:19 Tomas Vondra <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Tomas Vondra @ 2023-06-15 13:19 UTC (permalink / raw)
To: Laurenz Albe <[email protected]>; [email protected]
Hi,
On 6/15/23 13:48, Laurenz Albe wrote:
> ExecIncrementalSort() calls tuplesort_begin_common(), which creates the "TupleSort main"
> and "TupleSort sort" memory contexts, and ExecEndIncrementalSort() calls tuplesort_end(),
> which destroys them.
> But ExecReScanIncrementalSort() only resets the memory contexts. Since the next call to
> ExecIncrementalSort() will create them again, we end up leaking these contexts for every
> re-scan.
>
> Here is a reproducer with the regression test database:
>
> SET enable_sort = off;
> SET enable_hashjoin = off;
> SET enable_mergejoin = off;
> SET enable_material = off;
>
> SELECT t.unique2, t2.r
> FROM tenk1 AS t
> JOIN (SELECT unique1,
> row_number() OVER (ORDER BY hundred, thousand) AS r
> FROM tenk1
> OFFSET 0) AS t2
> ON t.unique1 + 0 = t2.unique1
> WHERE t.unique1 < 1000;
>
> The execution plan:
>
> Nested Loop
> Join Filter: ((t.unique1 + 0) = tenk1.unique1)
> -> Bitmap Heap Scan on tenk1 t
> Recheck Cond: (unique1 < 1000)
> -> Bitmap Index Scan on tenk1_unique1
> Index Cond: (unique1 < 1000)
> -> WindowAgg
> -> Incremental Sort
> Sort Key: tenk1.hundred, tenk1.thousand
> Presorted Key: tenk1.hundred
> -> Index Scan using tenk1_hundred on tenk1
>
>
> A memory context dump at the end of the execution looks like this:
>
> ExecutorState: 262144 total in 6 blocks; 74136 free (29 chunks); 188008 used
> TupleSort main: 32832 total in 2 blocks; 7320 free (0 chunks); 25512 used
> TupleSort sort: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
> Caller tuples: 8192 total in 1 blocks (0 chunks); 7984 free (0 chunks); 208 used
> TupleSort main: 32832 total in 2 blocks; 7256 free (0 chunks); 25576 used
> TupleSort sort: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
> Caller tuples: 8192 total in 1 blocks (0 chunks); 7984 free (0 chunks); 208 used
> TupleSort main: 32832 total in 2 blocks; 7320 free (0 chunks); 25512 used
> TupleSort sort: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
> Caller tuples: 8192 total in 1 blocks (0 chunks); 7984 free (0 chunks); 208 used
> [many more]
> 1903 more child contexts containing 93452928 total in 7597 blocks; 44073240 free (0 chunks); 49379688 used
>
>
> The following patch fixes the problem for me:
>
> --- a/src/backend/executor/nodeIncrementalSort.c
> +++ b/src/backend/executor/nodeIncrementalSort.c
> @@ -1145,21 +1145,16 @@ ExecReScanIncrementalSort(IncrementalSortState *node)
> node->execution_status = INCSORT_LOADFULLSORT;
>
> /*
> - * If we've set up either of the sort states yet, we need to reset them.
> - * We could end them and null out the pointers, but there's no reason to
> - * repay the setup cost, and because ExecIncrementalSort guards presorted
> - * column functions by checking to see if the full sort state has been
> - * initialized yet, setting the sort states to null here might actually
> - * cause a leak.
> + * Release tuplesort resources.
> */
> if (node->fullsort_state != NULL)
> {
> - tuplesort_reset(node->fullsort_state);
> + tuplesort_end(node->fullsort_state);
> node->fullsort_state = NULL;
> }
> if (node->prefixsort_state != NULL)
> {
> - tuplesort_reset(node->prefixsort_state);
> + tuplesort_end(node->prefixsort_state);
> node->prefixsort_state = NULL;
> }
>
>
> The original comment hints that this might mot be the correct thing to do...
>
I think it's correct, but I need to look at the code more closely - it's
been a while. The code is a bit silly, as it resets the tuplesort and
then throws away all the pointers - so what could the _end() break?
AFAICS the comment says that we can't just do tuplesort_reset and keep
the pointers, because some other code depends on them being NULL.
In hindsight, that's a bit awkward - it'd probably be better to have a
separate flag, which would allow us to just reset the tuplesort.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Memory leak in incremental sort re-scan
@ 2023-06-15 20:11 Tom Lane <[email protected]>
parent: Tomas Vondra <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Tom Lane @ 2023-06-15 20:11 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Laurenz Albe <[email protected]>; [email protected]
Tomas Vondra <[email protected]> writes:
> On 6/15/23 13:48, Laurenz Albe wrote:
>> ExecIncrementalSort() calls tuplesort_begin_common(), which creates the "TupleSort main"
>> and "TupleSort sort" memory contexts, and ExecEndIncrementalSort() calls tuplesort_end(),
>> which destroys them.
>> But ExecReScanIncrementalSort() only resets the memory contexts.
> I think it's correct, but I need to look at the code more closely - it's
> been a while. The code is a bit silly, as it resets the tuplesort and
> then throws away all the pointers - so what could the _end() break?
The report at [1] seems to be the same issue of ExecReScanIncrementalSort
leaking memory. I applied Laurenz's fix, and that greatly reduces the
speed of leak but doesn't remove the problem entirely. It looks like
the remaining issue is that the data computed by preparePresortedCols() is
recomputed each time we rescan the node. This seems entirely gratuitous,
because there's nothing in that that could change across rescans.
I see zero leakage in that example after applying the attached quick
hack. (It might be better to make the check in the caller, or to just
move the call to ExecInitIncrementalSort.)
regards, tom lane
[1] https://www.postgresql.org/message-id/db03c582-086d-e7cd-d4a1-3bc722f81765%40inf.ethz.ch
Attachments:
[text/x-diff] stop-incremental-sort-rescan-leaks.patch (1.2K, ../../[email protected]/2-stop-incremental-sort-rescan-leaks.patch)
download | inline diff:
diff --git a/src/backend/executor/nodeIncrementalSort.c b/src/backend/executor/nodeIncrementalSort.c
index 34257ce34b..655b1c30e1 100644
--- a/src/backend/executor/nodeIncrementalSort.c
+++ b/src/backend/executor/nodeIncrementalSort.c
@@ -166,6 +166,9 @@ preparePresortedCols(IncrementalSortState *node)
{
IncrementalSort *plannode = castNode(IncrementalSort, node->ss.ps.plan);
+ if (node->presorted_keys)
+ return;
+
node->presorted_keys =
(PresortedKeyData *) palloc(plannode->nPresortedCols *
sizeof(PresortedKeyData));
@@ -1140,7 +1143,6 @@ ExecReScanIncrementalSort(IncrementalSortState *node)
node->outerNodeDone = false;
node->n_fullsort_remaining = 0;
node->bound_Done = 0;
- node->presorted_keys = NULL;
node->execution_status = INCSORT_LOADFULLSORT;
@@ -1154,12 +1156,12 @@ ExecReScanIncrementalSort(IncrementalSortState *node)
*/
if (node->fullsort_state != NULL)
{
- tuplesort_reset(node->fullsort_state);
+ tuplesort_end(node->fullsort_state);
node->fullsort_state = NULL;
}
if (node->prefixsort_state != NULL)
{
- tuplesort_reset(node->prefixsort_state);
+ tuplesort_end(node->prefixsort_state);
node->prefixsort_state = NULL;
}
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Memory leak in incremental sort re-scan
@ 2023-06-15 20:30 Tomas Vondra <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Tomas Vondra @ 2023-06-15 20:30 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Laurenz Albe <[email protected]>; [email protected]
On 6/15/23 22:11, Tom Lane wrote:
> Tomas Vondra <[email protected]> writes:
>> On 6/15/23 13:48, Laurenz Albe wrote:
>>> ExecIncrementalSort() calls tuplesort_begin_common(), which creates the "TupleSort main"
>>> and "TupleSort sort" memory contexts, and ExecEndIncrementalSort() calls tuplesort_end(),
>>> which destroys them.
>>> But ExecReScanIncrementalSort() only resets the memory contexts.
>
>> I think it's correct, but I need to look at the code more closely - it's
>> been a while. The code is a bit silly, as it resets the tuplesort and
>> then throws away all the pointers - so what could the _end() break?
>
> The report at [1] seems to be the same issue of ExecReScanIncrementalSort
> leaking memory.
Funny how these reports often come in pairs ...
> I applied Laurenz's fix, and that greatly reduces the
> speed of leak but doesn't remove the problem entirely. It looks like
> the remaining issue is that the data computed by preparePresortedCols() is
> recomputed each time we rescan the node. This seems entirely gratuitous,
> because there's nothing in that that could change across rescans.
Yeah, I was wondering about that too when I skimmed over that code
earlier today.
> I see zero leakage in that example after applying the attached quick
> hack. (It might be better to make the check in the caller, or to just
> move the call to ExecInitIncrementalSort.)
>
Thanks for looking. Are you planning to work on this and push the fix,
or do you want me to finish this up?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2023-06-15 20:30 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-18 05:47 [PATCH v2] Do not use RelationNeedsWAL to identify relation persistence Kyotaro Horiguchi <[email protected]>
2023-06-15 13:19 Re: Memory leak in incremental sort re-scan Tomas Vondra <[email protected]>
2023-06-15 20:11 ` Re: Memory leak in incremental sort re-scan Tom Lane <[email protected]>
2023-06-15 20:30 ` Re: Memory leak in incremental sort re-scan Tomas Vondra <[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