agora inbox for [email protected]
help / color / mirror / Atom feedRe: [HACKERS] Lazy hash table for XidInMVCCSnapshot (helps Zipfian a bit)
7+ messages / 5 participants
[nested] [flat]
* Re: [HACKERS] Lazy hash table for XidInMVCCSnapshot (helps Zipfian a bit)
@ 2018-11-04 12:27 Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Dolgov @ 2018-11-04 12:27 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; Tomas Vondra <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; [email protected]; Robert Haas <[email protected]>; Alexander Korotkov <[email protected]>; pgsql-hackers; [email protected]
> On Sun, 1 Apr 2018 at 19:58, Yura Sokolov <[email protected]> wrote:
>
> I didn't change serialized format. Therefore is no need to change
> SerializeSnapshot.
> But in-memory representation were changed, so RestoreSnapshot is changed.
This patch went through the last tree commit fests without any noticeable
activity, but cfbot says it still applies and doesn't break any tests. Taking
into account potential performance improvements, I believe it would be a pity
to stop at this point.
Yura, what're your plans about it? If I understand correctly, there are still
some commentaries, that were not answered from the last few messages. At the
same time can anyone from active reviewers (Tomas, Amit) look at it to agree on
what should be done to push it forward?
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: [HACKERS] Lazy hash table for XidInMVCCSnapshot (helps Zipfian a bit)
@ 2018-11-30 15:05 Dmitry Dolgov <[email protected]>
parent: Dmitry Dolgov <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Dmitry Dolgov @ 2018-11-30 15:05 UTC (permalink / raw)
To: Юрий Соколов <[email protected]>; +Cc: Amit Kapila <[email protected]>; Tomas Vondra <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; [email protected]; Robert Haas <[email protected]>; Alexander Korotkov <[email protected]>; pgsql-hackers; [email protected]
>On Sun, Nov 4, 2018 at 1:27 PM Dmitry Dolgov <[email protected]> wrote:
>
> > On Sun, 1 Apr 2018 at 19:58, Yura Sokolov <[email protected]> wrote:
> >
> > I didn't change serialized format. Therefore is no need to change
> > SerializeSnapshot.
> > But in-memory representation were changed, so RestoreSnapshot is changed.
>
> This patch went through the last tree commit fests without any noticeable
> activity, but cfbot says it still applies and doesn't break any tests. Taking
> into account potential performance improvements, I believe it would be a pity
> to stop at this point.
>
> Yura, what're your plans about it? If I understand correctly, there are still
> some commentaries, that were not answered from the last few messages. At the
> same time can anyone from active reviewers (Tomas, Amit) look at it to agree on
> what should be done to push it forward?
Due to lack of response I'm marking it as "Returned with feedback". Feel free
to resubmit a new version though.
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH] Remove entries that haven't been used for a certain time
@ 2019-03-01 04:32 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Kyotaro Horiguchi @ 2019-03-01 04:32 UTC (permalink / raw)
Catcache entries happen to be left alone for several reasons. It is
not desirable that such useless entries eat up memory. Catcache
pruning feature removes entries that haven't been accessed for a
certain time before enlarging hash array.
---
doc/src/sgml/config.sgml | 19 +++++
src/backend/tcop/postgres.c | 2 +
src/backend/utils/cache/catcache.c | 105 +++++++++++++++++++++++++-
src/backend/utils/misc/guc.c | 12 +++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/utils/catcache.h | 16 ++++
6 files changed, 152 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index d383de2512..4231235447 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1677,6 +1677,25 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-catalog-cache-prune-min-age" xreflabel="catalog_cache_prune_min_age">
+ <term><varname>catalog_cache_prune_min_age</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>catalog_cache_prune_min_age</varname> configuration
+ parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Specifies the minimum amount of unused time in seconds at which a
+ system catalog cache entry is removed. -1 indicates that this feature
+ is disabled at all. The value defaults to 300 seconds (<literal>5
+ minutes</literal>). The entries that are not used for the duration
+ can be removed to prevent catalog cache from bloating with useless
+ entries.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-max-stack-depth" xreflabel="max_stack_depth">
<term><varname>max_stack_depth</varname> (<type>integer</type>)
<indexterm>
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index f9ce3d8f22..acab473d34 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -71,6 +71,7 @@
#include "tcop/pquery.h"
#include "tcop/tcopprot.h"
#include "tcop/utility.h"
+#include "utils/catcache.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
@@ -2575,6 +2576,7 @@ start_xact_command(void)
* not desired, the timeout has to be disabled explicitly.
*/
enable_statement_timeout();
+ SetCatCacheClock(GetCurrentStatementStartTimestamp());
}
static void
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index d05930bc4c..52586bd415 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -38,6 +38,7 @@
#include "utils/rel.h"
#include "utils/resowner_private.h"
#include "utils/syscache.h"
+#include "utils/timeout.h"
/* #define CACHEDEBUG */ /* turns DEBUG elogs on */
@@ -60,9 +61,18 @@
#define CACHE_elog(...)
#endif
+/*
+ * GUC variable to define the minimum age of entries that will be considered
+ * to be evicted in seconds. -1 to disable the feature.
+ */
+int catalog_cache_prune_min_age = 300;
+
/* Cache management header --- pointer is NULL until created */
static CatCacheHeader *CacheHdr = NULL;
+/* Clock for the last accessed time of a catcache entry. */
+TimestampTz catcacheclock = 0;
+
static inline HeapTuple SearchCatCacheInternal(CatCache *cache,
int nkeys,
Datum v1, Datum v2,
@@ -850,9 +860,83 @@ InitCatCache(int id,
*/
MemoryContextSwitchTo(oldcxt);
+ /* initialize catcache reference clock if haven't done yet */
+ if (catcacheclock == 0)
+ catcacheclock = GetCurrentTimestamp();
+
return cp;
}
+/*
+ * CatCacheCleanupOldEntries - Remove infrequently-used entries
+ *
+ * Catcache entries happen to be left unused for a long time for several
+ * reasons. Remove such entries to prevent catcache from bloating. It is based
+ * on the similar algorithm with buffer eviction. Entries that are accessed
+ * several times in a certain period live longer than those that have had less
+ * access in the same duration.
+ */
+static bool
+CatCacheCleanupOldEntries(CatCache *cp)
+{
+ int nremoved = 0;
+ int i;
+
+ /* Return immediately if disabled */
+ if (catalog_cache_prune_min_age == 0)
+ return false;
+
+ /* Scan over the whole hash to find entries to remove */
+ for (i = 0 ; i < cp->cc_nbuckets ; i++)
+ {
+ dlist_mutable_iter iter;
+
+ dlist_foreach_modify(iter, &cp->cc_bucket[i])
+ {
+ CatCTup *ct = dlist_container(CatCTup, cache_elem, iter.cur);
+ long entry_age;
+ int us;
+
+ /* Don't remove referenced entries */
+ if (ct->refcount != 0 ||
+ (ct->c_list && ct->c_list->refcount != 0))
+ continue;
+
+ /*
+ * Calculate the duration from the time from the last access to
+ * the "current" time. catcacheclock is updated per-statement
+ * basis and additionaly udpated periodically during a long
+ * running query.
+ */
+ TimestampDifference(ct->lastaccess, catcacheclock, &entry_age, &us);
+
+ if (entry_age < catalog_cache_prune_min_age)
+ continue;
+
+ /*
+ * Entries that are not accessed after the last pruning are
+ * removed in that seconds, and their lives are prolonged
+ * according to how many times they are accessed up to three times
+ * of the duration. We don't try shrink buckets since pruning
+ * effectively caps catcache expansion in the long term.
+ */
+ if (ct->naccess > 0)
+ ct->naccess--;
+ else
+ {
+ CatCacheRemoveCTup(cp, ct);
+ nremoved++;
+ }
+ }
+ }
+
+ if (nremoved > 0)
+ elog(DEBUG1, "pruning catalog cache id=%d for %s: removed %d / %d",
+ cp->id, cp->cc_relname, nremoved, cp->cc_ntup + nremoved);
+
+ return nremoved > 0;
+}
+
/*
* Enlarge a catcache, doubling the number of buckets.
*/
@@ -1264,6 +1348,12 @@ SearchCatCacheInternal(CatCache *cache,
*/
dlist_move_head(bucket, &ct->cache_elem);
+ /* prolong life of this entry */
+ if (ct->naccess < 2)
+ ct->naccess++;
+
+ ct->lastaccess = catcacheclock;
+
/*
* If it's a positive entry, bump its refcount and return it. If it's
* negative, we can report failure to the caller.
@@ -1888,19 +1978,28 @@ CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, Datum *arguments,
ct->dead = false;
ct->negative = negative;
ct->hash_value = hashValue;
+ ct->naccess = 0;
+ ct->lastaccess = catcacheclock;
dlist_push_head(&cache->cc_bucket[hashIndex], &ct->cache_elem);
cache->cc_ntup++;
CacheHdr->ch_ntup++;
+ /* increase refcount so that the new entry survives pruning */
+ ct->refcount++;
+
/*
- * If the hash table has become too full, enlarge the buckets array. Quite
- * arbitrarily, we enlarge when fill factor > 2.
+ * If the hash table has become too full, try removing infrequently used
+ * entries to make a room for the new entry. If failed, enlarge the bucket
+ * array instead. Quite arbitrarily, we try this when fill factor > 2.
*/
- if (cache->cc_ntup > cache->cc_nbuckets * 2)
+ if (cache->cc_ntup > cache->cc_nbuckets * 2 &&
+ !CatCacheCleanupOldEntries(cache))
RehashCatCache(cache);
+ ct->refcount--;
+
return ct;
}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index aa564d153a..e624c74bf9 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -82,6 +82,7 @@
#include "tsearch/ts_cache.h"
#include "utils/builtins.h"
#include "utils/bytea.h"
+#include "utils/catcache.h"
#include "utils/guc_tables.h"
#include "utils/float.h"
#include "utils/memutils.h"
@@ -2202,6 +2203,17 @@ static struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"catalog_cache_prune_min_age", PGC_USERSET, RESOURCES_MEM,
+ gettext_noop("System catalog cache entries that live unused for longer than this seconds are considered for removal."),
+ gettext_noop("The value of -1 turns off pruning."),
+ GUC_UNIT_S
+ },
+ &catalog_cache_prune_min_age,
+ 300, -1, INT_MAX,
+ NULL, NULL, NULL
+ },
+
/*
* We use the hopefully-safely-small value of 100kB as the compiled-in
* default for max_stack_depth. InitializeGUCOptions will increase it if
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index cccb5f145a..fa117f0573 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -128,6 +128,7 @@
#work_mem = 4MB # min 64kB
#maintenance_work_mem = 64MB # min 1MB
#autovacuum_work_mem = -1 # min 1MB, or -1 to use maintenance_work_mem
+#catalog_cache_prune_min_age = 300s # -1 disables pruning
#max_stack_depth = 2MB # min 100kB
#shared_memory_type = mmap # the default is the first option
# supported by the operating system:
diff --git a/src/include/utils/catcache.h b/src/include/utils/catcache.h
index 65d816a583..2134839ecf 100644
--- a/src/include/utils/catcache.h
+++ b/src/include/utils/catcache.h
@@ -22,6 +22,7 @@
#include "access/htup.h"
#include "access/skey.h"
+#include "datatype/timestamp.h"
#include "lib/ilist.h"
#include "utils/relcache.h"
@@ -119,6 +120,8 @@ typedef struct catctup
bool dead; /* dead but not yet removed? */
bool negative; /* negative cache entry? */
HeapTupleData tuple; /* tuple management header */
+ int naccess; /* # of access to this entry, up to 2 */
+ TimestampTz lastaccess; /* timestamp of the last usage */
/*
* The tuple may also be a member of at most one CatCList. (If a single
@@ -189,6 +192,19 @@ typedef struct catcacheheader
/* this extern duplicates utils/memutils.h... */
extern PGDLLIMPORT MemoryContext CacheMemoryContext;
+/* for guc.c, not PGDLLPMPORT'ed */
+extern int catalog_cache_prune_min_age;
+
+/* source clock for access timestamp of catcache entries */
+extern TimestampTz catcacheclock;
+
+/* SetCatCacheClock - set catcache timestamp source clodk */
+static inline void
+SetCatCacheClock(TimestampTz ts)
+{
+ catcacheclock = ts;
+}
+
extern void CreateCacheMemoryContext(void);
extern CatCache *InitCatCache(int id, Oid reloid, Oid indexoid,
--
2.16.3
----Next_Part(Fri_Mar_29_17_24_40_2019_805)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="FullScan-mod-0001-Remove-entries-that-haven-t-been-used-for-a-certain-.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v37 03/11] Allow to prolong life span of transition tables until transaction end
@ 2019-12-20 01:09 Yugo Nagata <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Yugo Nagata @ 2019-12-20 01:09 UTC (permalink / raw)
Originally, tuplestores of AFTER trigger's transition tables were
freed for each query depth. For our IVM implementation, we would like
to prolong life of the tuplestores because we have to preserve them
for a whole query assuming that some base tables might be changed
in some trigger functions.
---
src/backend/commands/trigger.c | 80 ++++++++++++++++++++++++++++++++--
src/include/commands/trigger.h | 2 +
2 files changed, 78 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index b87b4b40d07..49fe531198a 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3819,6 +3819,10 @@ typedef struct AfterTriggerEventList
* end of the list, so it is relatively easy to discard them. The event
* list chunks themselves are stored in event_cxt.
*
+ * prolonged_tuplestored is a list of transition table tuplestores whose
+ * life are prolonged to the end of the outmost query instead of each nested
+ * query.
+ *
* query_depth is the current depth of nested AfterTriggerBeginQuery calls
* (-1 when the stack is empty).
*
@@ -3884,6 +3888,7 @@ typedef struct AfterTriggersData
SetConstraintState state; /* the active S C state */
AfterTriggerEventList events; /* deferred-event list */
MemoryContext event_cxt; /* memory context for events, if any */
+ List *prolonged_tuplestores; /* list of prolonged tuplestores */
/* per-query-level data: */
AfterTriggersQueryData *query_stack; /* array of structs shown below */
@@ -3932,6 +3937,7 @@ struct AfterTriggersTableData
bool closed; /* true when no longer OK to add tuples */
bool before_trig_done; /* did we already queue BS triggers? */
bool after_trig_done; /* did we already queue AS triggers? */
+ bool prolonged; /* are transition tables prolonged? */
AfterTriggerEventList after_trig_events; /* if so, saved list pointer */
/* "old" transition table for UPDATE/DELETE, if any */
@@ -3978,6 +3984,7 @@ static void TransitionTableAddTuple(EState *estate,
TupleTableSlot *original_insert_tuple,
Tuplestorestate *tuplestore);
static void AfterTriggerFreeQuery(AfterTriggersQueryData *qs);
+static void release_or_prolong_tuplestore(Tuplestorestate *ts, bool prolonged);
static SetConstraintState SetConstraintStateCreate(int numalloc);
static SetConstraintState SetConstraintStateCopy(SetConstraintState origstate);
static SetConstraintState SetConstraintStateAddItem(SetConstraintState state,
@@ -4873,6 +4880,45 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
}
+/*
+ * SetTransitionTablePreserved
+ *
+ * Prolong lifespan of transition tables corresponding specified relid and
+ * command type to the end of the outmost query instead of each nested query.
+ * This enables to use nested AFTER trigger's transition tables from outer
+ * query's triggers. Currently, only immediate incremental view maintenance
+ * uses this.
+ */
+void
+SetTransitionTablePreserved(Oid relid, CmdType cmdType)
+{
+ AfterTriggersTableData *table;
+ AfterTriggersQueryData *qs;
+ bool found = false;
+ ListCell *lc;
+
+ /* Check state, like AfterTriggerSaveEvent. */
+ if (afterTriggers.query_depth < 0)
+ elog(ERROR, "SetTransitionTablePreserved() called outside of query");
+
+ qs = &afterTriggers.query_stack[afterTriggers.query_depth];
+
+ foreach(lc, qs->tables)
+ {
+ table = (AfterTriggersTableData *) lfirst(lc);
+ if (table->relid == relid && table->cmdType == cmdType &&
+ table->closed)
+ {
+ table->prolonged = true;
+ found = true;
+ }
+ }
+
+ if (!found)
+ elog(ERROR,"could not find table with OID %d and command type %d", relid, cmdType);
+}
+
+
/*
* GetAfterTriggersTableData
*
@@ -5113,6 +5159,7 @@ AfterTriggerBeginXact(void)
afterTriggers.firing_depth = 0;
afterTriggers.batch_callbacks = NIL;
afterTriggers.firing_batch_callbacks = false;
+ afterTriggers.prolonged_tuplestores = NIL;
/*
* Verify that there is no leftover state remaining. If these assertions
@@ -5287,11 +5334,11 @@ AfterTriggerFreeQuery(AfterTriggersQueryData *qs)
ts = table->old_tuplestore;
table->old_tuplestore = NULL;
if (ts)
- tuplestore_end(ts);
+ release_or_prolong_tuplestore(ts, table->prolonged);
ts = table->new_tuplestore;
table->new_tuplestore = NULL;
if (ts)
- tuplestore_end(ts);
+ release_or_prolong_tuplestore(ts, table->prolonged);
if (table->storeslot)
{
TupleTableSlot *slot = table->storeslot;
@@ -5309,8 +5356,33 @@ AfterTriggerFreeQuery(AfterTriggersQueryData *qs)
qs->tables = NIL;
list_free_deep(tables);
- list_free_deep(qs->batch_callbacks);
- qs->batch_callbacks = NIL;
+ /* Release prolonged tuplestores at the end of the outmost query */
+ if (afterTriggers.query_depth == 0)
+ {
+ foreach(lc, afterTriggers.prolonged_tuplestores)
+ {
+ ts = (Tuplestorestate *) lfirst(lc);
+ if (ts)
+ tuplestore_end(ts);
+ }
+ afterTriggers.prolonged_tuplestores = NIL;
+ }
+}
+
+/*
+ * Release the tuplestore, or append it to the prolonged tuplestores list.
+ */
+static void
+release_or_prolong_tuplestore(Tuplestorestate *ts, bool prolonged)
+{
+ if (prolonged && afterTriggers.query_depth > 0)
+ {
+ MemoryContext oldcxt = MemoryContextSwitchTo(CurTransactionContext);
+ afterTriggers.prolonged_tuplestores = lappend(afterTriggers.prolonged_tuplestores, ts);
+ MemoryContextSwitchTo(oldcxt);
+ }
+ else
+ tuplestore_end(ts);
}
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 1d9869973c0..e5dd915096d 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -269,6 +269,8 @@ extern void AfterTriggerEndSubXact(bool isCommit);
extern void AfterTriggerSetState(ConstraintsSetStmt *stmt);
extern bool AfterTriggerPendingOnRel(Oid relid);
+extern void SetTransitionTablePreserved(Oid relid, CmdType cmdType);
+
/*
* in utils/adt/ri_triggers.c
--
2.43.0
--Multipart=_Fri__29_May_2026_23_14_17_+0900_Te0o73X2VqYK57Gd
Content-Type: text/x-diff;
name="v37-0002-Add-relisivm-column-to-pg_class-system-catalog.patch"
Content-Disposition: attachment;
filename="v37-0002-Add-relisivm-column-to-pg_class-system-catalog.patch"
Content-Transfer-Encoding: 7bit
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Adding REPACK [concurrently]
@ 2026-02-28 15:16 Mihail Nikalayeu <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Mihail Nikalayeu @ 2026-02-28 15:16 UTC (permalink / raw)
To: Antonin Houska <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Pg Hackers <[email protected]>; Robert Treat <[email protected]>
Hello!
Some review comments:
------------
> attrs = palloc0_array(Datum, desc->natts);
> isnull = palloc0_array(bool, desc->natts);
It looks like there is a memory leak with those arrays.
------------
> # TOAST pointer, wich we need to update
typo
------------
> ident_idx = RelationGetReplicaIndex(rel);
> if (!OidIsValid(ident_idx) && OidIsValid(rel->rd_pkindex))
check_repack_concurrently_requirements uses rd_pkindex as fallback.
But rebuild_relation_finish_concurrent does not contain such logic:
> ident_idx_old = RelationGetReplicaIndex(OldHeap);
------------
> >
> > > ConditionVariablePrepareToSleep(&shared->cv);
> > > for (;;)
> > > {
> > > bool initialized;
> > >
> > > SpinLockAcquire(&shared->mutex);
> > > initialized = shared->initialized;
> > > SpinLockRelease(&shared->mutex);
> > src/backend/commands/cluster.c:3663
> >
> > I think we should check GetBackgroundWorkerPid for worker status, to
> > not wait forever in case of some issue with the worker.
> ConditionVariableSleep() calls CHECK_FOR_INTERRUPTS(). That should process
> error messages from the worker.
Hm, yes, and RepackWorkerShutdown will detach the queue. But
ProcessRepackMessages does not react somehow to SHM_MQ_DETACHED - just
ignores. Or am I missing something?
And looks like it applies to all wait-loops related to repack.
------------
> build_identity_key
> ....
> n = ident_idx->indnatts;
Should we use indnkeyatts here?
------------
> build_identity_key
> ....
> entry->sk_collation = att->attcollation;
Should we use index collation (not heap) here?
entry->sk_collation = ident_idx_rel->rd_indcollation[i];
------------
> SnapBuildInitialSnapshotForRepack
What is about to add defensive checks like SnapBuildInitialSnapshot does?
> if (!builder->committed.includes_all_transactions)
> elog(ERROR, "cannot build an initial slot snapshot, not all transactions are monitored anymore");
Best regards,
Mikhail.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Adding REPACK [concurrently]
@ 2026-03-02 17:23 Antonin Houska <[email protected]>
parent: Mihail Nikalayeu <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Antonin Houska @ 2026-03-02 17:23 UTC (permalink / raw)
To: Mihail Nikalayeu <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Pg Hackers <[email protected]>; Robert Treat <[email protected]>
Antonin Houska <[email protected]> wrote:
> Mihail Nikalayeu <[email protected]> wrote:
> > ------------
> >
> > > attrs = palloc0_array(Datum, desc->natts);
> > > isnull = palloc0_array(bool, desc->natts);
> >
> > It looks like there is a memory leak with those arrays.
>
> I suppose you mean store_change(). Yes, I tried to free the individual chunks
> and forgot these. The next version uses a new, per-change memory context.
I realize now that I forgot to reset the context at the end of the
function. I'll fix that in the next version (which will probably be posted
rather soon).
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Adding REPACK [concurrently]
@ 2026-03-07 14:28 Mihail Nikalayeu <[email protected]>
parent: Antonin Houska <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Mihail Nikalayeu @ 2026-03-07 14:28 UTC (permalink / raw)
To: Antonin Houska <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Pg Hackers <[email protected]>; Robert Treat <[email protected]>
Hello!
On Thu, Mar 5, 2026 at 8:06 PM Antonin Houska <[email protected]> wrote:
> This is it. One more diff added, to engage BulkInsertState.
Few more comments:
> * In case functions in the index need the active snapshot and caller
> * hasn't set one.
Looks like a stale comment.
-----------
> recheck = ExecInsertIndexTuples(iistate->rri,
> iistate->estate,
> 0,
> index_slot,
> NIL, NULL);
Such code in apply_concurrent_update and apply_concurrent_insert.
AFAIU we need to call ResetPerTupleExprContext(iistate->estate); after
list_free(recheck); to avoid small per-row memory leak.
-----------------
In find_target_tuple index_rescan is called before setting the
sk_argument - it works, but feels to be incorrect for the common case.
We should call it once entry->sk_argument is ready.
-----------------
I put possible fixes into the attached patch.
Mikhail.
Attachments:
[application/octet-stream] nocfbot-review_changes.patch (2.4K, ../../CADzfLwVVxC-Ptp98B=gTQgT7uuWY1KnQDLBhGkP1NS3iz-AYcA@mail.gmail.com/2-nocfbot-review_changes.patch)
download | inline diff:
Subject: [PATCH] review changes
---
Index: src/backend/commands/cluster.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
--- a/src/backend/commands/cluster.c (revision 5936ff16dfe4765ef0dd6e18be401aefcf120d36)
+++ b/src/backend/commands/cluster.c (date 1772889987377)
@@ -2971,9 +2971,6 @@
/*
* Update indexes.
- *
- * In case functions in the index need the active snapshot and caller
- * hasn't set one.
*/
ExecStoreHeapTuple(tup, index_slot, false);
recheck = ExecInsertIndexTuples(iistate->rri,
@@ -2988,6 +2985,7 @@
* committed.)
*/
list_free(recheck);
+ ResetPerTupleExprContext(iistate->estate);
pgstat_progress_incr_param(PROGRESS_REPACK_HEAP_TUPLES_INSERTED, 1);
}
@@ -3032,6 +3030,7 @@
index_slot,
NIL, NULL);
list_free(recheck);
+ ResetPerTupleExprContext(iistate->estate);
}
pgstat_progress_incr_param(PROGRESS_REPACK_HEAP_TUPLES_UPDATED, 1);
@@ -3260,25 +3259,24 @@
scan = index_beginscan(rel, ident_index, GetActiveSnapshot(),
NULL, dest->ident_key_nentries, 0);
- /*
- * Scan key is passed by caller, so it does not have to be constructed
- * multiple times. Key entries have all fields initialized, except for
- * sk_argument.
- */
- index_rescan(scan, dest->ident_key, dest->ident_key_nentries, NULL, 0);
-
/* Info needed to retrieve key values from heap tuple. */
ident_form = ident_index->rd_index;
ident_indkey = &ident_form->indkey;
- /* Use the incoming tuple to finalize the scan key. */
- for (int i = 0; i < scan->numberOfKeys; i++)
+ /*
+ * Scan key is passed by caller, so it does not have to be constructed
+ * multiple times. Key entries have all fields initialized, except for
+ * sk_argument.
+ *
+ * Use the incoming tuple to finalize the scan key.
+ */
+ for (int i = 0; i < dest->ident_key_nentries; i++)
{
ScanKey entry;
bool isnull;
int16 attno_heap;
- entry = &scan->keyData[i];
+ entry = &dest->ident_key[i];
attno_heap = ident_indkey->values[i];
entry->sk_argument = heap_getattr(tup_key,
attno_heap,
@@ -3286,6 +3284,8 @@
&isnull);
Assert(!isnull);
}
+
+ index_rescan(scan, dest->ident_key, dest->ident_key_nentries, NULL, 0);
if (index_getnext_slot(scan, ForwardScanDirection, ident_slot))
{
bool shouldFree;
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-03-07 14:28 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-11-04 12:27 Re: [HACKERS] Lazy hash table for XidInMVCCSnapshot (helps Zipfian a bit) Dmitry Dolgov <[email protected]>
2018-11-30 15:05 ` Dmitry Dolgov <[email protected]>
2019-03-01 04:32 [PATCH] Remove entries that haven't been used for a certain time Kyotaro Horiguchi <[email protected]>
2019-12-20 01:09 [PATCH v37 03/11] Allow to prolong life span of transition tables until transaction end Yugo Nagata <[email protected]>
2026-02-28 15:16 Re: Adding REPACK [concurrently] Mihail Nikalayeu <[email protected]>
2026-03-02 17:23 ` Re: Adding REPACK [concurrently] Antonin Houska <[email protected]>
2026-03-07 14:28 ` Re: Adding REPACK [concurrently] Mihail Nikalayeu <[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