public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v6 2/3] Remove "dead" flag from catcache tuple
3+ messages / 3 participants
[nested] [flat]
* [PATCH v6 2/3] Remove "dead" flag from catcache tuple
@ 2020-11-18 07:57 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-18 07:57 UTC (permalink / raw)
---
src/backend/utils/cache/catcache.c | 43 +++++++++++++-----------------
src/include/utils/catcache.h | 10 -------
2 files changed, 18 insertions(+), 35 deletions(-)
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 644d92dd9a..611b65168d 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -480,6 +480,13 @@ CatCacheRemoveCTup(CatCache *cache, CatCTup *ct)
Assert(ct->refcount == 0);
Assert(ct->my_cache == cache);
+ /* delink from linked list if not yet */
+ if (ct->cache_elem.prev)
+ {
+ dlist_delete(&ct->cache_elem);
+ ct->cache_elem.prev = NULL;
+ }
+
if (ct->c_list)
{
/*
@@ -487,14 +494,10 @@ CatCacheRemoveCTup(CatCache *cache, CatCTup *ct)
* which will recurse back to me, and the recursive call will do the
* work. Set the "dead" flag to make sure it does recurse.
*/
- ct->dead = true;
CatCacheRemoveCList(cache, ct->c_list);
return; /* nothing left to do */
}
- /* delink from linked list */
- dlist_delete(&ct->cache_elem);
-
/*
* Free keys when we're dealing with a negative entry, normal entries just
* point into tuple, allocated together with the CatCTup.
@@ -534,7 +537,7 @@ CatCacheRemoveCList(CatCache *cache, CatCList *cl)
/* if the member is dead and now has no references, remove it */
if (
#ifndef CATCACHE_FORCE_RELEASE
- ct->dead &&
+ ct->cache_elem.prev == NULL &&
#endif
ct->refcount == 0)
CatCacheRemoveCTup(cache, ct);
@@ -609,7 +612,9 @@ CatCacheInvalidate(CatCache *cache, uint32 hashValue)
if (ct->refcount > 0 ||
(ct->c_list && ct->c_list->refcount > 0))
{
- ct->dead = true;
+ dlist_delete(&ct->cache_elem);
+ ct->cache_elem.prev = NULL;
+
/* list, if any, was marked dead above */
Assert(ct->c_list == NULL || ct->c_list->dead);
}
@@ -688,7 +693,8 @@ ResetCatalogCache(CatCache *cache)
if (ct->refcount > 0 ||
(ct->c_list && ct->c_list->refcount > 0))
{
- ct->dead = true;
+ dlist_delete(&ct->cache_elem);
+ ct->cache_elem.prev = NULL;
/* list, if any, was marked dead above */
Assert(ct->c_list == NULL || ct->c_list->dead);
}
@@ -1268,9 +1274,6 @@ SearchCatCacheInternal(CatCache *cache,
{
ct = dlist_container(CatCTup, cache_elem, iter.cur);
- if (ct->dead)
- continue; /* ignore dead entries */
-
if (ct->hash_value != hashValue)
continue; /* quickly skip entry if wrong hash val */
@@ -1522,7 +1525,6 @@ ReleaseCatCache(HeapTuple tuple)
offsetof(CatCTup, tuple));
/* Safety checks to ensure we were handed a cache entry */
- Assert(ct->ct_magic == CT_MAGIC);
Assert(ct->refcount > 0);
ct->refcount--;
@@ -1530,7 +1532,7 @@ ReleaseCatCache(HeapTuple tuple)
if (
#ifndef CATCACHE_FORCE_RELEASE
- ct->dead &&
+ ct->cache_elem.prev == NULL &&
#endif
ct->refcount == 0 &&
(ct->c_list == NULL || ct->c_list->refcount == 0))
@@ -1737,8 +1739,8 @@ SearchCatCacheList(CatCache *cache,
{
ct = dlist_container(CatCTup, cache_elem, iter.cur);
- if (ct->dead || ct->negative)
- continue; /* ignore dead and negative entries */
+ if (ct->negative)
+ continue; /* ignore negative entries */
if (ct->hash_value != hashValue)
continue; /* quickly skip entry if wrong hash val */
@@ -1799,14 +1801,13 @@ SearchCatCacheList(CatCache *cache,
{
foreach(ctlist_item, ctlist)
{
+ Assert (ct->cache_elem.prev != NULL);
+
ct = (CatCTup *) lfirst(ctlist_item);
Assert(ct->c_list == NULL);
Assert(ct->refcount > 0);
ct->refcount--;
if (
-#ifndef CATCACHE_FORCE_RELEASE
- ct->dead &&
-#endif
ct->refcount == 0 &&
(ct->c_list == NULL || ct->c_list->refcount == 0))
CatCacheRemoveCTup(cache, ct);
@@ -1834,9 +1835,6 @@ SearchCatCacheList(CatCache *cache,
/* release the temporary refcount on the member */
Assert(ct->refcount > 0);
ct->refcount--;
- /* mark list dead if any members already dead */
- if (ct->dead)
- cl->dead = true;
}
Assert(i == nmembers);
@@ -1960,11 +1958,9 @@ CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, Datum *arguments,
* Finish initializing the CatCTup header, and add it to the cache's
* linked list and counts.
*/
- ct->ct_magic = CT_MAGIC;
ct->my_cache = cache;
ct->c_list = NULL;
ct->refcount = 0; /* for the moment */
- ct->dead = false;
ct->negative = negative;
ct->hash_value = hashValue;
ct->lastaccess = catcacheclock;
@@ -2158,9 +2154,6 @@ PrintCatCacheLeakWarning(HeapTuple tuple)
CatCTup *ct = (CatCTup *) (((char *) tuple) -
offsetof(CatCTup, tuple));
- /* Safety check to ensure we were handed a cache entry */
- Assert(ct->ct_magic == CT_MAGIC);
-
elog(WARNING, "cache reference leak: cache %s (%d), tuple %u/%u has count %d",
ct->my_cache->cc_relname, ct->my_cache->id,
ItemPointerGetBlockNumber(&(tuple->t_self)),
diff --git a/src/include/utils/catcache.h b/src/include/utils/catcache.h
index 291e857e38..53b0bf31eb 100644
--- a/src/include/utils/catcache.h
+++ b/src/include/utils/catcache.h
@@ -87,9 +87,6 @@ typedef struct catcache
typedef struct catctup
{
- int ct_magic; /* for identifying CatCTup entries */
-#define CT_MAGIC 0x57261502
-
uint32 hash_value; /* hash value for this tuple's keys */
/*
@@ -106,19 +103,12 @@ typedef struct catctup
dlist_node cache_elem; /* list member of per-bucket list */
/*
- * A tuple marked "dead" must not be returned by subsequent searches.
- * However, it won't be physically deleted from the cache until its
- * refcount goes to zero. (If it's a member of a CatCList, the list's
- * refcount must go to zero, too; also, remember to mark the list dead at
- * the same time the tuple is marked.)
- *
* A negative cache entry is an assertion that there is no tuple matching
* a particular key. This is just as useful as a normal entry so far as
* avoiding catalog searches is concerned. Management of positive and
* negative entries is identical.
*/
int refcount; /* number of active references */
- bool dead; /* dead but not yet removed? */
bool negative; /* negative cache entry? */
HeapTupleData tuple; /* tuple management header */
uint64 lastaccess; /* timestamp in us of the last usage */
--
2.27.0
----Next_Part(Thu_Jan_14_17_32_27_2021_995)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="v6-0003-catcachebench.patch"
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: pg_stop_backup() v2 incorrectly marked as proretset
@ 2022-03-02 10:04 Michael Paquier <[email protected]>
2022-03-02 10:25 ` Re: pg_stop_backup() v2 incorrectly marked as proretset Aleksander Alekseev <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Michael Paquier @ 2022-03-02 10:04 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]
On Wed, Mar 02, 2022 at 05:22:35PM +0900, Kyotaro Horiguchi wrote:
> But the patch forgets to remove an useless variable.
Indeed. I forgot to look at stderr.
>> /* Initialise attributes information in the tuple descriptor */
>> tupdesc = CreateTemplateTupleDesc(PG_STOP_BACKUP_V2_COLS);
>> TupleDescInitEntry(tupdesc, (AttrNumber) 1, "lsn",
>> PG_LSNOID, -1, 0);
>
> I think we can use get_call_resuilt_type here.
Yes, I don't mind doing so here.
--
Michael
Attachments:
[text/x-diff] stopbackup-no-srf-2.patch (3.8K, ../../[email protected]/2-stopbackup-no-srf-2.patch)
download | inline diff:
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index bf88858171..d8e8715ed1 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6275,9 +6275,9 @@
proname => 'pg_stop_backup', provolatile => 'v', proparallel => 'r',
prorettype => 'pg_lsn', proargtypes => '', prosrc => 'pg_stop_backup' },
{ oid => '2739', descr => 'finish taking an online backup',
- proname => 'pg_stop_backup', prorows => '1', proretset => 't',
- provolatile => 'v', proparallel => 'r', prorettype => 'record',
- proargtypes => 'bool bool', proallargtypes => '{bool,bool,pg_lsn,text,text}',
+ proname => 'pg_stop_backup', provolatile => 'v', proparallel => 'r',
+ prorettype => 'record', proargtypes => 'bool bool',
+ proallargtypes => '{bool,bool,pg_lsn,text,text}',
proargmodes => '{i,i,o,o,o}',
proargnames => '{exclusive,wait_for_archive,lsn,labelfile,spcmapfile}',
prosrc => 'pg_stop_backup_v2' },
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 12e2bf4135..9f7a282ed2 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -165,43 +165,20 @@ pg_stop_backup(PG_FUNCTION_ARGS)
Datum
pg_stop_backup_v2(PG_FUNCTION_ARGS)
{
- ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+#define PG_STOP_BACKUP_V2_COLS 3
TupleDesc tupdesc;
- Tuplestorestate *tupstore;
- MemoryContext per_query_ctx;
- MemoryContext oldcontext;
- Datum values[3];
- bool nulls[3];
+ Datum values[PG_STOP_BACKUP_V2_COLS];
+ bool nulls[PG_STOP_BACKUP_V2_COLS];
bool exclusive = PG_GETARG_BOOL(0);
bool waitforarchive = PG_GETARG_BOOL(1);
XLogRecPtr stoppoint;
SessionBackupState status = get_backup_status();
- /* check to see if caller supports us returning a tuplestore */
- if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("set-valued function called in context that cannot accept a set")));
- if (!(rsinfo->allowedModes & SFRM_Materialize))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("materialize mode required, but it is not allowed in this context")));
-
- /* Build a tuple descriptor for our result type */
+ /* Initialise attributes information in the tuple descriptor */
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
- oldcontext = MemoryContextSwitchTo(per_query_ctx);
-
- tupstore = tuplestore_begin_heap(true, false, work_mem);
- rsinfo->returnMode = SFRM_Materialize;
- rsinfo->setResult = tupstore;
- rsinfo->setDesc = tupdesc;
-
- MemoryContextSwitchTo(oldcontext);
-
MemSet(values, 0, sizeof(values));
MemSet(nulls, 0, sizeof(nulls));
@@ -251,9 +228,8 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
/* Stoppoint is included on both exclusive and nonexclusive backups */
values[0] = LSNGetDatum(stoppoint);
- tuplestore_putvalues(tupstore, tupdesc, values, nulls);
-
- return (Datum) 0;
+ /* Returns the record as Datum */
+ PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)));
}
/*
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index 758ab6e25a..81bac6f581 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -384,7 +384,7 @@ CREATE OR REPLACE FUNCTION
CREATE OR REPLACE FUNCTION pg_stop_backup (
exclusive boolean, wait_for_archive boolean DEFAULT true,
OUT lsn pg_lsn, OUT labelfile text, OUT spcmapfile text)
- RETURNS SETOF record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup_v2'
+ RETURNS record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup_v2'
PARALLEL RESTRICTED;
CREATE OR REPLACE FUNCTION
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: pg_stop_backup() v2 incorrectly marked as proretset
2022-03-02 10:04 Re: pg_stop_backup() v2 incorrectly marked as proretset Michael Paquier <[email protected]>
@ 2022-03-02 10:25 ` Aleksander Alekseev <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Aleksander Alekseev @ 2022-03-02 10:25 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi Michael,
```
Datum
pg_stop_backup_v2(PG_FUNCTION_ARGS)
{
- ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+#define PG_STOP_BACKUP_V2_COLS 3
TupleDesc tupdesc;
- Tuplestorestate *tupstore;
- MemoryContext per_query_ctx;
- MemoryContext oldcontext;
- Datum values[3];
- bool nulls[3];
+ Datum values[PG_STOP_BACKUP_V2_COLS];
+ bool nulls[PG_STOP_BACKUP_V2_COLS];
```
Declaring a macro inside the procedure body is a bit unconventional.
Since it doesn't seem to be used for anything except these two array
declarations I suggest keeping simply "3" here.
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2022-03-02 10:25 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-18 07:57 [PATCH v6 2/3] Remove "dead" flag from catcache tuple Kyotaro Horiguchi <[email protected]>
2022-03-02 10:04 Re: pg_stop_backup() v2 incorrectly marked as proretset Michael Paquier <[email protected]>
2022-03-02 10:25 ` Re: pg_stop_backup() v2 incorrectly marked as proretset Aleksander Alekseev <[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