public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 4/4] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>)
5+ messages / 4 participants
[nested] [flat]
* [PATCH 4/4] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>)
@ 2020-04-01 01:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Justin Pryzby @ 2020-04-01 01:35 UTC (permalink / raw)
---
doc/src/sgml/ref/cluster.sgml | 12 ++++-
doc/src/sgml/ref/vacuum.sgml | 12 ++++-
src/backend/commands/cluster.c | 56 ++++++++++++++---------
src/backend/commands/matview.c | 3 +-
src/backend/commands/tablecmds.c | 2 +-
src/backend/commands/vacuum.c | 40 ++++++++--------
src/backend/postmaster/autovacuum.c | 1 +
src/include/commands/cluster.h | 4 +-
src/include/commands/vacuum.h | 5 +-
src/test/regress/input/tablespace.source | 13 ++++++
src/test/regress/output/tablespace.source | 20 ++++++++
11 files changed, 117 insertions(+), 51 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 6758ef97a6..9c0f5add59 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -29,6 +29,7 @@ CLUSTER [VERBOSE]
VERBOSE [ <replaceable class="parameter">boolean</replaceable> ]
TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
+ INDEX_TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
</synopsis>
</refsynopsisdiv>
@@ -106,6 +107,15 @@ CLUSTER [VERBOSE]
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>INDEX_TABLESPACE</literal></term>
+ <listitem>
+ <para>
+ Specifies that the table's indexes will be rebuilt on a new tablespace.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>TABLESPACE</literal></term>
<listitem>
@@ -128,7 +138,7 @@ CLUSTER [VERBOSE]
<term><replaceable class="parameter">new_tablespace</replaceable></term>
<listitem>
<para>
- The tablespace where the table will be rebuilt.
+ The tablespace where the table or its indexes will be rebuilt.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index 5261a7c727..28cab119b6 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -36,6 +36,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
TRUNCATE [ <replaceable class="parameter">boolean</replaceable> ]
PARALLEL <replaceable class="parameter">integer</replaceable>
TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
+ INDEX_TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
@@ -265,6 +266,15 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>INDEX_TABLESPACE</literal></term>
+ <listitem>
+ <para>
+ Specifies that the relation's indexes will be rebuilt on a new tablespace.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">boolean</replaceable></term>
<listitem>
@@ -314,7 +324,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
<term><replaceable class="parameter">new_tablespace</replaceable></term>
<listitem>
<para>
- The tablespace where the relation will be rebuilt.
+ The tablespace where the relation or its indexes will be rebuilt.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 626321e3ac..7fc6b69e95 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -71,7 +71,7 @@ typedef struct
static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose,
- Oid NewTableSpaceOid);
+ Oid NewTableSpaceOid, Oid NewIdxTableSpaceOid);
static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool verbose, bool *pSwapToastByContent,
TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
@@ -108,8 +108,9 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
ListCell *lc;
ClusterParams params = {0};
bool verbose = false;
- /* Name of tablespace to use for clustered relation. */
- char *tablespaceName = NULL;
+ /* Names of tablespaces to use for clustered relations. */
+ char *tablespaceName = NULL,
+ *idxtablespaceName = NULL;
/* Parse option list */
foreach(lc, stmt->params)
@@ -120,6 +121,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
verbose = defGetBoolean(opt);
else if (strcmp(opt->defname, "tablespace") == 0)
tablespaceName = defGetString(opt);
+ else if (strcmp(opt->defname, "index_tablespace") == 0)
+ idxtablespaceName = defGetString(opt);
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -130,18 +133,11 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
params.options = (verbose ? CLUOPT_VERBOSE : 0);
- /* Select tablespace Oid to use. */
- if (tablespaceName)
- {
- params.tablespaceOid = get_tablespace_oid(tablespaceName, false);
-
- /* Can't move a non-shared relation into pg_global */
- if (params.tablespaceOid == GLOBALTABLESPACE_OID)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot move non-shared relation to tablespace \"%s\"",
- tablespaceName)));
- }
+ /* Get tablespaces to use. */
+ params.tablespaceOid = tablespaceName ?
+ get_tablespace_oid(tablespaceName, false) : InvalidOid;
+ params.idxtablespaceOid = idxtablespaceName ?
+ get_tablespace_oid(idxtablespaceName, false) : InvalidOid;
if (stmt->relation != NULL)
{
@@ -293,8 +289,9 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
* instead of index order. This is the new implementation of VACUUM FULL,
* and error messages should refer to the operation as VACUUM not CLUSTER.
*
- * "tablespaceOid" is the tablespace where the relation will be rebuilt, or
- * InvalidOid to use its current tablespace.
+ * "tablespaceOid" and "idxtablespaceOid" are the tablespaces where the relation
+ * and its indexes will be rebuilt, or InvalidOid to use their current
+ * tablespaces.
*/
void
cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
@@ -464,7 +461,8 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
TransferPredicateLocksToHeapRelation(OldHeap);
/* rebuild_relation does all the dirty work */
- rebuild_relation(OldHeap, indexOid, verbose, params->tablespaceOid);
+ rebuild_relation(OldHeap, indexOid, verbose, params->tablespaceOid,
+ params->idxtablespaceOid);
/* NB: rebuild_relation does table_close() on OldHeap */
@@ -613,10 +611,11 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
* NB: this routine closes OldHeap at the right time; caller should not.
*/
static void
-rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespaceOid)
+rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespaceOid, Oid NewIdxTablespaceOid)
{
Oid tableOid = RelationGetRelid(OldHeap);
Oid tableSpace = OldHeap->rd_rel->reltablespace;
+ Oid idxtableSpace;
Oid OIDNewHeap;
char relpersistence;
bool is_system_catalog;
@@ -626,7 +625,20 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespace
/* Use new tablespace if passed. */
if (OidIsValid(NewTablespaceOid))
+ {
tableSpace = NewTablespaceOid;
+ /* It's not a shared catalog, so refuse to move it to shared tablespace */
+ if (tableSpace == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move non-shared relation to tablespace \"%s\"",
+ get_tablespace_name(tableSpace))));
+ }
+
+ if (OidIsValid(NewIdxTablespaceOid))
+ idxtableSpace = NewIdxTablespaceOid;
+ else
+ idxtableSpace = get_rel_tablespace(indexOid);
/* Mark the correct index as clustered */
if (OidIsValid(indexOid))
@@ -655,7 +667,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespace
finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog,
swap_toast_by_content, false, true,
frozenXid, cutoffMulti,
- relpersistence);
+ relpersistence, idxtableSpace);
}
@@ -1403,12 +1415,12 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
bool is_internal,
TransactionId frozenXid,
MultiXactId cutoffMulti,
- char newrelpersistence)
+ char newrelpersistence, Oid idxtableSpace)
{
ObjectAddress object;
Oid mapped_tables[4];
int reindex_flags;
- ReindexParams reindex_params = {0};
+ ReindexParams reindex_params = { .tablespaceOid = idxtableSpace };
int i;
/* Report that we are now swapping relation files */
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index c5c25ce11d..ad1bfdc0d2 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -855,7 +855,8 @@ static void
refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence)
{
finish_heap_swap(matviewOid, OIDNewHeap, false, false, true, true,
- RecentXmin, ReadNextMultiXactId(), relpersistence);
+ RecentXmin, ReadNextMultiXactId(), relpersistence,
+ InvalidOid);
}
/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 75b04eb161..eba8478f91 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5077,7 +5077,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
!OidIsValid(tab->newTableSpace),
RecentXmin,
ReadNextMultiXactId(),
- persistence);
+ persistence, InvalidOid);
}
else
{
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 61565c3f14..0eb9786bea 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -109,9 +109,9 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
bool disable_page_skipping = false;
ListCell *lc;
- /* Name and Oid of tablespace to use for relations after VACUUM FULL. */
- char *tablespacename = NULL;
- Oid tablespaceOid = InvalidOid;
+ /* Tablespace to use for relations after VACUUM FULL. */
+ char *tablespacename = NULL,
+ *idxtablespacename = NULL;
/* Set default value */
params.index_cleanup = VACOPT_TERNARY_DEFAULT;
@@ -151,6 +151,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
params.truncate = get_vacopt_ternary_value(opt);
else if (strcmp(opt->defname, "tablespace") == 0)
tablespacename = defGetString(opt);
+ else if (strcmp(opt->defname, "index_tablespace") == 0)
+ idxtablespacename = defGetString(opt);
else if (strcmp(opt->defname, "parallel") == 0)
{
if (opt->arg == NULL)
@@ -211,26 +213,18 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("VACUUM FULL cannot be performed in parallel")));
- /* Get tablespace Oid to use. */
- if (tablespacename)
- {
- if ((params.options & VACOPT_FULL) == 0)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("incompatible TABLESPACE option"),
- errdetail("TABLESPACE can only be used with VACUUM FULL.")));
-
- tablespaceOid = get_tablespace_oid(tablespacename, false);
-
- /* Can't move a non-shared relation into pg_global */
- if (tablespaceOid == GLOBALTABLESPACE_OID)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot move non-shared relation to tablespace \"%s\"",
- tablespacename)));
+ if ((params.options & VACOPT_FULL) == 0 &&
+ (tablespacename || idxtablespacename))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("incompatible TABLESPACE option"),
+ errdetail("TABLESPACE can only be used with VACUUM FULL.")));
- }
- params.tablespace_oid = tablespaceOid;
+ /* Get tablespace Oids to use. */
+ params.tablespace_oid = tablespacename ?
+ get_tablespace_oid(tablespacename, false) : InvalidOid;
+ params.idxtablespace_oid = idxtablespacename ?
+ get_tablespace_oid(idxtablespacename, false) : InvalidOid;
/*
* Make sure VACOPT_ANALYZE is specified if any column lists are present.
@@ -1964,6 +1958,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
{
ClusterParams cluster_params = {
.tablespaceOid = params->tablespace_oid,
+ .idxtablespaceOid = params->idxtablespace_oid,
+ /* Other params initialized to 0/false/NULL */
};
/* close relation before vacuuming, but hold lock until commit */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index a5d98c8c95..fd9eb75e11 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2933,6 +2933,7 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
tab->at_params.is_wraparound = wraparound;
tab->at_params.log_min_duration = log_min_duration;
tab->at_params.tablespace_oid = InvalidOid;
+ tab->at_params.idxtablespace_oid = InvalidOid;
tab->at_vacuum_cost_limit = vac_cost_limit;
tab->at_vacuum_cost_delay = vac_cost_delay;
tab->at_relname = NULL;
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 868dea5ecc..c04a2ce20b 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -28,6 +28,7 @@ typedef struct ClusterParams
{
bits32 options; /* bitmask of CLUOPT_* */
Oid tablespaceOid; /* tablespace to rebuild relation, or InvalidOid */
+ Oid idxtablespaceOid; /* tablespace to rebuild relation's indexes */
} ClusterParams;
extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
@@ -45,6 +46,7 @@ extern void finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
bool is_internal,
TransactionId frozenXid,
MultiXactId minMulti,
- char newrelpersistence);
+ char newrelpersistence,
+ Oid idxtablespaceOid);
#endif /* CLUSTER_H */
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 0934487d4b..6c4e085a45 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -227,8 +227,9 @@ typedef struct VacuumParams
* disabled.
*/
int nworkers;
- Oid tablespace_oid; /* tablespace to use for relations
- * rebuilt by VACUUM FULL */
+ /* tablespaces to use for relations rebuilt by VACUUM FULL */
+ Oid tablespace_oid;
+ Oid idxtablespace_oid;
} VacuumParams;
/* GUC parameters */
diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source
index 2244c1d51d..9a13112605 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -80,6 +80,19 @@ REINDEX (TABLESPACE pg_default) TABLE CONCURRENTLY regress_tblspace_test_tbl; --
SELECT relname FROM pg_class
WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
+-- check CLUSTER with INDEX_TABLESPACE change to non-default location
+CLUSTER (INDEX_TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok
+-- check relations moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+-- check VACUUM with INDEX_TABLESPACE change
+VACUUM (FULL, ANALYSE, FREEZE, INDEX_TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok
+-- check relations moved back to pg_default
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
+
+
-- create a schema we can use
CREATE SCHEMA testschema;
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index 889933a9c8..2f8ecf514d 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -114,6 +114,26 @@ WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspa
---------
(0 rows)
+-- check CLUSTER with INDEX_TABLESPACE change to non-default location
+CLUSTER (INDEX_TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok
+-- check relations moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+ relname
+-------------------------------
+ regress_tblspace_test_tbl_idx
+(1 row)
+
+-- check VACUUM with INDEX_TABLESPACE change
+VACUUM (FULL, ANALYSE, FREEZE, INDEX_TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok
+-- check relations moved back to pg_default
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
+ relname
+---------
+(0 rows)
+
-- create a schema we can use
CREATE SCHEMA testschema;
-- try a table
--
2.17.0
--tsOsTdHNUZQcU9Ye--
^ permalink raw reply [nested|flat] 5+ messages in thread
* a litter question about mdunlinkfiletag function
@ 2024-09-30 02:43 px shi <[email protected]>
2024-10-14 20:59 ` Re: a litter question about mdunlinkfiletag function Bruce Momjian <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: px shi @ 2024-09-30 02:43 UTC (permalink / raw)
To: pgsql-hackers
*Hi, hackers*
*When calculating the path, *forknum* is hardcoded as *MAIN_FORKNUM*:*
/* Compute the path. */
p = relpathperm(ftag->rnode, MAIN_FORKNUM);
*But since the *ftag* structure already contains *forknum*:*
typedef struct FileTag
{
int16 handler; /* SyncRequestHandler value, saving space */
int16 forknum; /* ForkNumber, saving space */
RelFileNode rnode;
uint32 segno;
} FileTag;
*Wouldn’t it be more flexible to use the value from the *ftag* structure
directly?*
*Best regards, *
*Pixian Shi*
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: a litter question about mdunlinkfiletag function
2024-09-30 02:43 a litter question about mdunlinkfiletag function px shi <[email protected]>
@ 2024-10-14 20:59 ` Bruce Momjian <[email protected]>
2024-10-15 02:49 ` Re: a litter question about mdunlinkfiletag function px shi <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Bruce Momjian @ 2024-10-14 20:59 UTC (permalink / raw)
To: px shi <[email protected]>; +Cc: pgsql-hackers
On Mon, Sep 30, 2024 at 10:43:17AM +0800, px shi wrote:
> Hi, hackers
>
> When calculating the path, forknum is hardcoded as MAIN_FORKNUM:
>
> /* Compute the path. */
> p = relpathperm(ftag->rnode, MAIN_FORKNUM);
>
>
> But since the ftag structure already contains forknum:
>
> typedef struct FileTag
> {
> int16 handler; /* SyncRequestHandler value, saving space */
> int16 forknum; /* ForkNumber, saving space */
> RelFileNode rnode;
> uint32 segno;
> } FileTag;
>
>
> Wouldn’t it be more flexible to use the value from the ftag structure directly?
You will find other places where relpathperm() is called without having
a FileTag structure available, e.g. ReorderBufferProcessTXN().
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
When a patient asks the doctor, "Am I going to die?", he means
"Am I going to die soon?"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: a litter question about mdunlinkfiletag function
2024-09-30 02:43 a litter question about mdunlinkfiletag function px shi <[email protected]>
2024-10-14 20:59 ` Re: a litter question about mdunlinkfiletag function Bruce Momjian <[email protected]>
@ 2024-10-15 02:49 ` px shi <[email protected]>
2024-12-21 01:29 ` Re: a litter question about mdunlinkfiletag function Matthias van de Meent <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: px shi @ 2024-10-15 02:49 UTC (permalink / raw)
To: Bruce Momjian <[email protected]>; +Cc: pgsql-hackers
>
> You will find other places where relpathperm() is called without having
> a FileTag structure available, e.g. ReorderBufferProcessTXN().
>
I apologize for the confusion. What I meant to say is that in the
mdunlinkfiletag() function, the forknum is currently hardcoded as
MAIN_FORKNUM when calling relpathperm(). While mdunlinkfiletag currently
only handles MAIN_FORKNUM, wouldn’t it be more flexible to retrieve the
forknum from the ftag structure instead?
Bruce Momjian <[email protected]> 于2024年10月15日周二 04:59写道：
> On Mon, Sep 30, 2024 at 10:43:17AM +0800, px shi wrote:
> > Hi, hackers
> >
> > When calculating the path, forknum is hardcoded as MAIN_FORKNUM:
> >
> > /* Compute the path. */
> > p = relpathperm(ftag->rnode, MAIN_FORKNUM);
> >
> >
> > But since the ftag structure already contains forknum:
> >
> > typedef struct FileTag
> > {
> > int16 handler; /* SyncRequestHandler value, saving space */
> > int16 forknum; /* ForkNumber, saving space */
> > RelFileNode rnode;
> > uint32 segno;
> > } FileTag;
> >
> >
> > Wouldn’t it be more flexible to use the value from the ftag structure
> directly?
>
> You will find other places where relpathperm() is called without having
> a FileTag structure available, e.g. ReorderBufferProcessTXN().
>
> --
> Bruce Momjian <[email protected]> https://momjian.us
> EDB https://enterprisedb.com
>
> When a patient asks the doctor, "Am I going to die?", he means
> "Am I going to die soon?"
>
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: a litter question about mdunlinkfiletag function
2024-09-30 02:43 a litter question about mdunlinkfiletag function px shi <[email protected]>
2024-10-14 20:59 ` Re: a litter question about mdunlinkfiletag function Bruce Momjian <[email protected]>
2024-10-15 02:49 ` Re: a litter question about mdunlinkfiletag function px shi <[email protected]>
@ 2024-12-21 01:29 ` Matthias van de Meent <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Matthias van de Meent @ 2024-12-21 01:29 UTC (permalink / raw)
To: px shi <[email protected]>; +Cc: Bruce Momjian <[email protected]>; pgsql-hackers
On Tue, 15 Oct 2024 at 04:50, px shi <[email protected]> wrote:
>>
>> You will find other places where relpathperm() is called without having
>> a FileTag structure available, e.g. ReorderBufferProcessTXN().
>
>
> I apologize for the confusion. What I meant to say is that in the mdunlinkfiletag() function, the forknum is currently hardcoded as MAIN_FORKNUM when calling relpathperm(). While mdunlinkfiletag currently only handles MAIN_FORKNUM, wouldn’t it be more flexible to retrieve the forknum from the ftag structure instead?
I just noticed this mail thread as I was searching the archives for
other mentions of `mdunlinkfiletag` when doing some more digging on
uses of that function, to back my own bug report of what looks like
the same issue. See [0].
As was explained to me by Thomas, the reason why MAIN_FORKNUM is
hardcoded here (and why ftag.segno is also ignored) is that this code
is only ever reached for FileTag values with forknum=MAIN_FORKNUM (and
segno is also always 0) with the code in Postgres' repository. The
patch proposed in [0] is supposed to make that more clear to
developers.
I suspect the code will be further updated to include the correct fork
number and segment number when there is a need to unlink
non-MAIN_FORKNUM or non-segno=0 files in mdunlinkfiletag.
Kind regards,
Matthias van de Meent
[0] https://www.postgresql.org/message-id/flat/CAEze2WiWt%2B9%2BOnqW1g9rKz0gqxymmt%3Doe6pKAEDrutdfpDMpTw...
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-12-21 01:29 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-04-01 01:35 [PATCH 4/4] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>) Justin Pryzby <[email protected]>
2024-09-30 02:43 a litter question about mdunlinkfiletag function px shi <[email protected]>
2024-10-14 20:59 ` Re: a litter question about mdunlinkfiletag function Bruce Momjian <[email protected]>
2024-10-15 02:49 ` Re: a litter question about mdunlinkfiletag function px shi <[email protected]>
2024-12-21 01:29 ` Re: a litter question about mdunlinkfiletag function Matthias van de Meent <[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