Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Sun, Feb 14, 2021 at 08:10:50PM -0600, Justin Pryzby wrote: > Isn't this dead code ? Nope, it's not dead. Those two code paths can be hit when attempting a reidex with a tablespace move directly on toast tables and indexes, see: =# create table aa (a text); CREATE TABLE =# select relname from pg_class where oid > 16000; relname -- aa pg_toast_16385 pg_toast_16385_index (3 rows) =# reindex (concurrently, tablespace pg_default) table pg_toast.pg_toast_16385; ERROR: 0A000: cannot move system relation "pg_toast_16385" LOCATION: ReindexRelationConcurrently, indexcmds.c:3295 =# reindex (concurrently, tablespace pg_default) index pg_toast.pg_toast_16385_index; ERROR: 0A000: cannot move system relation "pg_toast_16385_index" LOCATION: ReindexRelationConcurrently, indexcmds.c:3439 It is easy to save the relation name using \gset in a regression test, but we had better keep a reference to the relation name in the error message so this would not be really portable. Using a PL function to do that with a CATCH block would not work either as CONCURRENTLY cannot be run in a transaction block. This leaves 090_reindexdb.pl, but I was not really convinced that this was worth the extra test cycles (I am aware of the --tablespace option missing in reindexdb, someone I know was trying to get that done for the next CF). -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Thu, Feb 04, 2021 at 03:38:39PM +0900, Michael Paquier wrote:
> On Wed, Feb 03, 2021 at 07:54:42PM +0900, Michael Paquier wrote:
> > Not sure I like that. Here is a proposal:
> > "it is recommended to separately use ALTER TABLE ONLY on them so as
> > any new partitions attached inherit the new tablespace value."
>
> So, I have done more work on this stuff today, and applied that as of
> c5b2860.
> A second thing I have come back to is allow_system_table_mods for
> toast relations, and decided to just forbid TABLESPACE if attempting
> to use it directly on a system table even if allow_system_table_mods
> is true. This was leading to inconsistent behaviors and weirdness in
> the concurrent case because all the indexes are processed in series
> after building a list. As we want to ignore the move of toast indexes
> when moving the indexes of the parent table, this was leading to extra
> conditions that are not really worth supporting after thinking about
> it. One other issue was the lack of consistency when using pg_global
> that was a no-op for the concurrent case but failed in the
> non-concurrent case. I have put in place more regression tests for
> all that.
Isn't this dead code ?
postgres=# REINDEX (CONCURRENTLY, TABLESPACE pg_global) TABLE pg_class;
ERROR: 0A000: cannot reindex system catalogs concurrently
LOCATION: ReindexRelationConcurrently, indexcmds.c:3276
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 127ba7835d..c77a9b2563 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3260,73 +3260,66 @@ ReindexRelationConcurrently(Oid relationOid,
ReindexParams *params)
{
if (IsCatalogRelationOid(relationOid))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot reindex
system catalogs concurrently")));
...
- if (OidIsValid(params->tablespaceOid) &&
- IsSystemRelation(heapRelation))
- ereport(ERROR,
-
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-errmsg("cannot move
system relation \"%s\"",
-
RelationGetRelationName(heapRelation;
-
@@ -3404,73 +3397,66 @@ ReindexRelationConcurrently(Oid relationOid,
ReindexParams *params)
if (IsCatalogRelationOid(heapId))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot reindex
system catalogs concurrently")));
...
- if (OidIsValid(params->tablespaceOid) &&
- IsSystemRelation(heapRelation))
- ereport(ERROR,
-
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-errmsg("cannot move
system relation \"%s\"",
-
get_rel_name(relationOid;
-
>From b4347c18bc732d30295c065ef71edaac65e68fe6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sun, 14 Feb 2021 19:49:52 -0600
Subject: [PATCH] Dead code: REINDEX (CONCURRENTLY, TABLESPACE ..):
c5b286047cd698021e57a527215b48865fd4ad4e
---
src/backend/commands/indexcmds.c | 14 --
1 file changed, 14 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 127ba7835d..c77a9b2563 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3260,73 +3260,66 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
{
/*
* In the case of a relation, find all its indexes including
* toast indexes.
*/
Relation heapRelation;
/* Save the list of relation OIDs in private context */
oldcontext = MemoryContextSwitchTo(private_context);
/* Track this relation for session locks */
heapRelationIds = lappend_oid(heapRelationIds, relationOid);
MemoryContextSwitchTo(oldcontext);
if (IsCatalogRelationOid(relationOid))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot reindex system catalogs concurrently")));
/* Open relation to get its indexes */
if ((params->options & REINDEXOPT_MISSING_OK) != 0)
{
heapRelation = try_table_open(relationOid,
ShareUpdateExclusiveLock);
/* leave if relation does not exist */
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Feb 03, 2021 at 07:54:42PM +0900, Michael Paquier wrote: > Not sure I like that. Here is a proposal: > "it is recommended to separately use ALTER TABLE ONLY on them so as > any new partitions attached inherit the new tablespace value." So, I have done more work on this stuff today, and applied that as of c5b2860. While reviewing my changes, I have noticed that I have managed to break ALTER TABLE SET TABLESPACE which would have failed when cascading to a toast relation, the extra check placed previously in CheckRelationTableSpaceMove() being incorrect. The most surprising part was that we had zero in-core tests to catch this mistake, so I have added an extra test to cover this scenario while on it. A second thing I have come back to is allow_system_table_mods for toast relations, and decided to just forbid TABLESPACE if attempting to use it directly on a system table even if allow_system_table_mods is true. This was leading to inconsistent behaviors and weirdness in the concurrent case because all the indexes are processed in series after building a list. As we want to ignore the move of toast indexes when moving the indexes of the parent table, this was leading to extra conditions that are not really worth supporting after thinking about it. One other issue was the lack of consistency when using pg_global that was a no-op for the concurrent case but failed in the non-concurrent case. I have put in place more regression tests for all that. Regarding the VACUUM and CLUSTER cases, I am not completely sure if going through these for a tablespace case is the best move we can do, as ALTER TABLE is able to mix multiple operations and all of them require already an AEL to work. REINDEX was different thanks to the case of CONCURRENTLY. Anyway, as a lot of work has been done here already, I would recommend to create new threads about those two topics. I am also closing this patch in the CF app. -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Feb 03, 2021 at 01:35:26PM +0300, Alexey Kondratov wrote:
> This check for OidIsValid() seems to be excessive, since you moved the whole
> ACL check under 'if (tablespacename != NULL)' here.
That's more consistent with ATPrepSetTableSpace().
> +SELECT relid, parentrelid, level FROM
> pg_partition_tree('tbspace_reindex_part_index')
> + ORDER BY relid, level;
> +SELECT relid, parentrelid, level FROM
> pg_partition_tree('tbspace_reindex_part_index')
> + ORDER BY relid, level;
>
> Why do you do the same twice in a row? It looks like a typo. Maybe it was
> intended to be called for partitioned table AND index.
Yes, my intention was to show the tree of the set of tables. It is
not really interesting for this test anyway, so let's just remove this
extra query.
--
Michael
signature.asc
Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Feb 03, 2021 at 12:53:42AM -0600, Justin Pryzby wrote:
> On Wed, Feb 03, 2021 at 03:37:39PM +0900, Michael Paquier wrote:
>> index 627b36300c..4ee3951ca0 100644
>> --- a/doc/src/sgml/ref/reindex.sgml
>> +++ b/doc/src/sgml/ref/reindex.sgml
>> @@ -293,8 +311,30 @@ REINDEX [ ( > class="parameter">option [, ...] ) ] { IN
>> respectively. Each partition of the specified partitioned relation is
>> reindexed in a separate transaction. Those commands cannot be used inside
>> a transaction block when working on a partitioned table or index.
>> + If a REINDEX command fails when run on a partitioned
>> + relation, and TABLESPACE was specified, then it may
>> not
>> + have moved all indexes to the new tablespace. Re-running the command
>> + will rebuild again all the partitions and move previously-unprocessed
>
> remove "again"
Okay.
>> + indexes to the new tablespace.
>> +
>> +
>> +
>> + When using the TABLESPACE clause with
>> + REINDEX on a partitioned index or table, only the
>> + tablespace references of the partitions are updated. As partitioned
>> indexes
>
> I think you should say "of the LEAF partitions ..". The intermediate,
> partitioned tables are also "partitions" (partitioned partitions if you like).
Indeed, I can see how that's confusing.
>> + are not updated, it is recommended to separately use
>> + ALTER TABLE ONLY on them to achieve that.
>
> Maybe say: "..to set the default tablespace of any new partitions created in
> the future".
Not sure I like that. Here is a proposal:
"it is recommended to separately use ALTER TABLE ONLY on them so as
any new partitions attached inherit the new tablespace value."
--
Michael
signature.asc
Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-02-03 09:37, Michael Paquier wrote:
On Tue, Feb 02, 2021 at 10:32:19AM +0900, Michael Paquier wrote:
On Mon, Feb 01, 2021 at 06:28:57PM +0300, Alexey Kondratov wrote:
> Hm, IIUC, REINDEX CONCURRENTLY doesn't use either of them. It directly uses
> index_create() with a proper tablespaceOid instead of
> SetRelationTableSpace(). And its checks structure is more restrictive even
> without tablespace change, so it doesn't use CheckRelationTableSpaceMove().
Sure. I have not checked the patch in details, but even with that it
would be much safer to me if we apply the same sanity checks
everywhere. That's less potential holes to worry about.
Thanks Alexey for the new patch. I have been looking at the main
patch in details.
/*
-* Don't allow reindex on temp tables of other backends ... their
local
-* buffer manager is not going to cope.
+* We don't support moving system relations into different
tablespaces
+* unless allow_system_table_mods=1.
*/
If you remove the check on RELATION_IS_OTHER_TEMP() in
reindex_index(), you would allow the reindex of a temp relation owned
by a different session if its tablespace is not changed, so this
cannot be removed.
+!allowSystemTableMods && IsSystemRelation(iRel))
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot reindex temporary tables of other
sessions")));
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a system
catalog",
+RelationGetRelationName(iRel;
Indeed, a system relation with a relfilenode should be allowed to move
under allow_system_table_mods. I think that we had better move this
check into CheckRelationTableSpaceMove() instead of reindex_index() to
centralize the logic. ALTER TABLE does this business in
RangeVarCallbackForAlterRelation(), but our code path opening the
relation is different for the non-concurrent case.
+ if (OidIsValid(params->tablespaceOid) &&
+ IsSystemClass(relid, classtuple))
+ {
+ if (!allowSystemTableMods)
+ {
+ /* Skip all system relations, if not
allowSystemTableMods *
I don't see the need for having two warnings here to say the same
thing if a relation is mapped or not mapped, so let's keep it simple.
Yeah, I just wanted to separate mapped and system relations, but
probably it is too complicated.
I have found that the test suite was rather messy in its
organization. Table creations were done first with a set of tests not
really ordered, so that was really hard to follow. This has also led
to a set of tests that were duplicated, while other tests have been
missed, mainly some cross checks for the concurrent and non-concurrent
behaviors. I have reordered the whole so as tests on catalogs, normal
tables and partitions are done separately with relations created and
dropped for each set. Partitions use a global check for tablespaces
and relfilenodes after one concurrent reindex (didn't see the point in
doubling with the non-concurrent case as the same code path to select
the relations from the partition tree is taken). An ACL test has been
added at the end.
The case of partitioned indexes was kind of interesting and I thought
about that a couple of days, and I took the decision to ignore
relations that have no storage as you did, documenting that ALTER
TABLE can be used to update the references of the partitioned
relations. The command is still useful with this behavior, and the
tests I have added track that.
Finally, I have reworked the docs, separating the limitations related
to system catalogs and partitioned relations, to be more consistent
with the notes at the end of the page.
Thanks for working on this.
+ if (tablespacename != NULL)
+ {
+ params.tablespaceOid = get_tablespace_oid(tablespacename,
false);
+
+ /* Check permissions except when moving to database's default */
+ if (OidIsValid(params.tablespaceOid) &&
This check for OidIsValid() seems to be excessive, since you moved the
whole ACL check under 'if (tablespacename != NULL)' here.
+ params.tablespaceOid != MyDatabaseTableSpace)
+ {
+ AclResult aclresult;
+CREATE INDEX regress_tblspace_test_tbl_idx ON regress_tblspace_test_tbl
(num1);
+-- move to global tablespace move fails
Maybe 'move to global tablespace, fail', just to match a style of the
previous comments.
+REINDEX (TABLESPACE pg_global) INDEX regress_tblspace_test_tbl_idx;
+SELECT relid, parentrelid, level FROM
pg_partition_tree('tbspace_reindex_part_index')
+ ORDER BY relid, level;
+SELECT relid, parentrelid, level FROM
pg_partition_tree('tbspace_reindex_part_index')
+ ORDER BY relid, level;
Why do you do the same twice in a row? It looks like a typo. Maybe it
was intended to be called for
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Feb 03, 2021 at 03:37:39PM +0900, Michael Paquier wrote:
> index 627b36300c..4ee3951ca0 100644
> --- a/doc/src/sgml/ref/reindex.sgml
> +++ b/doc/src/sgml/ref/reindex.sgml
> @@ -293,8 +311,30 @@ REINDEX [ ( class="parameter">option [, ...] ) ] { IN
> respectively. Each partition of the specified partitioned relation is
> reindexed in a separate transaction. Those commands cannot be used inside
> a transaction block when working on a partitioned table or index.
> + If a REINDEX command fails when run on a partitioned
> + relation, and TABLESPACE was specified, then it may not
> + have moved all indexes to the new tablespace. Re-running the command
> + will rebuild again all the partitions and move previously-unprocessed
remove "again"
> + indexes to the new tablespace.
> +
> +
> +
> + When using the TABLESPACE clause with
> + REINDEX on a partitioned index or table, only the
> + tablespace references of the partitions are updated. As partitioned
> indexes
I think you should say "of the LEAF partitions ..". The intermediate,
partitioned tables are also "partitions" (partitioned partitions if you like).
> + are not updated, it is recommended to separately use
> + ALTER TABLE ONLY on them to achieve that.
Maybe say: "..to set the default tablespace of any new partitions created in
the future".
--
Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Feb 02, 2021 at 10:32:19AM +0900, Michael Paquier wrote:
> On Mon, Feb 01, 2021 at 06:28:57PM +0300, Alexey Kondratov wrote:
> > Hm, IIUC, REINDEX CONCURRENTLY doesn't use either of them. It directly uses
> > index_create() with a proper tablespaceOid instead of
> > SetRelationTableSpace(). And its checks structure is more restrictive even
> > without tablespace change, so it doesn't use CheckRelationTableSpaceMove().
>
> Sure. I have not checked the patch in details, but even with that it
> would be much safer to me if we apply the same sanity checks
> everywhere. That's less potential holes to worry about.
Thanks Alexey for the new patch. I have been looking at the main
patch in details.
/*
-* Don't allow reindex on temp tables of other backends ... their local
-* buffer manager is not going to cope.
+* We don't support moving system relations into different tablespaces
+* unless allow_system_table_mods=1.
*/
If you remove the check on RELATION_IS_OTHER_TEMP() in
reindex_index(), you would allow the reindex of a temp relation owned
by a different session if its tablespace is not changed, so this
cannot be removed.
+!allowSystemTableMods && IsSystemRelation(iRel))
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot reindex temporary tables of other sessions")));
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a system catalog",
+RelationGetRelationName(iRel;
Indeed, a system relation with a relfilenode should be allowed to move
under allow_system_table_mods. I think that we had better move this
check into CheckRelationTableSpaceMove() instead of reindex_index() to
centralize the logic. ALTER TABLE does this business in
RangeVarCallbackForAlterRelation(), but our code path opening the
relation is different for the non-concurrent case.
+ if (OidIsValid(params->tablespaceOid) &&
+ IsSystemClass(relid, classtuple))
+ {
+ if (!allowSystemTableMods)
+ {
+ /* Skip all system relations, if not allowSystemTableMods *
I don't see the need for having two warnings here to say the same
thing if a relation is mapped or not mapped, so let's keep it simple.
+REINDEX (TABLESPACE regress_tblspace) SYSTEM CONCURRENTLY postgres; -- fail
+ERROR: cannot reindex system catalogs concurrently
[...]
+REINDEX (TABLESPACE regress_tblspace) DATABASE regression; -- ok with warning
+WARNING: cannot change tablespace of indexes on system relations, skipping all
+REINDEX (TABLESPACE pg_default) DATABASE regression; -- ok with warning
+WARNING: cannot change tablespace of indexes on system relations, skipping all
Those tests are costly by design, so let's drop them. They have been
useful to check the patch, but if tests are changed with objects
remaining around this would cost a lot of resources.
+ /* It's not a shared catalog, so refuse to move it to shared tablespace */
+ if (params->tablespaceOid == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("cannot move non-shared relation totablespace \"%s\"",
+get_tablespace_name(params->tablespaceOid;
There is no test coverage for this case with REINDEX CONCURRENTLY, and
that's easy enough to stress. So I have added one.
I have found that the test suite was rather messy in its
organization. Table creations were done first with a set of tests not
really ordered, so that was really hard to follow. This has also led
to a set of tests that were duplicated, while other tests have been
missed, mainly some cross checks for the concurrent and non-concurrent
behaviors. I have reordered the whole so as tests on catalogs, normal
tables and partitions are done separately with relations created and
dropped for each set. Partitions use a global check for tablespaces
and relfilenodes after one concurrent reindex (didn't see the point in
doubling with the non-concurrent case as the same code path to select
the relations from the partition tree is taken). An ACL test has been
added at the end.
The case of partitioned indexes was kind of interesting and I thought
about that a couple of days, and I took the decision to ignore
relations that have no storage as you did, documenting that ALTER
TABLE can be used to update the references of the partitioned
relations. The command is still useful with this behavior, and the
tests I have added track that.
Finally, I have reworked the docs, separating the limitations related
to system catalogs and partitioned relations, to be more consistent
with the notes at the end of the page.
--
Michael
From c021aeca905a738d38acb257cbc4724804273499 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Wed, 3 Feb 2021 15:26:21 +0900
Subject: [PATCH v11] Allow REINDEX to change tablespace
REINDEX already
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Mon, Feb 01, 2021 at 06:28:57PM +0300, Alexey Kondratov wrote: > Hm, IIUC, REINDEX CONCURRENTLY doesn't use either of them. It directly uses > index_create() with a proper tablespaceOid instead of > SetRelationTableSpace(). And its checks structure is more restrictive even > without tablespace change, so it doesn't use CheckRelationTableSpaceMove(). Sure. I have not checked the patch in details, but even with that it would be much safer to me if we apply the same sanity checks everywhere. That's less potential holes to worry about. > Basically, it implements this option "we could silently *not* do the switch > for partitioned indexes in the flow of REINDEX, letting users handle that > with an extra ALTER TABLE SET TABLESPACE once REINDEX has finished". It > probably makes sense, since we are doing tablespace change altogether with > index relation rewrite and don't touch relations without storage. Doing > ALTER INDEX ... SET TABLESPACE will be almost cost-less on them, since they > do not hold any data. Yeah, they'd still need an AEL for a short time on the partitioned bits with what's on HEAD. I'll keep in mind to look at the possibility to downgrade this lock if cascading only on partitioned tables. The main take is that AlterTableGetLockLevel() cannot select a lock type based on the table meta-data. Tricky problem it is if taken as a whole, but I guess that we should be able to tweak ALTER TABLE ONLY on a partitioned table/index pretty easily (inh = false). -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Mon, Feb 01, 2021 at 06:28:57PM +0300, Alexey Kondratov wrote: > On 2021-01-30 05:23, Michael Paquier wrote: > > This makes me really wonder if we would not be better to restrict this > > operation for partitioned relation as part of REINDEX as a first step. > > Another thing, mentioned upthread, is that we could do this part of > > the switch at the last transaction, or we could silently *not* do the > > switch for partitioned indexes in the flow of REINDEX, letting users > > handle that with an extra ALTER TABLE SET TABLESPACE once REINDEX has > > finished on all the partitions, cascading the command only on the > > partitioned relation of a tree. I suggest that it'd be un-intuitive to skip partitioned rels , silently requiring a user to also run "ALTER .. SET TABLESPACE". But I think it'd be okay if REINDEX(TABLESPACE) didn't support partitioned tables/indexes at first. I think it'd be better as an ERROR. -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-01-30 05:23, Michael Paquier wrote:
On Fri, Jan 29, 2021 at 08:56:47PM +0300, Alexey Kondratov wrote:
On 2021-01-28 14:42, Alexey Kondratov wrote:
No, you are right, we are doing REINDEX with AccessExclusiveLock as
it
was before. This part is a more specific one. It only applies to
partitioned indexes, which do not hold any data, so we do not reindex
them directly, only their leafs. However, if we are doing a
TABLESPACE
change, we have to record it in their pg_class entry, so all future
leaf partitions were created in the proper tablespace.
That way, we open partitioned index relation only for a reference,
i.e. read-only, but modify pg_class entry under a proper lock
(RowExclusiveLock). That's why I thought that ShareLock will be
enough.
IIUC, 'ALTER TABLE ... SET TABLESPACE' uses AccessExclusiveLock even
for relations with no storage, since AlterTableGetLockLevel() chooses
it if AT_SetTableSpace is met. This is very similar to our case, so
probably we should do the same?
Actually it is not completely clear for me why
ShareUpdateExclusiveLock is sufficient for newly added
SetRelationTableSpace() as Michael wrote in the comment.
Nay, it was not fine. That's something Alvaro has mentioned, leading
to 2484329. This also means that the main patch of this thread should
refresh the comments at the top of CheckRelationTableSpaceMove() and
SetRelationTableSpace() to mention that this is used by REINDEX
CONCURRENTLY with a lower lock.
Hm, IIUC, REINDEX CONCURRENTLY doesn't use either of them. It directly
uses index_create() with a proper tablespaceOid instead of
SetRelationTableSpace(). And its checks structure is more restrictive
even without tablespace change, so it doesn't use
CheckRelationTableSpaceMove().
Changed patch to use AccessExclusiveLock in this part for now. This is
what
'ALTER TABLE/INDEX ... SET TABLESPACE' and 'REINDEX' usually do.
Anyway, all
real leaf partitions are processed in the independent transactions
later.
+ if (partkind == RELKIND_PARTITIONED_INDEX)
+ {
+ Relation iRel = index_open(partoid, AccessExclusiveLock);
+
+ if (CheckRelationTableSpaceMove(iRel,
params->tablespaceOid))
+ SetRelationTableSpace(iRel,
+ params->tablespaceOid,
+ InvalidOid);
+ index_close(iRel, NoLock);
Are you sure that this does not represent a risk of deadlocks as EAL
is not taken consistently across all the partitions? A second issue
here is that this breaks the assumption of REINDEX CONCURRENTLY kicked
on partitioned relations that should use ShareUpdateExclusiveLock for
all its steps. This would make the first transaction invasive for the
user, but we don't want that.
This makes me really wonder if we would not be better to restrict this
operation for partitioned relation as part of REINDEX as a first step.
Another thing, mentioned upthread, is that we could do this part of
the switch at the last transaction, or we could silently *not* do the
switch for partitioned indexes in the flow of REINDEX, letting users
handle that with an extra ALTER TABLE SET TABLESPACE once REINDEX has
finished on all the partitions, cascading the command only on the
partitioned relation of a tree. It may be interesting to look as well
at if we could lower the lock used for partitioned relations with
ALTER TABLE SET TABLESPACE from AEL to SUEL, choosing AEL only if at
least one partition with storage is involved in the command,
CheckRelationTableSpaceMove() discarding anything that has no need to
change.
I am not sure right now, so I split previous patch into two parts:
0001: Adds TABLESPACE into REINDEX with tests, doc and all the stuff we
did before with the only exception that it doesn't move partitioned
indexes into the new tablespace.
Basically, it implements this option "we could silently *not* do the
switch for partitioned indexes in the flow of REINDEX, letting users
handle that with an extra ALTER TABLE SET TABLESPACE once REINDEX has
finished". It probably makes sense, since we are doing tablespace change
altogether with index relation rewrite and don't touch relations without
storage. Doing ALTER INDEX ... SET TABLESPACE will be almost cost-less
on them, since they do not hold any data.
0002: Implements the remaining part where pg_class entry is also changed
for partitioned indexes. I think that we should think more about it,
maybe it is not so dangerous and proper locking strategy could be
achieved.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres CompanyFrom 6322032b472e6b1a76e0ca9326974e5774371fb9 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Mon, 1 Feb 2021 15:20:29 +0300
Subject: [PATCH v10 2/2] Change tablespace of partitioned indexes during
REINDEX.
There are some doubts about proper locking of partitions
here. AccessExclusiveLock is surely enough, but may be
a reason of
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Fri, Jan 29, 2021 at 08:56:47PM +0300, Alexey Kondratov wrote:
> On 2021-01-28 14:42, Alexey Kondratov wrote:
>> No, you are right, we are doing REINDEX with AccessExclusiveLock as it
>> was before. This part is a more specific one. It only applies to
>> partitioned indexes, which do not hold any data, so we do not reindex
>> them directly, only their leafs. However, if we are doing a TABLESPACE
>> change, we have to record it in their pg_class entry, so all future
>> leaf partitions were created in the proper tablespace.
>>
>> That way, we open partitioned index relation only for a reference,
>> i.e. read-only, but modify pg_class entry under a proper lock
>> (RowExclusiveLock). That's why I thought that ShareLock will be
>> enough.
>>
>> IIUC, 'ALTER TABLE ... SET TABLESPACE' uses AccessExclusiveLock even
>> for relations with no storage, since AlterTableGetLockLevel() chooses
>> it if AT_SetTableSpace is met. This is very similar to our case, so
>> probably we should do the same?
>>
>> Actually it is not completely clear for me why
>> ShareUpdateExclusiveLock is sufficient for newly added
>> SetRelationTableSpace() as Michael wrote in the comment.
Nay, it was not fine. That's something Alvaro has mentioned, leading
to 2484329. This also means that the main patch of this thread should
refresh the comments at the top of CheckRelationTableSpaceMove() and
SetRelationTableSpace() to mention that this is used by REINDEX
CONCURRENTLY with a lower lock.
> Changed patch to use AccessExclusiveLock in this part for now. This is what
> 'ALTER TABLE/INDEX ... SET TABLESPACE' and 'REINDEX' usually do. Anyway, all
> real leaf partitions are processed in the independent transactions later.
+ if (partkind == RELKIND_PARTITIONED_INDEX)
+ {
+ Relation iRel = index_open(partoid, AccessExclusiveLock);
+
+ if (CheckRelationTableSpaceMove(iRel, params->tablespaceOid))
+ SetRelationTableSpace(iRel,
+ params->tablespaceOid,
+ InvalidOid);
+ index_close(iRel, NoLock);
Are you sure that this does not represent a risk of deadlocks as EAL
is not taken consistently across all the partitions? A second issue
here is that this breaks the assumption of REINDEX CONCURRENTLY kicked
on partitioned relations that should use ShareUpdateExclusiveLock for
all its steps. This would make the first transaction invasive for the
user, but we don't want that.
This makes me really wonder if we would not be better to restrict this
operation for partitioned relation as part of REINDEX as a first step.
Another thing, mentioned upthread, is that we could do this part of
the switch at the last transaction, or we could silently *not* do the
switch for partitioned indexes in the flow of REINDEX, letting users
handle that with an extra ALTER TABLE SET TABLESPACE once REINDEX has
finished on all the partitions, cascading the command only on the
partitioned relation of a tree. It may be interesting to look as well
at if we could lower the lock used for partitioned relations with
ALTER TABLE SET TABLESPACE from AEL to SUEL, choosing AEL only if at
least one partition with storage is involved in the command,
CheckRelationTableSpaceMove() discarding anything that has no need to
change.
--
Michael
signature.asc
Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-01-28 14:42, Alexey Kondratov wrote:
On 2021-01-28 00:36, Alvaro Herrera wrote:
I didn't look at the patch closely enough to understand why you're
trying to do something like CLUSTER, VACUUM FULL or REINDEX without
holding full AccessExclusiveLock on the relation. But do keep in mind
that once you hold a lock on a relation, trying to grab a weaker lock
afterwards is pretty pointless.
No, you are right, we are doing REINDEX with AccessExclusiveLock as it
was before. This part is a more specific one. It only applies to
partitioned indexes, which do not hold any data, so we do not reindex
them directly, only their leafs. However, if we are doing a TABLESPACE
change, we have to record it in their pg_class entry, so all future
leaf partitions were created in the proper tablespace.
That way, we open partitioned index relation only for a reference,
i.e. read-only, but modify pg_class entry under a proper lock
(RowExclusiveLock). That's why I thought that ShareLock will be
enough.
IIUC, 'ALTER TABLE ... SET TABLESPACE' uses AccessExclusiveLock even
for relations with no storage, since AlterTableGetLockLevel() chooses
it if AT_SetTableSpace is met. This is very similar to our case, so
probably we should do the same?
Actually it is not completely clear for me why
ShareUpdateExclusiveLock is sufficient for newly added
SetRelationTableSpace() as Michael wrote in the comment.
Changed patch to use AccessExclusiveLock in this part for now. This is
what 'ALTER TABLE/INDEX ... SET TABLESPACE' and 'REINDEX' usually do.
Anyway, all real leaf partitions are processed in the independent
transactions later.
Also changed some doc/comment parts Justin pointed me to.
+ then all "mapped" and system relations will be skipped and a
single
+ WARNING will be generated. Indexes on TOAST
tables
+ are reindexed, but not moved the new tablespace.
moved *to* the new tablespace.
Fixed.
I don't know if that needs to be said at all. We talked about it a lot
to
arrive at the current behavior, but I think that's only due to the
difficulty
of correcting the initial mistake.
I do not think that it will be a big deal to move indexes on TOAST
tables as well. I just thought that since 'ALTER TABLE/INDEX ... SET
TABLESPACE' only moves them together with host table, we also should not
do that. Yet, I am ready to change this logic if requested.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres CompanyFrom 6e9db8d362e794edf421733bc7cade38c917bff4 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Wed, 27 Jan 2021 00:46:17 +0300
Subject: [PATCH v9] Allow REINDEX to change tablespace
REINDEX already does full relation rewrite, this patch adds a
possibility to specify a new tablespace where new relfilenode
will be created.
---
doc/src/sgml/ref/reindex.sgml | 31 +++-
src/backend/catalog/index.c | 47 +-
src/backend/commands/indexcmds.c | 112 -
src/bin/psql/tab-complete.c | 4 +-
src/include/catalog/index.h | 9 +-
src/test/regress/input/tablespace.source | 106 +
src/test/regress/output/tablespace.source | 181 ++
7 files changed, 478 insertions(+), 12 deletions(-)
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 627b36300c..2b39699d42 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -27,6 +27,7 @@ REINDEX [ ( option [, ...] ) ] { IN
CONCURRENTLY [ boolean ]
VERBOSE [ boolean ]
+TABLESPACE new_tablespace
@@ -187,6 +188,21 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+TABLESPACE
+
+
+ Specifies that indexes will be rebuilt on a new tablespace.
+ Cannot be used with "mapped" or (unless allow_system_table_mods)
+ system relations. If SCHEMA,
+ DATABASE or SYSTEM are specified,
+ then all "mapped" and system relations will be skipped and a single
+ WARNING will be generated. Indexes on TOAST tables
+ are reindexed, but not moved to the new tablespace.
+
+
+
+
VERBOSE
@@ -210,6 +226,14 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+new_tablespace
+
+
+ The tablespace where indexes will be rebuilt.
+
+
+
@@ -292,7 +316,12 @@ REINDEX [ ( option [, ...] ) ] { IN
with REINDEX INDEX or REINDEX TABLE,
respectively. Each partition of the specified partitioned relation is
reindexed in a separate transaction. Those commands cannot be used inside
- a transaction block when working on a partitioned table or index.
+ a transaction block when working on a partitioned table or index. If
+ a REINDEX command fails when run on a partitioned
+ relation, and TABLESPACE was specified, then it may have
+ moved indexes on some partitions to the new tablespace. Re-running the command
+ will reindex all
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-01-28 00:36, Alvaro Herrera wrote: On 2021-Jan-28, Alexey Kondratov wrote: I have read more about lock levels and ShareLock should prevent any kind of physical modification of indexes. We already hold ShareLock doing find_all_inheritors(), which is higher than ShareUpdateExclusiveLock, so using ShareLock seems to be safe here, but I will look on it closer. You can look at lock.c where LockConflicts[] is; that would tell you that ShareLock indeed conflicts with ShareUpdateExclusiveLock ... but it does not conflict with itself! So it would be possible to have more than one process doing this thing at the same time, which surely makes no sense. Thanks for the explanation and pointing me to the LockConflicts[]. This is a good reference. I didn't look at the patch closely enough to understand why you're trying to do something like CLUSTER, VACUUM FULL or REINDEX without holding full AccessExclusiveLock on the relation. But do keep in mind that once you hold a lock on a relation, trying to grab a weaker lock afterwards is pretty pointless. No, you are right, we are doing REINDEX with AccessExclusiveLock as it was before. This part is a more specific one. It only applies to partitioned indexes, which do not hold any data, so we do not reindex them directly, only their leafs. However, if we are doing a TABLESPACE change, we have to record it in their pg_class entry, so all future leaf partitions were created in the proper tablespace. That way, we open partitioned index relation only for a reference, i.e. read-only, but modify pg_class entry under a proper lock (RowExclusiveLock). That's why I thought that ShareLock will be enough. IIUC, 'ALTER TABLE ... SET TABLESPACE' uses AccessExclusiveLock even for relations with no storage, since AlterTableGetLockLevel() chooses it if AT_SetTableSpace is met. This is very similar to our case, so probably we should do the same? Actually it is not completely clear for me why ShareUpdateExclusiveLock is sufficient for newly added SetRelationTableSpace() as Michael wrote in the comment. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-Jan-28, Alexey Kondratov wrote: > I have read more about lock levels and ShareLock should prevent any kind of > physical modification of indexes. We already hold ShareLock doing > find_all_inheritors(), which is higher than ShareUpdateExclusiveLock, so > using ShareLock seems to be safe here, but I will look on it closer. You can look at lock.c where LockConflicts[] is; that would tell you that ShareLock indeed conflicts with ShareUpdateExclusiveLock ... but it does not conflict with itself! So it would be possible to have more than one process doing this thing at the same time, which surely makes no sense. I didn't look at the patch closely enough to understand why you're trying to do something like CLUSTER, VACUUM FULL or REINDEX without holding full AccessExclusiveLock on the relation. But do keep in mind that once you hold a lock on a relation, trying to grab a weaker lock afterwards is pretty pointless. -- Álvaro Herrera39°49'30"S 73°17'W "E pur si muove" (Galileo Galilei)
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Thanks for updating the patch. I have just a couple comments on the new (and old) language. On Thu, Jan 28, 2021 at 12:19:06AM +0300, Alexey Kondratov wrote: > Also added tests for ACL checks, relfilenode changes. Added ACL recheck for > multi-transactional case. Added info about TOAST index reindexing. Changed > some comments. > + Specifies that indexes will be rebuilt on a new tablespace. > + Cannot be used with "mapped" and system (unless > allow_system_table_mods say mapped *or* system relations Or maybe: mapped or (unless >allow_system_table_mods<) system relations. > + is set to TRUE) relations. If > SCHEMA, > + DATABASE or SYSTEM are specified, > + then all "mapped" and system relations will be skipped and a single > + WARNING will be generated. Indexes on TOAST tables > + are reindexed, but not moved the new tablespace. moved *to* the new tablespace. I don't know if that needs to be said at all. We talked about it a lot to arrive at the current behavior, but I think that's only due to the difficulty of correcting the initial mistake. > + /* > + * Set the new tablespace for the relation. Do that only in the > + * case where the reindex caller wishes to enforce a new tablespace. I'd say just "/* Set new tablespace, if requested */ You wrote something similar in an earlier revision of your refactoring patch. > + * Mark the relation as ready to be dropped at transaction > commit, > + * before making visible the new tablespace change so as this > won't > + * miss things. This comment is vague. I think Michael first wrote this comment about a year ago. Does it mean "so the new tablespace won't be missed" ? Missed by what ? -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-01-27 06:14, Michael Paquier wrote:
On Wed, Jan 27, 2021 at 01:00:50AM +0300, Alexey Kondratov wrote:
In the new 0002 I moved ACL check to the upper level, i.e.
ExecReindex(),
and removed expensive text generation in test. Not touched yet some of
your
previously raised concerns. Also, you made SetRelationTableSpace() to
accept
Relation instead of Oid, so now we have to open/close indexes in the
ReindexPartitions(), I am not sure that I use proper locking there,
but it
works.
Passing down Relation to the new routines makes the most sense to me
because we force the callers to think about the level of locking
that's required when doing any tablespace moves.
+ Relation iRel = index_open(partoid, ShareLock);
+
+ if (CheckRelationTableSpaceMove(iRel,
params->tablespaceOid))
+ SetRelationTableSpace(iRel,
+ params->tablespaceOid,
+ InvalidOid);
Speaking of which, this breaks the locking assumptions of
SetRelationTableSpace(). I feel that we should think harder about
this part for partitioned indexes and tables because this looks rather
unsafe in terms of locking assumptions with partition trees. If we
cannot come up with a safe solution, I would be fine with disallowing
TABLESPACE in this case, as a first step. Not all problems have to be
solved at once, and even without this part the feature is still
useful.
I have read more about lock levels and ShareLock should prevent any kind
of physical modification of indexes. We already hold ShareLock doing
find_all_inheritors(), which is higher than ShareUpdateExclusiveLock, so
using ShareLock seems to be safe here, but I will look on it closer.
+ /* It's not a shared catalog, so refuse to move it to shared
tablespace */
+ if (params->tablespaceOid == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("cannot move non-shared relation to tablespace
\"%s\"",
+get_tablespace_name(params->tablespaceOid;
Why is that needed if CheckRelationTableSpaceMove() is used?
This is from ReindexRelationConcurrently() where we do not use
CheckRelationTableSpaceMove(). For me it makes sense to add only this
GLOBALTABLESPACE_OID check there, since before we already check for
system catalogs and after for temp relations, so adding
CheckRelationTableSpaceMove() will be a double-check.
- indexRelation->rd_rel->reltablespace,
+ OidIsValid(tablespaceOid) ?
+ tablespaceOid :
indexRelation->rd_rel->reltablespace,
Let's remove this logic from index_concurrently_create_copy() and let
the caller directly decide the tablespace to use, without a dependency
on InvalidOid in the inner routine. A share update exclusive lock is
already hold on the old index when creating the concurrent copy, so
there won't be concurrent schema changes.
Changed.
Also added tests for ACL checks, relfilenode changes. Added ACL recheck
for multi-transactional case. Added info about TOAST index reindexing.
Changed some comments.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres CompanyFrom f176a6e5a81ab133fee849f72e4edb8b287d6062 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Wed, 27 Jan 2021 00:46:17 +0300
Subject: [PATCH v8] Allow REINDEX to change tablespace
REINDEX already does full relation rewrite, this patch adds a
possibility to specify a new tablespace where new relfilenode
will be created.
---
doc/src/sgml/ref/reindex.sgml | 31 +++-
src/backend/catalog/index.c | 50 +-
src/backend/commands/indexcmds.c | 112 -
src/bin/psql/tab-complete.c | 4 +-
src/include/catalog/index.h | 9 +-
src/test/regress/input/tablespace.source | 106 +
src/test/regress/output/tablespace.source | 181 ++
7 files changed, 481 insertions(+), 12 deletions(-)
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 627b36300c..e610a0f52c 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -27,6 +27,7 @@ REINDEX [ ( option [, ...] ) ] { IN
CONCURRENTLY [ boolean ]
VERBOSE [ boolean ]
+TABLESPACE new_tablespace
@@ -187,6 +188,21 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+TABLESPACE
+
+
+ Specifies that indexes will be rebuilt on a new tablespace.
+ Cannot be used with "mapped" and system (unless allow_system_table_mods
+ is set to TRUE) relations. If SCHEMA,
+ DATABASE or SYSTEM are specified,
+ then all "mapped" and system relations will be skipped and a single
+ WARNING will be generated. Indexes on TOAST tables
+ are reindexed, but not moved the new tablespace.
+
+
+
+
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Jan 27, 2021 at 01:00:50AM +0300, Alexey Kondratov wrote:
> CheckRelationTableSpaceMove() does not feel like a right place for invoking
> post alter hooks. It is intended only to check for tablespace change
> possibility. Anyway, ATExecSetTableSpace() and
> ATExecSetTableSpaceNoStorage() already do that in the no-op case.
>
> I have removed this InvokeObjectPostAlterHook() from your 0001 and made 0002
> to work on top of it. I think that now it should look closer to what you
> described above.
Yeah, I was a bit hesitating about this part as those new routines
would not be used by ALTER-related commands in the next steps. Your
patch got that midway in-between though, by adding the hook to
SetRelationTableSpace but not to CheckRelationTableSpaceMove(). For
now, I have kept the hook out of those new routines because using an
ALTER hook for a utility command is inconsistent. Perhaps we'd want a
separate hook type dedicated to utility commands in objectaccess.c.
I have double-checked the code, and applied it after a few tweaks.
> In the new 0002 I moved ACL check to the upper level, i.e. ExecReindex(),
> and removed expensive text generation in test. Not touched yet some of your
> previously raised concerns. Also, you made SetRelationTableSpace() to accept
> Relation instead of Oid, so now we have to open/close indexes in the
> ReindexPartitions(), I am not sure that I use proper locking there, but it
> works.
Passing down Relation to the new routines makes the most sense to me
because we force the callers to think about the level of locking
that's required when doing any tablespace moves.
+ Relation iRel = index_open(partoid, ShareLock);
+
+ if (CheckRelationTableSpaceMove(iRel, params->tablespaceOid))
+ SetRelationTableSpace(iRel,
+ params->tablespaceOid,
+ InvalidOid);
Speaking of which, this breaks the locking assumptions of
SetRelationTableSpace(). I feel that we should think harder about
this part for partitioned indexes and tables because this looks rather
unsafe in terms of locking assumptions with partition trees. If we
cannot come up with a safe solution, I would be fine with disallowing
TABLESPACE in this case, as a first step. Not all problems have to be
solved at once, and even without this part the feature is still
useful.
+ /* It's not a shared catalog, so refuse to move it to shared tablespace */
+ if (params->tablespaceOid == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("cannot move non-shared relation to tablespace \"%s\"",
+get_tablespace_name(params->tablespaceOid;
Why is that needed if CheckRelationTableSpaceMove() is used?
bits32 options;/* bitmask of REINDEXOPT_* */
+ Oid tablespaceOid; /* tablespace to rebuild index */
} ReindexParams;
Mentioned upthread, but here I think that we should tell that
InvalidOid => keep the existing tablespace.
--
Michael
signature.asc
Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-01-26 09:58, Michael Paquier wrote:
On Mon, Jan 25, 2021 at 11:11:38PM +0300, Alexey Kondratov wrote:
I updated comment with CCI info, did pgindent run and renamed new
function
to SetRelationTableSpace(). New patch is attached.
[...]
Yeah, all these checks we complicated from the beginning. I will try
to find
a better place tomorrow or put more info into the comments at least.
I was reviewing that, and I think that we can do a better
consolidation on several points that will also help the features
discussed on this thread for VACUUM, CLUSTER and REINDEX.
If you look closely, ATExecSetTableSpace() uses the same logic as the
code modified here to check if a relation can be moved to a new
tablespace, with extra checks for mapped relations,
GLOBALTABLESPACE_OID or if attempting to manipulate a temp relation
from another session. There are two differences though:
- Custom actions are taken between the phase where we check if a
relation can be moved to a new tablespace, and the update of
pg_class.
- ATExecSetTableSpace() needs to be able to set a given relation
relfilenode on top of reltablespace, the newly-created one.
So I think that the heart of the problem is made of two things here:
- We should have one common routine for the existing code paths and
the new code paths able to check if a tablespace move can be done or
not. The case of a cluster, reindex or vacuum on a list of relations
extracted from pg_class would still require a different handling
as incorrect relations have to be skipped, but the case of individual
relations can reuse the refactoring pieces done here
(see CheckRelationTableSpaceMove() in the attached).
- We need to have a second routine able to update reltablespace and
optionally relfilenode for a given relation's pg_class entry, once the
caller has made sure that CheckRelationTableSpaceMove() validates a
tablespace move.
I think that I got your idea. One comment:
+bool
+CheckRelationTableSpaceMove(Relation rel, Oid newTableSpaceId)
+{
+ Oid oldTableSpaceId;
+ Oid reloid = RelationGetRelid(rel);
+
+ /*
+* No work if no change in tablespace. Note that MyDatabaseTableSpace
+* is stored as 0.
+*/
+ oldTableSpaceId = rel->rd_rel->reltablespace;
+ if (newTableSpaceId == oldTableSpaceId ||
+ (newTableSpaceId == MyDatabaseTableSpace && oldTableSpaceId ==
0))
+ {
+ InvokeObjectPostAlterHook(RelationRelationId, reloid, 0);
+ return false;
+ }
CheckRelationTableSpaceMove() does not feel like a right place for
invoking post alter hooks. It is intended only to check for tablespace
change possibility. Anyway, ATExecSetTableSpace() and
ATExecSetTableSpaceNoStorage() already do that in the no-op case.
Please note that was a bug in your previous patch 0002: shared
dependencies need to be registered if reltablespace is updated of
course, but also iff the relation has no physical storage. So
changeDependencyOnTablespace() requires a check based on
RELKIND_HAS_STORAGE(), or REINDEX would have registered shared
dependencies even for relations with storage, something we don't
want per the recent work done by Alvaro in ebfe2db.
Yes, thanks.
I have removed this InvokeObjectPostAlterHook() from your 0001 and made
0002 to work on top of it. I think that now it should look closer to
what you described above.
In the new 0002 I moved ACL check to the upper level, i.e.
ExecReindex(), and removed expensive text generation in test. Not
touched yet some of your previously raised concerns. Also, you made
SetRelationTableSpace() to accept Relation instead of Oid, so now we
have to open/close indexes in the ReindexPartitions(), I am not sure
that I use proper locking there, but it works.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres CompanyFrom 96a37399a9cf9ae08d62e28496e73b36087e5a19 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 26 Jan 2021 15:53:06 +0900
Subject: [PATCH v7 1/2] Refactor code to detect and process tablespace moves
---
src/backend/commands/tablecmds.c | 218 +--
src/include/commands/tablecmds.h | 4 +
2 files changed, 127 insertions(+), 95 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8687e9a97c..c08eedf995 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3037,6 +3037,116 @@ SetRelationHasSubclass(Oid relationId, bool relhassubclass)
table_close(relationRelation, RowExclusiveLock);
}
+/*
+ * CheckRelationTableSpaceMove
+ * Check if relation can be moved to new tablespace.
+ *
+ * NOTE: caller must be holding an appropriate lock on the relation.
+ * ShareUpdateExclusiveLock is sufficient to prevent concurrent schema
+ * changes.
+ *
+ * Returns true if the relation can be moved to the new tablespace;
+ * false
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Mon, Jan 25, 2021 at 11:11:38PM +0300, Alexey Kondratov wrote:
> I updated comment with CCI info, did pgindent run and renamed new function
> to SetRelationTableSpace(). New patch is attached.
>
> [...]
>
> Yeah, all these checks we complicated from the beginning. I will try to find
> a better place tomorrow or put more info into the comments at least.
I was reviewing that, and I think that we can do a better
consolidation on several points that will also help the features
discussed on this thread for VACUUM, CLUSTER and REINDEX.
If you look closely, ATExecSetTableSpace() uses the same logic as the
code modified here to check if a relation can be moved to a new
tablespace, with extra checks for mapped relations,
GLOBALTABLESPACE_OID or if attempting to manipulate a temp relation
from another session. There are two differences though:
- Custom actions are taken between the phase where we check if a
relation can be moved to a new tablespace, and the update of
pg_class.
- ATExecSetTableSpace() needs to be able to set a given relation
relfilenode on top of reltablespace, the newly-created one.
So I think that the heart of the problem is made of two things here:
- We should have one common routine for the existing code paths and
the new code paths able to check if a tablespace move can be done or
not. The case of a cluster, reindex or vacuum on a list of relations
extracted from pg_class would still require a different handling
as incorrect relations have to be skipped, but the case of individual
relations can reuse the refactoring pieces done here
(see CheckRelationTableSpaceMove() in the attached).
- We need to have a second routine able to update reltablespace and
optionally relfilenode for a given relation's pg_class entry, once the
caller has made sure that CheckRelationTableSpaceMove() validates a
tablespace move.
Please note that was a bug in your previous patch 0002: shared
dependencies need to be registered if reltablespace is updated of
course, but also iff the relation has no physical storage. So
changeDependencyOnTablespace() requires a check based on
RELKIND_HAS_STORAGE(), or REINDEX would have registered shared
dependencies even for relations with storage, something we don't
want per the recent work done by Alvaro in ebfe2db.
--
Michael
From 265631fb5966ae7b454287c489684c8275b7b001 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 26 Jan 2021 15:53:06 +0900
Subject: [PATCH v6] Refactor code to detect and process tablespace moves
---
src/include/commands/tablecmds.h | 4 +
src/backend/commands/tablecmds.c | 222 ++-
2 files changed, 131 insertions(+), 95 deletions(-)
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 08c463d3c4..b3d30acc35 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -61,6 +61,10 @@ extern void ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_
extern void SetRelationHasSubclass(Oid relationId, bool relhassubclass);
+extern bool CheckRelationTableSpaceMove(Relation rel, Oid newTableSpaceId);
+extern void SetRelationTableSpace(Relation rel, Oid newTableSpaceId,
+ Oid newRelFileNode);
+
extern ObjectAddress renameatt(RenameStmt *stmt);
extern ObjectAddress RenameConstraint(RenameStmt *stmt);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8687e9a97c..1f88eebabd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3037,6 +3037,120 @@ SetRelationHasSubclass(Oid relationId, bool relhassubclass)
table_close(relationRelation, RowExclusiveLock);
}
+/*
+ * CheckRelationTableSpaceMove
+ * Check if relation can be moved to new tablespace.
+ *
+ * NOTE: caller must be holding an appropriate lock on the relation.
+ * ShareUpdateExclusiveLock is sufficient to prevent concurrent schema
+ * changes.
+ *
+ * Returns true if the relation can be moved to the new tablespace;
+ * false otherwise.
+ */
+bool
+CheckRelationTableSpaceMove(Relation rel, Oid newTableSpaceId)
+{
+ Oid oldTableSpaceId;
+ Oid reloid = RelationGetRelid(rel);
+
+ /*
+ * No work if no change in tablespace. Note that MyDatabaseTableSpace
+ * is stored as 0.
+ */
+ oldTableSpaceId = rel->rd_rel->reltablespace;
+ if (newTableSpaceId == oldTableSpaceId ||
+ (newTableSpaceId == MyDatabaseTableSpace && oldTableSpaceId == 0))
+ {
+ InvokeObjectPostAlterHook(RelationRelationId, reloid, 0);
+ return false;
+ }
+
+ /*
+ * We cannot support moving mapped relations into different tablespaces.
+ * (In particular this eliminates all shared catalogs.)
+ */
+ if (RelationIsMapped(rel))
+ ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move system relation \"%s\"",
+ RelationGetRelationName(rel;
+
+ /* Cannot move a non-shared relation into pg_global */
+ if (newTableSpaceId == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-01-25 11:07, Michael Paquier wrote:
On Fri, Jan 22, 2021 at 05:07:02PM +0300, Alexey Kondratov wrote:
I have updated patches accordingly and also simplified tablespaceOid
checks
and assignment in the newly added SetRelTableSpace(). Result is
attached as
two separate patches for an ease of review, but no objections to merge
them
and apply at once if everything is fine.
extern void SetRelationHasSubclass(Oid relationId, bool
relhassubclass);
+extern bool SetRelTableSpace(Oid reloid, Oid tablespaceOid);
Seeing SetRelationHasSubclass(), wouldn't it be more consistent to use
SetRelationTableSpace() as routine name?
I think that we should document that the caller of this routine had
better do a CCI once done to make the tablespace chage visible.
Except for those two nits, the patch needs an indentation run and some
style tweaks but its logic looks fine. So I'll apply that first
piece.
I updated comment with CCI info, did pgindent run and renamed new
function to SetRelationTableSpace(). New patch is attached.
+INSERT INTO regress_tblspace_test_tbl (num1, num2, t)
+ SELECT round(random()*100), random(), repeat('text', 100)
+ FROM generate_series(1, 10) s(i);
Repeating 1M times a text value is too costly for such a test. And as
even for empty tables there is one page created for toast indexes,
there is no need for that?
Yes, TOAST relation is created anyway. I just wanted to put some data
into a TOAST index, so REINDEX did some meaningful work there, not only
a new relfilenode creation. However you are right and this query
increases tablespace tests execution for more for more than 2 times on
my machine. I think that it is not really required.
This patch is introducing three new checks for system catalogs:
- don't use tablespace for mapped relations.
- don't use tablespace for system relations, except if
allowSystemTableMods.
- don't move non-shared relation to global tablespace.
For the non-concurrent case, all three checks are in reindex_index().
For the concurrent case, the two first checks are in
ReindexMultipleTables() and the third one is in
ReindexRelationConcurrently(). That's rather tricky to follow because
CONCURRENTLY is not allowed on system relations. I am wondering if it
would be worth an extra comment effort, or if there is a way to
consolidate that better.
Yeah, all these checks we complicated from the beginning. I will try to
find a better place tomorrow or put more info into the comments at
least.
I am also going to check/fix the remaining points regarding 002
tomorrow.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres CompanyFrom 39880842d7af31dcbfcffe7219250b31102955d5 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Wed, 20 Jan 2021 20:21:12 +0300
Subject: [PATCH v5 1/2] Extract common part from ATExecSetTableSpaceNoStorage
for a future usage
---
src/backend/commands/tablecmds.c | 95 +++-
src/include/commands/tablecmds.h | 2 +
2 files changed, 58 insertions(+), 39 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8687e9a97c..ec9c440e4e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13291,6 +13291,59 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
list_free(reltoastidxids);
}
+/*
+ * SetRelationTableSpace - modify relation tablespace in the pg_class entry.
+ *
+ * 'reloid' is an Oid of relation to be modified.
+ * 'tablespaceOid' is an Oid of new tablespace.
+ *
+ * Catalog modification is done only if tablespaceOid is different from
+ * the currently set. Returned bool value is indicating whether any changes
+ * were made or not. Note that caller is responsible for doing
+ * CommandCounterIncrement() to make tablespace changes visible.
+ */
+bool
+SetRelationTableSpace(Oid reloid, Oid tablespaceOid)
+{
+ Relation pg_class;
+ HeapTuple tuple;
+ Form_pg_class rd_rel;
+ bool changed = false;
+
+ /* Get a modifiable copy of the relation's pg_class row. */
+ pg_class = table_open(RelationRelationId, RowExclusiveLock);
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", reloid);
+ rd_rel = (Form_pg_class) GETSTRUCT(tuple);
+
+ /* MyDatabaseTableSpace is stored as InvalidOid. */
+ if (tablespaceOid == MyDatabaseTableSpace)
+ tablespaceOid = InvalidOid;
+
+ /* No work if no change in tablespace. */
+ if (tablespaceOid != rd_rel->reltablespace)
+ {
+ /* Update the pg_class row. */
+ rd_rel->reltablespace = tablespaceOid;
+ CatalogTupleUpdate(pg_class, >t_self, tuple);
+
+ /* Record dependency on tablespace. */
+ changeDependencyOnTablespace(RelationRelationId,
+ reloid, rd_rel->reltablespace);
+
+ changed = true;
+ }
+
+ InvokeObjectPostAlterHook(RelationRelationId, reloid, 0);
+
+ heap_freetuple(tuple);
+
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Mon, Jan 25, 2021 at 05:07:29PM +0900, Michael Paquier wrote: > On Fri, Jan 22, 2021 at 05:07:02PM +0300, Alexey Kondratov wrote: > > I have updated patches accordingly and also simplified tablespaceOid checks > > and assignment in the newly added SetRelTableSpace(). Result is attached as > > two separate patches for an ease of review, but no objections to merge them > > and apply at once if everything is fine. ... > +SELECT relname FROM pg_class > +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE > spcname='regress_tblspace'); > [...] > +-- first, check a no-op case > +REINDEX (TABLESPACE pg_default) INDEX regress_tblspace_test_tbl_idx; > +REINDEX (TABLESPACE pg_default) TABLE regress_tblspace_test_tbl; > Reindexing means that the relfilenodes are changed, so the tests > should track the original and new relfilenodes and compare them, no? > In short, this set of regression tests does not make sure that a > REINDEX actually happens or not, and this may read as a reindex not > happening at all for those tests. For single units, these could be > saved in a variable and compared afterwards. create_index.sql does > that a bit with REINDEX SCHEMA for a set of relations. You might also check my "CLUSTER partitioned" patch for another way to do that. https://www.postgresql.org/message-id/20210118183459.GJ8560%40telsasoft.com https://www.postgresql.org/message-id/attachment/118126/v6-0002-Implement-CLUSTER-of-partitioned-table.patch -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-01-22 00:26, Justin Pryzby wrote:
On Thu, Jan 21, 2021 at 11:48:08PM +0300, Alexey Kondratov wrote:
Attached is a new patch set of first two patches, that should resolve
all
the issues raised before (ACL, docs, tests) excepting TOAST. Double
thanks
for suggestion to add more tests with nested partitioning. I have
found and
squashed a huge bug related to the returning back to the default
tablespace
using newly added tests.
Regarding TOAST. Now we skip moving toast indexes or throw error if
someone
wants to move TOAST index directly. I had a look on ALTER TABLE SET
TABLESPACE and it has a bit complicated logic:
1) You cannot move TOAST table directly.
2) But if you move basic relation that TOAST table belongs to, then
they are
moved altogether.
3) Same logic as 2) happens if one does ALTER TABLE ALL IN TABLESPACE
...
That way, ALTER TABLE allows moving TOAST tables (with indexes)
implicitly,
but does not allow doing that explicitly. In the same time I found
docs to
be vague about such behavior it only says:
All tables in the current database in a tablespace can be moved
by using the ALL IN TABLESPACE ... Note that system catalogs are
not moved by this command
Changing any part of a system catalog table is not permitted.
So actually ALTER TABLE treats TOAST relations as system sometimes,
but
sometimes not.
From the end user perspective it makes sense to move TOAST with main
table
when doing ALTER TABLE SET TABLESPACE. But should we touch indexes on
TOAST
table with REINDEX? We cannot move TOAST relation itself, since we are
doing
only a reindex, so we end up in the state when TOAST table and its
index are
placed in the different tablespaces. This state is not reachable with
ALTER
TABLE/INDEX, so it seem we should not allow it with REINDEX as well,
should
we?
+ * Even if a table's indexes were moved to a new tablespace, the
index
+* on its toast table is not normally moved.
*/
ReindexParams newparams = *params;
newparams.options &= ~(REINDEXOPT_MISSING_OK);
+ if (!allowSystemTableMods)
+ newparams.tablespaceOid = InvalidOid;
I think you're right. So actually TOAST should never move, even if
allowSystemTableMods, right ?
I think so. I would prefer to do not move TOAST indexes implicitly at
all during reindex.
@@ -292,7 +315,11 @@ REINDEX [ ( class="parameter">option [, ...] ) ] { IN
with REINDEX INDEX or REINDEX
TABLE,
respectively. Each partition of the specified partitioned relation
is
reindexed in a separate transaction. Those commands cannot be used
inside
- a transaction block when working on a partitioned table or index.
+ a transaction block when working on a partitioned table or index.
If
+ REINDEX with TABLESPACE
executed
+ on partitioned relation fails it may have moved some partitions to
the new
+ tablespace. Repeated command will still reindex all partitions
even if they
+ are already in the new tablespace.
Minor corrections here:
If a REINDEX command fails when run on a partitioned
relation, and TABLESPACE was specified, then it may
have
moved indexes on some partitions to the new tablespace. Re-running the
command
will reindex all partitions and move previously-unprocessed indexes to
the new
tablespace.
Sounds good to me.
I have updated patches accordingly and also simplified tablespaceOid
checks and assignment in the newly added SetRelTableSpace(). Result is
attached as two separate patches for an ease of review, but no
objections to merge them and apply at once if everything is fine.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres CompanyFrom 87e47e9b5b3d6b49230045e5db8f844b14b34ba0 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Mon, 23 Mar 2020 21:10:29 +0300
Subject: [PATCH v4 2/2] Allow REINDEX to change tablespace
REINDEX already does full relation rewrite, this patch adds a
possibility to specify a new tablespace where new relfilenode
will be created.
---
doc/src/sgml/ref/reindex.sgml | 30 +++-
src/backend/catalog/index.c | 81 ++-
src/backend/commands/indexcmds.c | 81 ++-
src/bin/psql/tab-complete.c | 4 +-
src/include/catalog/index.h | 2 +
src/test/regress/input/tablespace.source | 85
src/test/regress/output/tablespace.source | 159 ++
7 files changed, 436 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 627b36300c..a1c7736aec 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -27,6 +27,7 @@ REINDEX [ ( option [, ...] ) ] { IN
CONCURRENTLY [ boolean ]
VERBOSE [ boolean ]
+TABLESPACE new_tablespace
@@ -187,6 +188,20 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Thu, Jan 21, 2021 at 11:48:08PM +0300, Alexey Kondratov wrote:
> Attached is a new patch set of first two patches, that should resolve all
> the issues raised before (ACL, docs, tests) excepting TOAST. Double thanks
> for suggestion to add more tests with nested partitioning. I have found and
> squashed a huge bug related to the returning back to the default tablespace
> using newly added tests.
>
> Regarding TOAST. Now we skip moving toast indexes or throw error if someone
> wants to move TOAST index directly. I had a look on ALTER TABLE SET
> TABLESPACE and it has a bit complicated logic:
>
> 1) You cannot move TOAST table directly.
> 2) But if you move basic relation that TOAST table belongs to, then they are
> moved altogether.
> 3) Same logic as 2) happens if one does ALTER TABLE ALL IN TABLESPACE ...
>
> That way, ALTER TABLE allows moving TOAST tables (with indexes) implicitly,
> but does not allow doing that explicitly. In the same time I found docs to
> be vague about such behavior it only says:
>
> All tables in the current database in a tablespace can be moved
> by using the ALL IN TABLESPACE ... Note that system catalogs are
> not moved by this command
>
> Changing any part of a system catalog table is not permitted.
>
> So actually ALTER TABLE treats TOAST relations as system sometimes, but
> sometimes not.
>
> From the end user perspective it makes sense to move TOAST with main table
> when doing ALTER TABLE SET TABLESPACE. But should we touch indexes on TOAST
> table with REINDEX? We cannot move TOAST relation itself, since we are doing
> only a reindex, so we end up in the state when TOAST table and its index are
> placed in the different tablespaces. This state is not reachable with ALTER
> TABLE/INDEX, so it seem we should not allow it with REINDEX as well, should
> we?
> + * Even if a table's indexes were moved to a new tablespace,
> the index
> + * on its toast table is not normally moved.
>*/
> ReindexParams newparams = *params;
>
> newparams.options &= ~(REINDEXOPT_MISSING_OK);
> + if (!allowSystemTableMods)
> + newparams.tablespaceOid = InvalidOid;
I think you're right. So actually TOAST should never move, even if
allowSystemTableMods, right ?
> @@ -292,7 +315,11 @@ REINDEX [ ( class="parameter">option [, ...] ) ] { IN
> with REINDEX INDEX or REINDEX TABLE,
> respectively. Each partition of the specified partitioned relation is
> reindexed in a separate transaction. Those commands cannot be used inside
> - a transaction block when working on a partitioned table or index.
> + a transaction block when working on a partitioned table or index. If
> + REINDEX with TABLESPACE executed
> + on partitioned relation fails it may have moved some partitions to the new
> + tablespace. Repeated command will still reindex all partitions even if
> they
> + are already in the new tablespace.
Minor corrections here:
If a REINDEX command fails when run on a partitioned
relation, and TABLESPACE was specified, then it may have
moved indexes on some partitions to the new tablespace. Re-running the command
will reindex all partitions and move previously-unprocessed indexes to the new
tablespace.
--
Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-01-21 17:06, Alexey Kondratov wrote:
On 2021-01-21 04:41, Michael Paquier wrote:
There are no tests for partitioned tables, aka we'd want to make sure
that the new partitioned index is on the correct tablespace, as well
as all its leaves. It may be better to have at least two levels of
partitioned tables, as well as a partitioned table with no leaves in
the cases dealt with.
Yes, sure, it makes sense.
+*
+* Even if a table's indexes were moved to a new tablespace,
the index
+* on its toast table is not normally moved.
*/
Still, REINDEX (TABLESPACE) TABLE should move all of them to be
consistent with ALTER TABLE SET TABLESPACE, but that's not the case
with this code, no? This requires proper test coverage, but there is
nothing of the kind in this patch.
You are right, we do not move TOAST indexes now, since
IsSystemRelation() is true for TOAST indexes, so I thought that we
should not allow moving them without allow_system_table_mods=true. Now
I wonder why ALTER TABLE does that.
I am going to attach the new version of patch set today or tomorrow.
Attached is a new patch set of first two patches, that should resolve
all the issues raised before (ACL, docs, tests) excepting TOAST. Double
thanks for suggestion to add more tests with nested partitioning. I have
found and squashed a huge bug related to the returning back to the
default tablespace using newly added tests.
Regarding TOAST. Now we skip moving toast indexes or throw error if
someone wants to move TOAST index directly. I had a look on ALTER TABLE
SET TABLESPACE and it has a bit complicated logic:
1) You cannot move TOAST table directly.
2) But if you move basic relation that TOAST table belongs to, then they
are moved altogether.
3) Same logic as 2) happens if one does ALTER TABLE ALL IN TABLESPACE
...
That way, ALTER TABLE allows moving TOAST tables (with indexes)
implicitly, but does not allow doing that explicitly. In the same time I
found docs to be vague about such behavior it only says:
All tables in the current database in a tablespace can be moved
by using the ALL IN TABLESPACE ... Note that system catalogs are
not moved by this command
Changing any part of a system catalog table is not permitted.
So actually ALTER TABLE treats TOAST relations as system sometimes, but
sometimes not.
From the end user perspective it makes sense to move TOAST with main
table when doing ALTER TABLE SET TABLESPACE. But should we touch indexes
on TOAST table with REINDEX? We cannot move TOAST relation itself, since
we are doing only a reindex, so we end up in the state when TOAST table
and its index are placed in the different tablespaces. This state is not
reachable with ALTER TABLE/INDEX, so it seem we should not allow it with
REINDEX as well, should we?
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres CompanyFrom bcd690da6bc3db16a96305b45546d3c9e400f769 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Mon, 23 Mar 2020 21:10:29 +0300
Subject: [PATCH v3 2/2] Allow REINDEX to change tablespace
REINDEX already does full relation rewrite, this patch adds a
possibility to specify a new tablespace where new relfilenode
will be created.
---
doc/src/sgml/ref/reindex.sgml | 29 +++-
src/backend/catalog/index.c | 82 +++-
src/backend/commands/indexcmds.c | 81 +++-
src/bin/psql/tab-complete.c | 4 +-
src/include/catalog/index.h | 2 +
src/test/regress/input/tablespace.source | 79 +++
src/test/regress/output/tablespace.source | 154 ++
7 files changed, 425 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 627b36300c..90fdad0b4c 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -27,6 +27,7 @@ REINDEX [ ( option [, ...] ) ] { IN
CONCURRENTLY [ boolean ]
VERBOSE [ boolean ]
+TABLESPACE new_tablespace
@@ -187,6 +188,20 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+TABLESPACE
+
+
+ Specifies that indexes will be rebuilt on a new tablespace.
+ Cannot be used with "mapped" and system (unless allow_system_table_mods
+ is set to TRUE) relations. If SCHEMA,
+ DATABASE or SYSTEM are specified, then
+ all "mapped" and system relations will be skipped and a single
+ WARNING will be generated.
+
+
+
+
VERBOSE
@@ -210,6 +225,14 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+new_tablespace
+
+
+ The tablespace where indexes will be rebuilt.
+
+
+
@@ -292,7 +315,11 @@ REINDEX [ ( option [, ...] ) ] { IN
with REINDEX INDEX or REINDEX TABLE,
respectively. Each partition of the specified partitioned relation is
reindexed in a separate transaction. Those
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-01-21 04:41, Michael Paquier wrote:
On Wed, Jan 20, 2021 at 03:34:39PM -0300, Alvaro Herrera wrote:
On 2021-Jan-20, Alexey Kondratov wrote:
Ugh, forgot to attach the patches. Here they are.
Yeah, looks reasonable.
+
+ if (changed)
+ /* Record dependency on tablespace */
+ changeDependencyOnTablespace(RelationRelationId,
+reloid,
rd_rel->reltablespace);
Why have a separate "if (changed)" block here instead of merging with
the above?
Yep.
Sure, this is a refactoring artifact.
+ if (SetRelTablespace(reloid, newTableSpace))
+ /* Make sure the reltablespace change is visible */
+ CommandCounterIncrement();
At quick glance, I am wondering why you just don't do a CCI within
SetRelTablespace().
I did it that way for a better readability at first, since it looks more
natural when you do some change (SetRelTablespace) and then make them
visible with CCI. Second argument was that in the case of
reindex_index() we have to also call RelationAssumeNewRelfilenode() and
RelationDropStorage() before doing CCI and making the new tablespace
visible. And this part is critical, I guess.
+ This specifies that indexes will be rebuilt on a new tablespace.
+ Cannot be used with "mapped" relations. If
SCHEMA,
+ DATABASE or SYSTEM is
specified, then
+ all unsuitable relations will be skipped and a single
WARNING
+ will be generated.
What is an unsuitable relation? How can the end user know that?
This was referring to mapped relations mentioned in the previous
sentence. I have tried to rewrite this part and make it more specific in
my current version. Also added Justin's changes to the docs and comment.
This is missing ACL checks when moving the index into a new location,
so this requires some pg_tablespace_aclcheck() calls, and the other
patches share the same issue.
I added proper pg_tablespace_aclcheck()'s into the reindex_index() and
ReindexPartitions().
+ else if (partkind == RELKIND_PARTITIONED_TABLE)
+ {
+ Relation rel = table_open(partoid, ShareLock);
+ List*indexIds = RelationGetIndexList(rel);
+ ListCell *lc;
+
+ table_close(rel, NoLock);
+ foreach (lc, indexIds)
+ {
+ Oid indexid = lfirst_oid(lc);
+ (void) set_rel_tablespace(indexid,
params->tablespaceOid);
+ }
+ }
This is really a good question. ReindexPartitions() would trigger one
transaction per leaf to work on. Changing the tablespace of the
partitioned table(s) before doing any work has the advantage to tell
any new partition to use the new tablespace. Now, I see a struggling
point here: what should we do if the processing fails in the middle of
the move, leaving a portion of the leaves in the previous tablespace?
On a follow-up reindex with the same command, should the command force
a reindex even on the partitions that have been moved? Or could there
be a point in skipping the partitions that are already on the new
tablespace and only process the ones on the previous tablespace? It
seems to me that the first scenario makes the most sense as currently
a REINDEX works on all the relations defined, though there could be
use cases for the second case. This should be documented, I think.
I agree that follow-up REINDEX should also reindex moved partitions,
since REINDEX (TABLESPACE ...) is still reindex at first. I will try to
put something about this part into the docs. Also I think that we cannot
be sure that nothing happened with already reindexed partitions between
two consequent REINDEX calls.
There are no tests for partitioned tables, aka we'd want to make sure
that the new partitioned index is on the correct tablespace, as well
as all its leaves. It may be better to have at least two levels of
partitioned tables, as well as a partitioned table with no leaves in
the cases dealt with.
Yes, sure, it makes sense.
+*
+* Even if a table's indexes were moved to a new tablespace,
the index
+* on its toast table is not normally moved.
*/
Still, REINDEX (TABLESPACE) TABLE should move all of them to be
consistent with ALTER TABLE SET TABLESPACE, but that's not the case
with this code, no? This requires proper test coverage, but there is
nothing of the kind in this patch.
You are right, we do not move TOAST indexes now, since
IsSystemRelation() is true for TOAST indexes, so I thought that we
should not allow moving them without allow_system_table_mods=true. Now I
wonder why ALTER TABLE does that.
I am going to attach the new version of patch set today or tomorrow.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Jan 20, 2021 at 03:34:39PM -0300, Alvaro Herrera wrote:
> On 2021-Jan-20, Alexey Kondratov wrote:
>> Ugh, forgot to attach the patches. Here they are.
>
> Yeah, looks reasonable.
Patch 0002 still has a whole set of issues as I pointed out a couple
of hours ago, but if we agree on 0001 as being useful if done
independently, I'd rather get that done first. This way or just
merging both things in a single commit is not a big deal seeing the
amount of code, so I am fine with any approach. It may be possible
that 0001 requires more changes depending on the work to-be-done for
0002 though?
>> +/* No work if no change in tablespace. */
>> +oldTablespaceOid = rd_rel->reltablespace;
>> +if (tablespaceOid != oldTablespaceOid ||
>> +(tablespaceOid == MyDatabaseTableSpace &&
>> OidIsValid(oldTablespaceOid)))
>> +{
>> +/* Update the pg_class row. */
>> +rd_rel->reltablespace = (tablespaceOid == MyDatabaseTableSpace)
>> ?
>> +InvalidOid : tablespaceOid;
>> +CatalogTupleUpdate(pg_class, >t_self, tuple);
>> +
>> +changed = true;
>> +}
>> +
>> +if (changed)
>> +/* Record dependency on tablespace */
>> +changeDependencyOnTablespace(RelationRelationId,
>> +
>> reloid, rd_rel->reltablespace);
>
> Why have a separate "if (changed)" block here instead of merging with
> the above?
Yep.
+ if (SetRelTablespace(reloid, newTableSpace))
+ /* Make sure the reltablespace change is visible */
+ CommandCounterIncrement();
At quick glance, I am wondering why you just don't do a CCI within
SetRelTablespace().
--
Michael
signature.asc
Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-Jan-20, Alexey Kondratov wrote:
> On 2021-01-20 21:08, Alexey Kondratov wrote:
> >
> > I did a refactoring of ATExecSetTableSpaceNoStorage() in the 0001. New
> > function SetRelTablesapce() is placed into the tablecmds.c. Following
> > 0002 gets use of it. Is it close to what you and Michael suggested?
>
> Ugh, forgot to attach the patches. Here they are.
Yeah, looks reasonable.
> + /* No work if no change in tablespace. */
> + oldTablespaceOid = rd_rel->reltablespace;
> + if (tablespaceOid != oldTablespaceOid ||
> + (tablespaceOid == MyDatabaseTableSpace &&
> OidIsValid(oldTablespaceOid)))
> + {
> + /* Update the pg_class row. */
> + rd_rel->reltablespace = (tablespaceOid == MyDatabaseTableSpace)
> ?
> + InvalidOid : tablespaceOid;
> + CatalogTupleUpdate(pg_class, >t_self, tuple);
> +
> + changed = true;
> + }
> +
> + if (changed)
> + /* Record dependency on tablespace */
> + changeDependencyOnTablespace(RelationRelationId,
> +
> reloid, rd_rel->reltablespace);
Why have a separate "if (changed)" block here instead of merging with
the above?
--
Álvaro Herrera39°49'30"S 73°17'W
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-01-20 21:08, Alexey Kondratov wrote:
On 2021-01-20 18:54, Alvaro Herrera wrote:
On 2021-Jan-20, Alvaro Herrera wrote:
On 2021-Jan-20, Michael Paquier wrote:
> +/*
> + * This is mostly duplicating ATExecSetTableSpaceNoStorage,
> + * which should maybe be factored out to a library function.
> + */
> Wouldn't it be better to do first the refactoring of 0002 and then
> 0001 so as REINDEX can use the new routine, instead of putting that
> into a comment?
I think merging 0001 and 0002 into a single commit is a reasonable
approach.
... except it doesn't make a lot of sense to have set_rel_tablespace
in
either indexcmds.c or index.c. I think tablecmds.c is a better place
for it. (I would have thought catalog/storage.c, but that one's not
the
right abstraction level it seems.)
I did a refactoring of ATExecSetTableSpaceNoStorage() in the 0001. New
function SetRelTablesapce() is placed into the tablecmds.c. Following
0002 gets use of it. Is it close to what you and Michael suggested?
Ugh, forgot to attach the patches. Here they are.
--
AlexeyFrom 2c3876f99bc8ebdd07c532619992e7ec3093e50a Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Mon, 23 Mar 2020 21:10:29 +0300
Subject: [PATCH v2 2/2] Allow REINDEX to change tablespace
REINDEX already does full relation rewrite, this patch adds a
possibility to specify a new tablespace where new relfilenode
will be created.
---
doc/src/sgml/ref/reindex.sgml | 22 +
src/backend/catalog/index.c | 72 ++-
src/backend/commands/indexcmds.c | 68 ++-
src/bin/psql/tab-complete.c | 4 +-
src/include/catalog/index.h | 2 +
src/test/regress/input/tablespace.source | 53 +++
src/test/regress/output/tablespace.source | 102 ++
7 files changed, 318 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 627b36300c..4f84060c4d 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -27,6 +27,7 @@ REINDEX [ ( option [, ...] ) ] { IN
CONCURRENTLY [ boolean ]
VERBOSE [ boolean ]
+TABLESPACE new_tablespace
@@ -187,6 +188,19 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+TABLESPACE
+
+
+ This specifies that indexes will be rebuilt on a new tablespace.
+ Cannot be used with "mapped" relations. If SCHEMA,
+ DATABASE or SYSTEM is specified, then
+ all unsuitable relations will be skipped and a single WARNING
+ will be generated.
+
+
+
+
VERBOSE
@@ -210,6 +224,14 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+new_tablespace
+
+
+ The tablespace where indexes will be rebuilt.
+
+
+
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index b8cd35e995..ed98b17483 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -57,6 +57,7 @@
#include "commands/event_trigger.h"
#include "commands/progress.h"
#include "commands/tablecmds.h"
+#include "commands/tablespace.h"
#include "commands/trigger.h"
#include "executor/executor.h"
#include "miscadmin.h"
@@ -1394,9 +1395,13 @@ index_update_collation_versions(Oid relid, Oid coll)
* Create concurrently an index based on the definition of the one provided by
* caller. The index is inserted into catalogs and needs to be built later
* on. This is called during concurrent reindex processing.
+ *
+ * "tablespaceOid" is the new tablespace to use for this index. If
+ * InvalidOid, use the tablespace in-use instead.
*/
Oid
-index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, const char *newName)
+index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
+ Oid tablespaceOid, const char *newName)
{
Relation indexRelation;
IndexInfo *oldInfo,
@@ -1526,7 +1531,8 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, const char
newInfo,
indexColNames,
indexRelation->rd_rel->relam,
- indexRelation->rd_rel->reltablespace,
+ OidIsValid(tablespaceOid) ?
+tablespaceOid : indexRelation->rd_rel->reltablespace,
indexRelation->rd_indcollation,
indclass->values,
indcoloptions->values,
@@ -3591,6 +3597,8 @@ IndexGetRelation(Oid indexId, bool missing_ok)
/*
* reindex_index - This routine is used to recreate a single index
+ *
+ * See comments of reindex_relation() for details about "tablespaceOid".
*/
void
reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
@@ -3603,6 +3611,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
volatile bool skipped_constraint = false;
PGRUsage ru0;
bool progress = ((params->options & REINDEXOPT_REPORT_PROGRESS) != 0);
+ bool set_tablespace = OidIsValid(params->tablespaceOid);
pg_rusage_init();
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-01-20 18:54, Alvaro Herrera wrote: On 2021-Jan-20, Alvaro Herrera wrote: On 2021-Jan-20, Michael Paquier wrote: > +/* > + * This is mostly duplicating ATExecSetTableSpaceNoStorage, > + * which should maybe be factored out to a library function. > + */ > Wouldn't it be better to do first the refactoring of 0002 and then > 0001 so as REINDEX can use the new routine, instead of putting that > into a comment? I think merging 0001 and 0002 into a single commit is a reasonable approach. ... except it doesn't make a lot of sense to have set_rel_tablespace in either indexcmds.c or index.c. I think tablecmds.c is a better place for it. (I would have thought catalog/storage.c, but that one's not the right abstraction level it seems.) I did a refactoring of ATExecSetTableSpaceNoStorage() in the 0001. New function SetRelTablesapce() is placed into the tablecmds.c. Following 0002 gets use of it. Is it close to what you and Michael suggested? But surely ATExecSetTableSpaceNoStorage should be using this new routine. (I first thought 0002 was doing that, since that commit is calling itself a "refactoring", but now that I look closer, it's not.) Yeah, this 'refactoring' was initially referring to refactoring of what Justin added to one of the previous 0001. And it was meant to be merged with 0001, once agreed, but we got distracted by other stuff. I have not yet addressed Michael's concerns regarding reindex of partitions. I am going to look closer on it tomorrow. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-Jan-20, Alvaro Herrera wrote: > On 2021-Jan-20, Michael Paquier wrote: > > > +/* > > + * This is mostly duplicating ATExecSetTableSpaceNoStorage, > > + * which should maybe be factored out to a library function. > > + */ > > Wouldn't it be better to do first the refactoring of 0002 and then > > 0001 so as REINDEX can use the new routine, instead of putting that > > into a comment? > > I think merging 0001 and 0002 into a single commit is a reasonable > approach. ... except it doesn't make a lot of sense to have set_rel_tablespace in either indexcmds.c or index.c. I think tablecmds.c is a better place for it. (I would have thought catalog/storage.c, but that one's not the right abstraction level it seems.) But surely ATExecSetTableSpaceNoStorage should be using this new routine. (I first thought 0002 was doing that, since that commit is calling itself a "refactoring", but now that I look closer, it's not.) -- Álvaro Herrera39°49'30"S 73°17'W "On the other flipper, one wrong move and we're Fatal Exceptions" (T.U.X.: Term Unit X - http://www.thelinuxreview.com/TUX/)
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-Jan-20, Michael Paquier wrote: > +/* > + * This is mostly duplicating ATExecSetTableSpaceNoStorage, > + * which should maybe be factored out to a library function. > + */ > Wouldn't it be better to do first the refactoring of 0002 and then > 0001 so as REINDEX can use the new routine, instead of putting that > into a comment? I think merging 0001 and 0002 into a single commit is a reasonable approach. I don't oppose an initial refactoring commit if you want to do that, but it doesn't seem necessary. -- Álvaro Herrera39°49'30"S 73°17'W
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Mon, Jan 18, 2021 at 02:37:57AM -0600, Justin Pryzby wrote:
> Attached. I will re-review these myself tomorrow.
I have begun looking at 0001 and 0002...
+/*
+ * This is mostly duplicating ATExecSetTableSpaceNoStorage,
+ * which should maybe be factored out to a library function.
+ */
Wouldn't it be better to do first the refactoring of 0002 and then
0001 so as REINDEX can use the new routine, instead of putting that
into a comment?
+ This specifies that indexes will be rebuilt on a new tablespace.
+ Cannot be used with "mapped" relations. If SCHEMA,
+ DATABASE or SYSTEM is specified,
then
+ all unsuitable relations will be skipped and a single
WARNING
+ will be generated.
What is an unsuitable relation? How can the end user know that?
This is missing ACL checks when moving the index into a new location,
so this requires some pg_tablespace_aclcheck() calls, and the other
patches share the same issue.
+ else if (partkind == RELKIND_PARTITIONED_TABLE)
+ {
+ Relation rel = table_open(partoid, ShareLock);
+ List*indexIds = RelationGetIndexList(rel);
+ ListCell *lc;
+
+ table_close(rel, NoLock);
+ foreach (lc, indexIds)
+ {
+ Oid indexid = lfirst_oid(lc);
+ (void) set_rel_tablespace(indexid, params->tablespaceOid);
+ }
+ }
This is really a good question. ReindexPartitions() would trigger one
transaction per leaf to work on. Changing the tablespace of the
partitioned table(s) before doing any work has the advantage to tell
any new partition to use the new tablespace. Now, I see a struggling
point here: what should we do if the processing fails in the middle of
the move, leaving a portion of the leaves in the previous tablespace?
On a follow-up reindex with the same command, should the command force
a reindex even on the partitions that have been moved? Or could there
be a point in skipping the partitions that are already on the new
tablespace and only process the ones on the previous tablespace? It
seems to me that the first scenario makes the most sense as currently
a REINDEX works on all the relations defined, though there could be
use cases for the second case. This should be documented, I think.
There are no tests for partitioned tables, aka we'd want to make sure
that the new partitioned index is on the correct tablespace, as well
as all its leaves. It may be better to have at least two levels of
partitioned tables, as well as a partitioned table with no leaves in
the cases dealt with.
+*
+* Even if a table's indexes were moved to a new tablespace, the index
+* on its toast table is not normally moved.
*/
Still, REINDEX (TABLESPACE) TABLE should move all of them to be
consistent with ALTER TABLE SET TABLESPACE, but that's not the case
with this code, no? This requires proper test coverage, but there is
nothing of the kind in this patch.
--
Michael
signature.asc
Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Hi,
For 0001-Allow-REINDEX-to-change-tablespace.patch :
+ * InvalidOid, use the tablespace in-use instead.
'in-use' seems a bit redundant in the sentence.
How about : InvalidOid, use the tablespace of the index instead.
Cheers
On Mon, Jan 18, 2021 at 12:38 AM Justin Pryzby wrote:
> On Mon, Jan 18, 2021 at 02:18:44PM +0900, Michael Paquier wrote:
> > On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
> > > It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and
> ReindexParams
> > > In my v31 patch, I moved ReindexOptions to a private structure in
> indexcmds.c,
> > > with an "int options" bitmask which is passed to reindex_index() et
> al. Your
> > > patch keeps/puts ReindexOptions index.h, so it also applies to
> reindex_index,
> > > which I think is good.
> >
> > a3dc926 is an equivalent of 0001~0003 merged together. 0008 had
> > better be submitted into a separate thread if there is value to it.
> > 0004~0007 are the pieces remaining. Could it be possible to rebase
> > things on HEAD and put the tablespace bits into the structures
> > {Vacuum,Reindex,Cluster}Params?
>
> Attached. I will re-review these myself tomorrow.
>
> --
> Justin
>
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Mon, Jan 18, 2021 at 02:18:44PM +0900, Michael Paquier wrote:
> On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
> > It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and
> > ReindexParams
> > In my v31 patch, I moved ReindexOptions to a private structure in
> > indexcmds.c,
> > with an "int options" bitmask which is passed to reindex_index() et al.
> > Your
> > patch keeps/puts ReindexOptions index.h, so it also applies to
> > reindex_index,
> > which I think is good.
>
> a3dc926 is an equivalent of 0001~0003 merged together. 0008 had
> better be submitted into a separate thread if there is value to it.
> 0004~0007 are the pieces remaining. Could it be possible to rebase
> things on HEAD and put the tablespace bits into the structures
> {Vacuum,Reindex,Cluster}Params?
Attached. I will re-review these myself tomorrow.
--
Justin
>From 42991c90afda1b2a9fe4095418a87b5ead4b23d9 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Mon, 23 Mar 2020 21:10:29 +0300
Subject: [PATCH 1/4] Allow REINDEX to change tablespace
REINDEX already does full relation rewrite, this patch adds a
possibility to specify a new tablespace where new relfilenode
will be created.
---
doc/src/sgml/ref/reindex.sgml | 22 +
src/backend/catalog/index.c | 93 ++-
src/backend/commands/indexcmds.c | 103 +-
src/bin/psql/tab-complete.c | 4 +-
src/include/catalog/index.h | 2 +
src/test/regress/input/tablespace.source | 53 +++
src/test/regress/output/tablespace.source | 102 +
7 files changed, 374 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 627b36300c..4f84060c4d 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -27,6 +27,7 @@ REINDEX [ ( option [, ...] ) ] { IN
CONCURRENTLY [ boolean ]
VERBOSE [ boolean ]
+TABLESPACE new_tablespace
@@ -187,6 +188,19 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+TABLESPACE
+
+
+ This specifies that indexes will be rebuilt on a new tablespace.
+ Cannot be used with "mapped" relations. If SCHEMA,
+ DATABASE or SYSTEM is specified, then
+ all unsuitable relations will be skipped and a single WARNING
+ will be generated.
+
+
+
+
VERBOSE
@@ -210,6 +224,14 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+new_tablespace
+
+
+ The tablespace where indexes will be rebuilt.
+
+
+
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index b8cd35e995..9aa9fdf291 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -57,6 +57,7 @@
#include "commands/event_trigger.h"
#include "commands/progress.h"
#include "commands/tablecmds.h"
+#include "commands/tablespace.h"
#include "commands/trigger.h"
#include "executor/executor.h"
#include "miscadmin.h"
@@ -1394,9 +1395,13 @@ index_update_collation_versions(Oid relid, Oid coll)
* Create concurrently an index based on the definition of the one provided by
* caller. The index is inserted into catalogs and needs to be built later
* on. This is called during concurrent reindex processing.
+ *
+ * "tablespaceOid" is the new tablespace to use for this index. If
+ * InvalidOid, use the tablespace in-use instead.
*/
Oid
-index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, const char *newName)
+index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
+ Oid tablespaceOid, const char *newName)
{
Relation indexRelation;
IndexInfo *oldInfo,
@@ -1526,7 +1531,8 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, const char
newInfo,
indexColNames,
indexRelation->rd_rel->relam,
- indexRelation->rd_rel->reltablespace,
+ OidIsValid(tablespaceOid) ?
+tablespaceOid : indexRelation->rd_rel->reltablespace,
indexRelation->rd_indcollation,
indclass->values,
indcoloptions->values,
@@ -3591,6 +3597,8 @@ IndexGetRelation(Oid indexId, bool missing_ok)
/*
* reindex_index - This routine is used to recreate a single index
+ *
+ * See comments of reindex_relation() for details about "tablespaceOid".
*/
void
reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
@@ -3603,6 +3611,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
volatile bool skipped_constraint = false;
PGRUsage ru0;
bool progress = ((params->options & REINDEXOPT_REPORT_PROGRESS) != 0);
+ bool set_tablespace = OidIsValid(params->tablespaceOid);
pg_rusage_init();
@@ -3654,6 +3663,35 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
get_namespace_name(RelationGetNamespace(iRel)),
RelationGetRelationName(iRel));
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
> It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
> In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
> with an "int options" bitmask which is passed to reindex_index() et al. Your
> patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
> which I think is good.
a3dc926 is an equivalent of 0001~0003 merged together. 0008 had
better be submitted into a separate thread if there is value to it.
0004~0007 are the pieces remaining. Could it be possible to rebase
things on HEAD and put the tablespace bits into the structures
{Vacuum,Reindex,Cluster}Params?
--
Michael
signature.asc
Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Thu, Jan 14, 2021 at 02:18:45PM +0900, Michael Paquier wrote: > Indeed. Let's first wait a couple of days and see if others have any > comments or objections about this v6. Hearing nothing, I have looked at that again this morning and applied v6 after a reindent and some adjustments in the comments. -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Jan 13, 2021 at 04:39:40PM +0300, Alexey Kondratov wrote: > + bits32 options;/* bitmask of > CLUSTEROPT_* */ > > This should say '/* bitmask of CLUOPT_* */', I guess, since there are only > CLUOPT's defined. Otherwise, everything looks as per discussed upthread. Indeed. Let's first wait a couple of days and see if others have any comments or objections about this v6. > By the way, something went wrong with the last email subject, so I have > changed it back to the original in this response. I also attached your patch > (with only this CLUOPT_* correction) to keep it in the thread for sure. > Although, postgresql.org's web archive is clever enough to link your email > to the same thread even with different subject. Oops. Not sure what went wrong here. Thanks. -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2021-01-13 14:34, Michael Paquier wrote:
On Wed, Jan 13, 2021 at 05:22:49PM +0900, Michael Paquier wrote:
Yeah, that makes sense. I'll send an updated patch based on that.
And here you go as per the attached. I don't think that there was
anything remaining on my radar. This version still needs to be
indented properly though.
Thoughts?
Thanks.
+ bits32 options;/* bitmask of
CLUSTEROPT_* */
This should say '/* bitmask of CLUOPT_* */', I guess, since there are
only CLUOPT's defined. Otherwise, everything looks as per discussed
upthread.
By the way, something went wrong with the last email subject, so I have
changed it back to the original in this response. I also attached your
patch (with only this CLUOPT_* correction) to keep it in the thread for
sure. Although, postgresql.org's web archive is clever enough to link
your email to the same thread even with different subject.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Companydiff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 9904a76387..43cfdeaa6b 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -30,13 +30,16 @@ typedef enum
} IndexStateFlagsAction;
/* options for REINDEX */
-typedef enum ReindexOption
+typedef struct ReindexParams
{
- REINDEXOPT_VERBOSE = 1 << 0, /* print progress info */
- REINDEXOPT_REPORT_PROGRESS = 1 << 1, /* report pgstat progress */
- REINDEXOPT_MISSING_OK = 1 << 2, /* skip missing relations */
- REINDEXOPT_CONCURRENTLY = 1 << 3 /* concurrent mode */
-} ReindexOption;
+ bits32 options; /* bitmask of REINDEXOPT_* */
+} ReindexParams;
+
+/* flag bits for ReindexParams->flags */
+#define REINDEXOPT_VERBOSE 0x01 /* print progress info */
+#define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */
+#define REINDEXOPT_MISSING_OK 0x04 /* skip missing relations */
+#define REINDEXOPT_CONCURRENTLY 0x08 /* concurrent mode */
/* state info for validate_index bulkdelete callback */
typedef struct ValidateIndexState
@@ -146,7 +149,7 @@ extern void index_set_state_flags(Oid indexId, IndexStateFlagsAction action);
extern Oid IndexGetRelation(Oid indexId, bool missing_ok);
extern void reindex_index(Oid indexId, bool skip_constraint_checks,
- char relpersistence, int options);
+ char relpersistence, ReindexParams *params);
/* Flag bits for reindex_relation(): */
#define REINDEX_REL_PROCESS_TOAST 0x01
@@ -155,7 +158,7 @@ extern void reindex_index(Oid indexId, bool skip_constraint_checks,
#define REINDEX_REL_FORCE_INDEXES_UNLOGGED 0x08
#define REINDEX_REL_FORCE_INDEXES_PERMANENT 0x10
-extern bool reindex_relation(Oid relid, int flags, int options);
+extern bool reindex_relation(Oid relid, int flags, ReindexParams *params);
extern bool ReindexIsProcessingHeap(Oid heapOid);
extern bool ReindexIsProcessingIndex(Oid indexOid);
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 401a0827ae..1245d944dc 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -18,16 +18,17 @@
#include "storage/lock.h"
#include "utils/relcache.h"
-
/* options for CLUSTER */
-typedef enum ClusterOption
+#define CLUOPT_RECHECK 0x01 /* recheck relation state */
+#define CLUOPT_VERBOSE 0x02 /* print progress info */
+
+typedef struct ClusterParams
{
- CLUOPT_RECHECK = 1 << 0, /* recheck relation state */
- CLUOPT_VERBOSE = 1 << 1 /* print progress info */
-} ClusterOption;
+ bits32 options; /* bitmask of CLUOPT_* */
+} ClusterParams;
extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
-extern void cluster_rel(Oid tableOid, Oid indexOid, int options);
+extern void cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params);
extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
bool recheck, LOCKMODE lockmode);
extern void mark_index_clustered(Relation rel, Oid indexOid, bool is_internal);
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index e2d2a77ca4..91281d6f8e 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -14,6 +14,7 @@
#ifndef DEFREM_H
#define DEFREM_H
+#include "catalog/index.h"
#include "catalog/objectaddress.h"
#include "nodes/params.h"
#include "parser/parse_node.h"
@@ -34,11 +35,7 @@ extern ObjectAddress DefineIndex(Oid relationId,
bool check_not_in_use,
bool skip_build,
bool quiet);
-extern int ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt);
-extern void ReindexIndex(RangeVar *indexRelation, int options, bool isTopLevel);
-extern Oid ReindexTable(RangeVar *relation, int options, bool isTopLevel);
-extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
- int options);
+extern void ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel);
extern char
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Dec 23, 2020 at 07:30:35PM +0300, Alexey Kondratov wrote: > After eyeballing the patch I can add that we should alter this comment: > > int options;/* bitmask of VacuumOption */ > > as you are going to replace VacuumOption with VACOPT_* defs. So this should > say: > > /* bitmask of VACOPT_* */ Check. > > Also I have found naming to be a bit inconsistent: > * we have ReindexOptions, but VacuumParams > * and ReindexOptions->flags, but VacuumParams->options Check. As ReindexOptions and ClusterOptions are the new members of the family here, we could change them to use Params instead with "options" as bits32 internally. > And the last one, you have used bits32 for Cluster/ReindexOptions, but left > VacuumParams->options as int. Maybe we should also change it to bits32 for > consistency? Yeah, that makes sense. I'll send an updated patch based on that. -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Thu, Dec 24, 2020 at 10:50:34AM +0900, Michael Paquier wrote: > FWIW, it still makes the most sense to me to keep the options that are > extracted from the grammar or things that apply to all the > sub-routines of REINDEX to be tracked in a single structure, so this > should include only the REINDEXOPT_* set for now, with the tablespace > OID as of this thread, and also the reindex filtering options. > REINDEX_REL_* is in my opinion of a different family because they only > apply to reindex_relation(), and partially to reindex_index(), so they > are very localized. In short, anything in need of only > reindex_relation() has no need to know about the whole ReindexOption > business. I need more coffee here.. reindex_relation() knows about ReindexOptions. Still it would be weird to track REINDEX_REL_* at a global level as ExecReindex(), ReindexTable(), ReindexMultipleTables() and the like don't need to know about that. -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Dec 23, 2020 at 07:07:54PM -0600, Justin Pryzby wrote: > On Wed, Dec 23, 2020 at 09:14:18PM -0300, Alvaro Herrera wrote: >> On 2020-Dec-23, Justin Pryzby wrote: >>> This was getting ugly: >>> >>> extern void reindex_index(Oid indexId, bool skip_constraint_checks, >>> char relpersistence, int options, Oid >>> tablespaceOid) >> >> Is this what I suggested? No idea if this is what you suggested, but it would be annoying to have to change this routine's signature each time we need to pass down a new option. > No ; that was from an earlier revision of the patch adding the tablespace, > before Michael suggested a ReindexOptions struct, which subsumes 'options' and > 'tablespaceOid'. > > I see now that 'skip_constraint_checks' is from REINDEX_REL_CHECK_CONSTRAINTS. > It seems like that should be a REINDEXOPT_* flag, rather than REINDEX_REL_*, > so doesn't need to be a separate boolean. See also: 2d3320d3d. FWIW, it still makes the most sense to me to keep the options that are extracted from the grammar or things that apply to all the sub-routines of REINDEX to be tracked in a single structure, so this should include only the REINDEXOPT_* set for now, with the tablespace OID as of this thread, and also the reindex filtering options. REINDEX_REL_* is in my opinion of a different family because they only apply to reindex_relation(), and partially to reindex_index(), so they are very localized. In short, anything in need of only reindex_relation() has no need to know about the whole ReindexOption business. -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Dec 23, 2020 at 09:14:18PM -0300, Alvaro Herrera wrote: > On 2020-Dec-23, Justin Pryzby wrote: > > > This was getting ugly: > > > > extern void reindex_index(Oid indexId, bool skip_constraint_checks, > > char relpersistence, int options, Oid > > tablespaceOid)Z > > Is this what I suggested? No ; that was from an earlier revision of the patch adding the tablespace, before Michael suggested a ReindexOptions struct, which subsumes 'options' and 'tablespaceOid'. I see now that 'skip_constraint_checks' is from REINDEX_REL_CHECK_CONSTRAINTS. It seems liek that should be a REINDEXOPT_* flag, rather than REINDEX_REL_*, so doesn't need to be a separate boolean. See also: 2d3320d3d. -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-Dec-23, Justin Pryzby wrote: > This was getting ugly: > > extern void reindex_index(Oid indexId, bool skip_constraint_checks, > char relpersistence, int options, Oid > tablespaceOid)Z Is this what I suggested?
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Dec 23, 2020 at 07:22:05PM -0300, Alvaro Herrera wrote:
> Also: it seems a bit weird to me to put the flags inside the options
> struct. I would keep them separate -- so initially the options struct
> would only have the tablespace OID, on API cleanliness grounds:
I don't see why they'd be separate or why it's cleaner ?
If the user says REINDEX (CONCURRENTLY, VERBOSE, TABLESPACE ts) , why would we
pass around the boolean flags separately from the other options ?
> struct ReindexOptions
> {
> tablepaceOidoid;
> };
> extern bool
> reindex_relation(Oid relid, bits32 flags, ReindexOptions *options);
> But also, are we really envisioning that these routines would have all
> that many additional options? Maybe it is sufficient to do just
>
> extern bool
> reindex_relation(Oid relid, bits32 flags, tablespaceOid Oid);
That's what we did initially, and Michael suggested to put it into a struct.
Which makes the tablespace patches cleaner for each of REINDEX, CLUSTER,
VACUUM, since it doesn't require modifying the signature of 5-10 functions.
And future patches get to reap the benefit.
These are intended to be like VacuumParams. Consider that ClusterOptions is
proposed to get not just a tablespaceOid but also an idxtablespaceOid.
This was getting ugly:
extern void reindex_index(Oid indexId, bool skip_constraint_checks,
char relpersistence, int options, Oid tablespaceOid);
--
Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-Dec-23, Michael Paquier wrote:
> bool
> -reindex_relation(Oid relid, int flags, int options)
> +reindex_relation(Oid relid, int flags, ReindexOptions *options)
> {
> Relationrel;
> Oid toast_relid;
Wait a minute. reindex_relation has 'flags' and *also* 'options' with
an embedded 'flags' member? Surely that's not right. I see that they
carry orthogonal sets of options ... but why aren't they a single
bitmask instead of two separate ones? This looks weird and confusing.
Also: it seems a bit weird to me to put the flags inside the options
struct. I would keep them separate -- so initially the options struct
would only have the tablespace OID, on API cleanliness grounds:
struct ReindexOptions
{
tablepaceOidoid;
};
extern bool
reindex_relation(Oid relid, bits32 flags, ReindexOptions *options);
I guess you could argue that it's more performance to set up only two
arguments to the function call instead of three .. but I doubt that's
measurable for anything in DDL-land.
But also, are we really envisioning that these routines would have all
that many additional options? Maybe it is sufficient to do just
extern bool
reindex_relation(Oid relid, bits32 flags, tablespaceOid Oid);
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-12-23 10:38, Michael Paquier wrote: On Tue, Dec 22, 2020 at 03:15:37PM -0600, Justin Pryzby wrote: Now, I really think utility.c ought to pass in a pointer to a local ReindexOptions variable to avoid all the memory context, which is unnecessary and prone to error. Yeah, it sounds right to me to just bite the bullet and do this refactoring, limiting the manipulations of the options as much as possible across contexts. So +1 from me to merge 0001 and 0002 together. I have adjusted a couple of comments and simplified a bit more the code in utility.c. I think that this is commitable, but let's wait more than a couple of days for Alvaro and Peter first. This is a period of vacations for a lot of people, and there is no point to apply something that would need more work at the end. Using hexas for the flags with bitmasks is the right conclusion IMO, but we are not alone. After eyeballing the patch I can add that we should alter this comment: int options;/* bitmask of VacuumOption */ as you are going to replace VacuumOption with VACOPT_* defs. So this should say: /* bitmask of VACOPT_* */ Also I have found naming to be a bit inconsistent: * we have ReindexOptions, but VacuumParams * and ReindexOptions->flags, but VacuumParams->options And the last one, you have used bits32 for Cluster/ReindexOptions, but left VacuumParams->options as int. Maybe we should also change it to bits32 for consistency? Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-12-23 08:22, Justin Pryzby wrote: On Tue, Dec 22, 2020 at 03:22:19PM -0800, Zhihong Yu wrote: Justin: For reindex_index() : + if (options->tablespaceOid == MyDatabaseTableSpace) + options->tablespaceOid = InvalidOid; ... + oldTablespaceOid = iRel->rd_rel->reltablespace; + if (set_tablespace && + (options->tablespaceOid != oldTablespaceOid || + (options->tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid I wonder why the options->tablespaceOid == MyDatabaseTableSpace clause appears again in the second if statement. Since the first if statement would assign InvalidOid to options->tablespaceOid when the first if condition is satisfied. Good question. Alexey mentioned on Sept 23 that he added the first stanza. to avoid storing the DB's tablespace OID (rather than InvalidOid). I think the 2nd half of the "or" is unnecessary since that was added setting to options->tablespaceOid = InvalidOid. If requesting to move to the DB's default tablespace, it'll now hit the first part of the OR: + (options->tablespaceOid != oldTablespaceOid || Without the first stanza setting, it would've hit the 2nd condition: + (options->tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid which means: "user requested to move a table to the DB's default tblspace, and it was previously on a nondefault space". So I think we can drop the 2nd half of the OR. Thanks for noticing. Yes, I have not noticed that we would have already assigned tablespaceOid to InvalidOid in this case. Back to the v7 we were doing this assignment a bit later, so this could make sense, but now it seems to be redundant. For some reason I have mixed these refactorings separated by a dozen of versions... Thanks -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Dec 22, 2020 at 03:15:37PM -0600, Justin Pryzby wrote:
> Now, I really think utility.c ought to pass in a pointer to a local
> ReindexOptions variable to avoid all the memory context, which is unnecessary
> and prone to error.
Yeah, it sounds right to me to just bite the bullet and do this
refactoring, limiting the manipulations of the options as much as
possible across contexts. So +1 from me to merge 0001 and 0002
together.
I have adjusted a couple of comments and simplified a bit more the
code in utility.c. I think that this is commitable, but let's wait
more than a couple of days for Alvaro and Peter first. This is a
period of vacations for a lot of people, and there is no point to
apply something that would need more work at the end. Using hexas for
the flags with bitmasks is the right conclusion IMO, but we are not
alone.
> ExecReindex() will set options.tablespaceOid, not a pointer. Like
> this.
OK. Good to know, I have not looked at this part of the patch yet.
--
Michael
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index c041628049..89394b648e 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -30,13 +30,16 @@ typedef enum
} IndexStateFlagsAction;
/* options for REINDEX */
-typedef enum ReindexOption
+typedef struct ReindexOptions
{
- REINDEXOPT_VERBOSE = 1 << 0, /* print progress info */
- REINDEXOPT_REPORT_PROGRESS = 1 << 1, /* report pgstat progress */
- REINDEXOPT_MISSING_OK = 1 << 2, /* skip missing relations */
- REINDEXOPT_CONCURRENTLY = 1 << 3 /* concurrent mode */
-} ReindexOption;
+ bits32 flags; /* bitmask of REINDEXOPT_* */
+} ReindexOptions;
+
+/* flag bits for ReindexOptions->flags */
+#define REINDEXOPT_VERBOSE 0x01 /* print progress info */
+#define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */
+#define REINDEXOPT_MISSING_OK 0x04 /* skip missing relations */
+#define REINDEXOPT_CONCURRENTLY 0x08 /* concurrent mode */
/* state info for validate_index bulkdelete callback */
typedef struct ValidateIndexState
@@ -146,7 +149,7 @@ extern void index_set_state_flags(Oid indexId, IndexStateFlagsAction action);
extern Oid IndexGetRelation(Oid indexId, bool missing_ok);
extern void reindex_index(Oid indexId, bool skip_constraint_checks,
- char relpersistence, int options);
+ char relpersistence, ReindexOptions *options);
/* Flag bits for reindex_relation(): */
#define REINDEX_REL_PROCESS_TOAST 0x01
@@ -155,7 +158,7 @@ extern void reindex_index(Oid indexId, bool skip_constraint_checks,
#define REINDEX_REL_FORCE_INDEXES_UNLOGGED 0x08
#define REINDEX_REL_FORCE_INDEXES_PERMANENT 0x10
-extern bool reindex_relation(Oid relid, int flags, int options);
+extern bool reindex_relation(Oid relid, int flags, ReindexOptions *options);
extern bool ReindexIsProcessingHeap(Oid heapOid);
extern bool ReindexIsProcessingIndex(Oid indexOid);
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 7cfb37c9b2..c66629cf73 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -18,16 +18,17 @@
#include "storage/lock.h"
#include "utils/relcache.h"
-
/* options for CLUSTER */
-typedef enum ClusterOption
+#define CLUOPT_RECHECK 0x01 /* recheck relation state */
+#define CLUOPT_VERBOSE 0x02 /* print progress info */
+
+typedef struct ClusterOptions
{
- CLUOPT_RECHECK = 1 << 0, /* recheck relation state */
- CLUOPT_VERBOSE = 1 << 1 /* print progress info */
-} ClusterOption;
+ bits32 flags; /* bitmask of CLUSTEROPT_* */
+} ClusterOptions;
extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
-extern void cluster_rel(Oid tableOid, Oid indexOid, int options);
+extern void cluster_rel(Oid tableOid, Oid indexOid, ClusterOptions *options);
extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
bool recheck, LOCKMODE lockmode);
extern void mark_index_clustered(Relation rel, Oid indexOid, bool is_internal);
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 1133ae1143..d4ea57e757 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -14,6 +14,7 @@
#ifndef DEFREM_H
#define DEFREM_H
+#include "catalog/index.h"
#include "catalog/objectaddress.h"
#include "nodes/params.h"
#include "parser/parse_node.h"
@@ -34,11 +35,7 @@ extern ObjectAddress DefineIndex(Oid relationId,
bool check_not_in_use,
bool skip_build,
bool quiet);
-extern int ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt);
-extern void ReindexIndex(RangeVar *indexRelation, int options, bool isTopLevel);
-extern Oid ReindexTable(RangeVar *relation, int options, bool isTopLevel);
-extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
- int options);
+extern void ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel);
extern char *makeObjectName(const char *name1,
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Dec 22, 2020 at 03:22:19PM -0800, Zhihong Yu wrote: > Justin: > For reindex_index() : > > + if (options->tablespaceOid == MyDatabaseTableSpace) > + options->tablespaceOid = InvalidOid; > ... > + oldTablespaceOid = iRel->rd_rel->reltablespace; > + if (set_tablespace && > + (options->tablespaceOid != oldTablespaceOid || > + (options->tablespaceOid == MyDatabaseTableSpace && > OidIsValid(oldTablespaceOid > > I wonder why the options->tablespaceOid == MyDatabaseTableSpace clause > appears again in the second if statement. > Since the first if statement would assign InvalidOid > to options->tablespaceOid when the first if condition is satisfied. Good question. Alexey mentioned on Sept 23 that he added the first stanza. to avoid storing the DB's tablespace OID (rather than InvalidOid). I think the 2nd half of the "or" is unnecessary since that was added setting to options->tablespaceOid = InvalidOid. If requesting to move to the DB's default tablespace, it'll now hit the first part of the OR: > + (options->tablespaceOid != oldTablespaceOid || Without the first stanza setting, it would've hit the 2nd condition: > + (options->tablespaceOid == MyDatabaseTableSpace && > OidIsValid(oldTablespaceOid which means: "user requested to move a table to the DB's default tblspace, and it was previously on a nondefault space". So I think we can drop the 2nd half of the OR. Thanks for noticing. -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Justin: For reindex_index() : + if (options->tablespaceOid == MyDatabaseTableSpace) + options->tablespaceOid = InvalidOid; ... + if (set_tablespace && + (options->tablespaceOid != oldTablespaceOid || + (options->tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid I wonder why the options->tablespaceOid == MyDatabaseTableSpace clause appears again in the second if statement. Since the first if statement would assign InvalidOid to options->tablespaceOid when the first if condition is satisfied. Cheers On Tue, Dec 22, 2020 at 1:15 PM Justin Pryzby wrote: > On Tue, Dec 22, 2020 at 06:57:41PM +0900, Michael Paquier wrote: > > On Tue, Dec 22, 2020 at 02:32:05AM -0600, Justin Pryzby wrote: > > > Also, this one is going to be subsumed by ExecReindex(), so the palloc > will go > > > away (otherwise I would ask to pass it in from the caller): > > > > Yeah, maybe. Still you need to be very careful if you have any > > allocated variables like a tablespace or a path which requires to be > > in the private context used by ReindexMultipleInternal() or even > > ReindexRelationConcurrently(), so I am not sure you can avoid that > > completely. For now, we could choose the option to still use a > > palloc(), and then save the options in the private contexts. Forgot > > that in the previous version actually. > > I can't see why this still uses memset instead of structure assignment. > > Now, I really think utility.c ought to pass in a pointer to a local > ReindexOptions variable to avoid all the memory context, which is > unnecessary > and prone to error. > > ExecReindex() will set options.tablesapceOid, not a pointer. Like this. > > I also changed the callback to be a ReindexOptions and not a pointer. > > -- > Justin >
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Dec 22, 2020 at 06:57:41PM +0900, Michael Paquier wrote:
> On Tue, Dec 22, 2020 at 02:32:05AM -0600, Justin Pryzby wrote:
> > Also, this one is going to be subsumed by ExecReindex(), so the palloc will
> > go
> > away (otherwise I would ask to pass it in from the caller):
>
> Yeah, maybe. Still you need to be very careful if you have any
> allocated variables like a tablespace or a path which requires to be
> in the private context used by ReindexMultipleInternal() or even
> ReindexRelationConcurrently(), so I am not sure you can avoid that
> completely. For now, we could choose the option to still use a
> palloc(), and then save the options in the private contexts. Forgot
> that in the previous version actually.
I can't see why this still uses memset instead of structure assignment.
Now, I really think utility.c ought to pass in a pointer to a local
ReindexOptions variable to avoid all the memory context, which is unnecessary
and prone to error.
ExecReindex() will set options.tablesapceOid, not a pointer. Like this.
I also changed the callback to be a ReindexOptions and not a pointer.
--
Justin
>From f12723a8e4ad550c009935fa5397d1d4a968c7bf Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 22 Dec 2020 18:57:41 +0900
Subject: [PATCH 1/3] refactor-utility-opts-michael-2.patch - hacked on by
Justin
---
src/backend/catalog/index.c | 19 ---
src/backend/commands/cluster.c | 19 ---
src/backend/commands/indexcmds.c | 97 +---
src/backend/commands/tablecmds.c | 3 +-
src/backend/commands/vacuum.c| 6 +-
src/backend/tcop/utility.c | 12 ++--
src/include/catalog/index.h | 19 ---
src/include/commands/cluster.h | 13 +++--
src/include/commands/defrem.h| 17 --
src/include/commands/vacuum.h| 20 +++
src/tools/pgindent/typedefs.list | 2 +
11 files changed, 124 insertions(+), 103 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 731610c701..d776c661d7 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3594,7 +3594,7 @@ IndexGetRelation(Oid indexId, bool missing_ok)
*/
void
reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
- int options)
+ ReindexOptions *options)
{
Relation iRel,
heapRelation;
@@ -3602,7 +3602,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
PGRUsage ru0;
- bool progress = (options & REINDEXOPT_REPORT_PROGRESS) != 0;
+ bool progress = ((options->flags & REINDEXOPT_REPORT_PROGRESS) != 0);
pg_rusage_init();
@@ -3611,12 +3611,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
* we only need to be sure no schema or data changes are going on.
*/
heapId = IndexGetRelation(indexId,
- (options & REINDEXOPT_MISSING_OK) != 0);
+ (options->flags & REINDEXOPT_MISSING_OK) != 0);
/* if relation is missing, leave */
if (!OidIsValid(heapId))
return;
- if ((options & REINDEXOPT_MISSING_OK) != 0)
+ if ((options->flags & REINDEXOPT_MISSING_OK) != 0)
heapRelation = try_table_open(heapId, ShareLock);
else
heapRelation = table_open(heapId, ShareLock);
@@ -3792,7 +3792,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
}
/* Log what we did */
- if (options & REINDEXOPT_VERBOSE)
+ if ((options->flags & REINDEXOPT_VERBOSE) != 0)
ereport(INFO,
(errmsg("index \"%s\" was reindexed",
get_rel_name(indexId)),
@@ -3846,7 +3846,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
* index rebuild.
*/
bool
-reindex_relation(Oid relid, int flags, int options)
+reindex_relation(Oid relid, int flags, ReindexOptions *options)
{
Relation rel;
Oid toast_relid;
@@ -3861,7 +3861,7 @@ reindex_relation(Oid relid, int flags, int options)
* to prevent schema and data changes in it. The lock level used here
* should match ReindexTable().
*/
- if ((options & REINDEXOPT_MISSING_OK) != 0)
+ if ((options->flags & REINDEXOPT_MISSING_OK) != 0)
rel = try_table_open(relid, ShareLock);
else
rel = table_open(relid, ShareLock);
@@ -3965,8 +3965,9 @@ reindex_relation(Oid relid, int flags, int options)
* Note that this should fail if the toast relation is missing, so
* reset REINDEXOPT_MISSING_OK.
*/
- result |= reindex_relation(toast_relid, flags,
- options & ~(REINDEXOPT_MISSING_OK));
+ ReindexOptions newoptions = *options;
+ newoptions.flags &= ~(REINDEXOPT_MISSING_OK);
+ result |= reindex_relation(toast_relid, flags, );
}
return result;
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index fd5a6eec86..32f5ae848f 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -103,7 +103,7 @@ void
cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
{
ListCell
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Dec 22, 2020 at 02:32:05AM -0600, Justin Pryzby wrote:
> Also, this one is going to be subsumed by ExecReindex(), so the palloc will go
> away (otherwise I would ask to pass it in from the caller):
Yeah, maybe. Still you need to be very careful if you have any
allocated variables like a tablespace or a path which requires to be
in the private context used by ReindexMultipleInternal() or even
ReindexRelationConcurrently(), so I am not sure you can avoid that
completely. For now, we could choose the option to still use a
palloc(), and then save the options in the private contexts. Forgot
that in the previous version actually.
--
Michael
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index c041628049..89394b648e 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -30,13 +30,16 @@ typedef enum
} IndexStateFlagsAction;
/* options for REINDEX */
-typedef enum ReindexOption
+typedef struct ReindexOptions
{
- REINDEXOPT_VERBOSE = 1 << 0, /* print progress info */
- REINDEXOPT_REPORT_PROGRESS = 1 << 1, /* report pgstat progress */
- REINDEXOPT_MISSING_OK = 1 << 2, /* skip missing relations */
- REINDEXOPT_CONCURRENTLY = 1 << 3 /* concurrent mode */
-} ReindexOption;
+ bits32 flags; /* bitmask of REINDEXOPT_* */
+} ReindexOptions;
+
+/* flag bits for ReindexOptions->flags */
+#define REINDEXOPT_VERBOSE 0x01 /* print progress info */
+#define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */
+#define REINDEXOPT_MISSING_OK 0x04 /* skip missing relations */
+#define REINDEXOPT_CONCURRENTLY 0x08 /* concurrent mode */
/* state info for validate_index bulkdelete callback */
typedef struct ValidateIndexState
@@ -146,7 +149,7 @@ extern void index_set_state_flags(Oid indexId, IndexStateFlagsAction action);
extern Oid IndexGetRelation(Oid indexId, bool missing_ok);
extern void reindex_index(Oid indexId, bool skip_constraint_checks,
- char relpersistence, int options);
+ char relpersistence, ReindexOptions *options);
/* Flag bits for reindex_relation(): */
#define REINDEX_REL_PROCESS_TOAST 0x01
@@ -155,7 +158,7 @@ extern void reindex_index(Oid indexId, bool skip_constraint_checks,
#define REINDEX_REL_FORCE_INDEXES_UNLOGGED 0x08
#define REINDEX_REL_FORCE_INDEXES_PERMANENT 0x10
-extern bool reindex_relation(Oid relid, int flags, int options);
+extern bool reindex_relation(Oid relid, int flags, ReindexOptions *options);
extern bool ReindexIsProcessingHeap(Oid heapOid);
extern bool ReindexIsProcessingIndex(Oid indexOid);
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 7cfb37c9b2..c66629cf73 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -18,16 +18,17 @@
#include "storage/lock.h"
#include "utils/relcache.h"
-
/* options for CLUSTER */
-typedef enum ClusterOption
+#define CLUOPT_RECHECK 0x01 /* recheck relation state */
+#define CLUOPT_VERBOSE 0x02 /* print progress info */
+
+typedef struct ClusterOptions
{
- CLUOPT_RECHECK = 1 << 0, /* recheck relation state */
- CLUOPT_VERBOSE = 1 << 1 /* print progress info */
-} ClusterOption;
+ bits32 flags; /* bitmask of CLUSTEROPT_* */
+} ClusterOptions;
extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
-extern void cluster_rel(Oid tableOid, Oid indexOid, int options);
+extern void cluster_rel(Oid tableOid, Oid indexOid, ClusterOptions *options);
extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
bool recheck, LOCKMODE lockmode);
extern void mark_index_clustered(Relation rel, Oid indexOid, bool is_internal);
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 1133ae1143..43d5480c20 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -14,6 +14,7 @@
#ifndef DEFREM_H
#define DEFREM_H
+#include "catalog/index.h"
#include "catalog/objectaddress.h"
#include "nodes/params.h"
#include "parser/parse_node.h"
@@ -34,11 +35,16 @@ extern ObjectAddress DefineIndex(Oid relationId,
bool check_not_in_use,
bool skip_build,
bool quiet);
-extern int ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt);
-extern void ReindexIndex(RangeVar *indexRelation, int options, bool isTopLevel);
-extern Oid ReindexTable(RangeVar *relation, int options, bool isTopLevel);
-extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
- int options);
+extern ReindexOptions *ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt);
+extern void ReindexIndex(RangeVar *indexRelation,
+ ReindexOptions *options,
+ bool isTopLevel);
+extern Oid ReindexTable(RangeVar *relation,
+ ReindexOptions *options,
+ bool isTopLevel);
+extern void ReindexMultipleTables(const char *objectName,
+ ReindexObjectType objectKind,
+ ReindexOptions *options);
extern char *makeObjectName(const char *name1,
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Dec 22, 2020 at 03:47:57PM +0900, Michael Paquier wrote:
> On Wed, Dec 16, 2020 at 10:01:11AM +0900, Michael Paquier wrote:
> > On Tue, Dec 15, 2020 at 09:45:17PM -0300, Alvaro Herrera wrote:
> > > I don't like this idea too much, because adding an option causes an ABI
> > > break. I don't think we commonly add options in backbranches, but it
> > > has happened. The bitmask is much easier to work with in that regard.
> >
> > ABI flexibility is a good point here. I did not consider this point
> > of view. Thanks!
>
> FWIW, I have taken a shot at this part of the patch, and finished with
> the attached. This uses bits32 for the bitmask options and an hex
> style for the bitmask params, while bundling all the flags into
> dedicated structures for all the options that can be extended for the
> tablespace case (or some filtering for REINDEX).
Seems fine, but why do you do memcpy() instead of a structure assignment ?
> @@ -3965,8 +3965,11 @@ reindex_relation(Oid relid, int flags, int options)
>* Note that this should fail if the toast relation is missing,
> so
>* reset REINDEXOPT_MISSING_OK.
>*/
> - result |= reindex_relation(toast_relid, flags,
> -options &
> ~(REINDEXOPT_MISSING_OK));
> + ReindexOptions newoptions;
> +
> + memcpy(, options, sizeof(ReindexOptions));
> + newoptions.flags &= ~(REINDEXOPT_MISSING_OK);
> + result |= reindex_relation(toast_relid, flags, );
Could be newoptions = *options;
Also, this one is going to be subsumed by ExecReindex(), so the palloc will go
away (otherwise I would ask to pass it in from the caller):
> +ReindexOptions *
> ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt)
> {
> ListCell *lc;
> - int options = 0;
> + ReindexOptions *options;
> boolconcurrently = false;
> boolverbose = false;
>
> + options = (ReindexOptions *) palloc0(sizeof(ReindexOptions));
> +
--
Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Dec 16, 2020 at 10:01:11AM +0900, Michael Paquier wrote:
> On Tue, Dec 15, 2020 at 09:45:17PM -0300, Alvaro Herrera wrote:
> > I don't like this idea too much, because adding an option causes an ABI
> > break. I don't think we commonly add options in backbranches, but it
> > has happened. The bitmask is much easier to work with in that regard.
>
> ABI flexibility is a good point here. I did not consider this point
> of view. Thanks!
FWIW, I have taken a shot at this part of the patch, and finished with
the attached. This uses bits32 for the bitmask options and an hex
style for the bitmask params, while bundling all the flags into
dedicated structures for all the options that can be extended for the
tablespace case (or some filtering for REINDEX).
--
Michael
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index c041628049..89394b648e 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -30,13 +30,16 @@ typedef enum
} IndexStateFlagsAction;
/* options for REINDEX */
-typedef enum ReindexOption
+typedef struct ReindexOptions
{
- REINDEXOPT_VERBOSE = 1 << 0, /* print progress info */
- REINDEXOPT_REPORT_PROGRESS = 1 << 1, /* report pgstat progress */
- REINDEXOPT_MISSING_OK = 1 << 2, /* skip missing relations */
- REINDEXOPT_CONCURRENTLY = 1 << 3 /* concurrent mode */
-} ReindexOption;
+ bits32 flags; /* bitmask of REINDEXOPT_* */
+} ReindexOptions;
+
+/* flag bits for ReindexOptions->flags */
+#define REINDEXOPT_VERBOSE 0x01 /* print progress info */
+#define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */
+#define REINDEXOPT_MISSING_OK 0x04 /* skip missing relations */
+#define REINDEXOPT_CONCURRENTLY 0x08 /* concurrent mode */
/* state info for validate_index bulkdelete callback */
typedef struct ValidateIndexState
@@ -146,7 +149,7 @@ extern void index_set_state_flags(Oid indexId, IndexStateFlagsAction action);
extern Oid IndexGetRelation(Oid indexId, bool missing_ok);
extern void reindex_index(Oid indexId, bool skip_constraint_checks,
- char relpersistence, int options);
+ char relpersistence, ReindexOptions *options);
/* Flag bits for reindex_relation(): */
#define REINDEX_REL_PROCESS_TOAST 0x01
@@ -155,7 +158,7 @@ extern void reindex_index(Oid indexId, bool skip_constraint_checks,
#define REINDEX_REL_FORCE_INDEXES_UNLOGGED 0x08
#define REINDEX_REL_FORCE_INDEXES_PERMANENT 0x10
-extern bool reindex_relation(Oid relid, int flags, int options);
+extern bool reindex_relation(Oid relid, int flags, ReindexOptions *options);
extern bool ReindexIsProcessingHeap(Oid heapOid);
extern bool ReindexIsProcessingIndex(Oid indexOid);
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 7cfb37c9b2..c66629cf73 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -18,16 +18,17 @@
#include "storage/lock.h"
#include "utils/relcache.h"
-
/* options for CLUSTER */
-typedef enum ClusterOption
+#define CLUOPT_RECHECK 0x01 /* recheck relation state */
+#define CLUOPT_VERBOSE 0x02 /* print progress info */
+
+typedef struct ClusterOptions
{
- CLUOPT_RECHECK = 1 << 0, /* recheck relation state */
- CLUOPT_VERBOSE = 1 << 1 /* print progress info */
-} ClusterOption;
+ bits32 flags; /* bitmask of CLUSTEROPT_* */
+} ClusterOptions;
extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
-extern void cluster_rel(Oid tableOid, Oid indexOid, int options);
+extern void cluster_rel(Oid tableOid, Oid indexOid, ClusterOptions *options);
extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
bool recheck, LOCKMODE lockmode);
extern void mark_index_clustered(Relation rel, Oid indexOid, bool is_internal);
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 1133ae1143..43d5480c20 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -14,6 +14,7 @@
#ifndef DEFREM_H
#define DEFREM_H
+#include "catalog/index.h"
#include "catalog/objectaddress.h"
#include "nodes/params.h"
#include "parser/parse_node.h"
@@ -34,11 +35,16 @@ extern ObjectAddress DefineIndex(Oid relationId,
bool check_not_in_use,
bool skip_build,
bool quiet);
-extern int ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt);
-extern void ReindexIndex(RangeVar *indexRelation, int options, bool isTopLevel);
-extern Oid ReindexTable(RangeVar *relation, int options, bool isTopLevel);
-extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
- int options);
+extern ReindexOptions *ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt);
+extern void ReindexIndex(RangeVar *indexRelation,
+ ReindexOptions *options,
+ bool isTopLevel);
+extern Oid ReindexTable(RangeVar *relation,
+ ReindexOptions *options,
+ bool isTopLevel);
+extern void ReindexMultipleTables(const char *objectName,
+
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Dec 15, 2020 at 09:45:17PM -0300, Alvaro Herrera wrote: > I don't like this idea too much, because adding an option causes an ABI > break. I don't think we commonly add options in backbranches, but it > has happened. The bitmask is much easier to work with in that regard. ABI flexibility is a good point here. I did not consider this point of view. Thanks! -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-Dec-12, Peter Eisentraut wrote: > On 2020-12-11 21:27, Alvaro Herrera wrote: > > By the way-- What did you think of the idea of explictly marking the > > types used for bitmasks using types bits32 and friends, instead of plain > > int, which is harder to spot? > > If we want to make it clearer, why not turn the thing into a struct, as in > the attached patch, and avoid the bit fiddling altogether. I don't like this idea too much, because adding an option causes an ABI break. I don't think we commonly add options in backbranches, but it has happened. The bitmask is much easier to work with in that regard.
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Mon, Dec 14, 2020 at 06:14:17PM -0600, Justin Pryzby wrote:
> On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
> > On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:
> > > On 2020-12-11 21:27, Alvaro Herrera wrote:
> > > > By the way-- What did you think of the idea of explictly marking the
> > > > types used for bitmasks using types bits32 and friends, instead of plain
> > > > int, which is harder to spot?
> > >
> > > If we want to make it clearer, why not turn the thing into a struct, as in
> > > the attached patch, and avoid the bit fiddling altogether.
> >
> > I like this.
> > It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and
> > ReindexParams
> > In my v31 patch, I moved ReindexOptions to a private structure in
> > indexcmds.c,
> > with an "int options" bitmask which is passed to reindex_index() et al.
> > Your
> > patch keeps/puts ReindexOptions index.h, so it also applies to
> > reindex_index,
> > which I think is good.
> >
> > So I've rebased this branch on your patch.
> >
> > Some thoughts:
> >
> > - what about removing the REINDEXOPT_* prefix ?
> > - You created local vars with initialization like "={}". But I thought it's
> >needed to include at least one struct member like "={false}", or else
> >they're not guaranteed to be zerod ?
> > - You passed the structure across function calls. The usual convention is
> > to
> >pass a pointer.
>
> I think maybe Michael missed this message (?)
> I had applied some changes on top of Peter's patch.
>
> I squished those commits now, and also handled ClusterOption and VacuumOption
> in the same style.
>
> Some more thoughts:
> - should the structures be named in plural ? "ReindexOptions" etc. Since
> they
>define *all* the options, not just a single bit.
> - For vacuum, do we even need a separate structure, or should the members be
>directly within VacuumParams ? It's a bit odd to write
>params.options.verbose. Especially since there's also ternary options
> which
>are directly within params.
> - Then, for cluster, I think it should be called ClusterParams, and
> eventually
>include the tablespaceOid, like what we're doing for Reindex.
>
> I am awaiting feedback on these before going further since I've done too much
> rebasing with these ideas going back and forth and back.
With Alexey's agreement, I propose something like this.
I've merged some commits and kept separate the ones which are more likely to be
disputed/amended. But it may be best to read the series as a single commit,
like "git diff origin.."
--
Justin
>From ee702a6f49392e84bb51ec432d0974c7369b Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Tue, 15 Dec 2020 17:55:21 -0600
Subject: [PATCH 1/4] Convert options to struct: Reindex/Cluster/Vacuum
Remove prefixes and lowercase structure member variables;
Rename to plural: Vacuum/ClusterOptions;
---
src/backend/access/heap/vacuumlazy.c | 8 +-
src/backend/catalog/index.c | 26 +++---
src/backend/commands/analyze.c | 15 ++--
src/backend/commands/cluster.c | 29 +++---
src/backend/commands/indexcmds.c | 100 ++---
src/backend/commands/tablecmds.c | 4 +-
src/backend/commands/vacuum.c| 128 +--
src/backend/postmaster/autovacuum.c | 14 +--
src/backend/tcop/utility.c | 10 +--
src/include/catalog/index.h | 16 ++--
src/include/commands/cluster.h | 10 +--
src/include/commands/defrem.h| 9 +-
src/include/commands/vacuum.h| 29 +++---
13 files changed, 198 insertions(+), 200 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 25f2d5df1b..6bfed2b2da 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -456,7 +456,7 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params,
starttime = GetCurrentTimestamp();
}
- if (params->options & VACOPT_VERBOSE)
+ if (params->options.verbose)
elevel = INFO;
else
elevel = DEBUG2;
@@ -484,7 +484,7 @@ heap_vacuum_rel(Relation onerel, VacuumParams *params,
xidFullScanLimit);
aggressive |= MultiXactIdPrecedesOrEquals(onerel->rd_rel->relminmxid,
mxactFullScanLimit);
- if (params->options & VACOPT_DISABLE_PAGE_SKIPPING)
+ if (params->options.disable_page_skipping)
aggressive = true;
vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats));
@@ -902,7 +902,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
* be replayed on any hot standby, where it can be disruptive.
*/
next_unskippable_block = 0;
- if ((params->options & VACOPT_DISABLE_PAGE_SKIPPING) == 0)
+ if (!params->options.disable_page_skipping)
{
while (next_unskippable_block < nblocks)
{
@@ -960,7 +960,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
{
/* Time to advance
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-12-15 03:14, Justin Pryzby wrote:
On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:
> On 2020-12-11 21:27, Alvaro Herrera wrote:
> > By the way-- What did you think of the idea of explictly marking the
> > types used for bitmasks using types bits32 and friends, instead of plain
> > int, which is harder to spot?
>
> If we want to make it clearer, why not turn the thing into a struct, as in
> the attached patch, and avoid the bit fiddling altogether.
I like this.
It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and
ReindexParams
In my v31 patch, I moved ReindexOptions to a private structure in
indexcmds.c,
with an "int options" bitmask which is passed to reindex_index() et
al. Your
patch keeps/puts ReindexOptions index.h, so it also applies to
reindex_index,
which I think is good.
So I've rebased this branch on your patch.
Some thoughts:
- what about removing the REINDEXOPT_* prefix ?
- You created local vars with initialization like "={}". But I
thought it's
needed to include at least one struct member like "={false}", or
else
they're not guaranteed to be zerod ?
- You passed the structure across function calls. The usual
convention is to
pass a pointer.
I think maybe Michael missed this message (?)
I had applied some changes on top of Peter's patch.
I squished those commits now, and also handled ClusterOption and
VacuumOption
in the same style.
Some more thoughts:
- should the structures be named in plural ? "ReindexOptions" etc.
Since they
define *all* the options, not just a single bit.
- For vacuum, do we even need a separate structure, or should the
members be
directly within VacuumParams ? It's a bit odd to write
params.options.verbose. Especially since there's also ternary
options which
are directly within params.
This is exactly what I have thought after looking on Peter's patch.
Writing 'params.options.some_option' looks just too verbose. I even
started moving all vacuum options into VacuumParams on my own and was in
the middle of the process when realized that there are some places that
do not fit well, like:
if (!vacuum_is_relation_owner(RelationGetRelid(onerel),
onerel->rd_rel,
params->options & VACOPT_ANALYZE))
Here we do not want to set option permanently, but rather to trigger
some additional code path in the vacuum_is_relation_owner(), IIUC. With
unified VacuumParams we should do:
bool opt_analyze = params->analyze;
...
params->analyze = true;
if (!vacuum_is_relation_owner(RelationGetRelid(onerel),
onerel->rd_rel, params))
...
params->analyze = opt_analyze;
to achieve the same, but it does not look good to me, or change the
whole interface. I have found at least one other place like that so far
--- vacuum_open_relation() in the analyze_rel().
Not sure how we could better cope with such logic.
- Then, for cluster, I think it should be called ClusterParams, and
eventually
include the tablespaceOid, like what we're doing for Reindex.
I am awaiting feedback on these before going further since I've done
too much
rebasing with these ideas going back and forth and back.
Yes, we have moved a bit from my original patch set in the thread with
all this refactoring. However, all the consequent patches are heavily
depend on this interface, so let us decide first on the proposed
refactoring.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
> On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:
> > On 2020-12-11 21:27, Alvaro Herrera wrote:
> > > By the way-- What did you think of the idea of explictly marking the
> > > types used for bitmasks using types bits32 and friends, instead of plain
> > > int, which is harder to spot?
> >
> > If we want to make it clearer, why not turn the thing into a struct, as in
> > the attached patch, and avoid the bit fiddling altogether.
>
> I like this.
> It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
> In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
> with an "int options" bitmask which is passed to reindex_index() et al. Your
> patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
> which I think is good.
>
> So I've rebased this branch on your patch.
>
> Some thoughts:
>
> - what about removing the REINDEXOPT_* prefix ?
> - You created local vars with initialization like "={}". But I thought it's
>needed to include at least one struct member like "={false}", or else
>they're not guaranteed to be zerod ?
> - You passed the structure across function calls. The usual convention is to
>pass a pointer.
I think maybe Michael missed this message (?)
I had applied some changes on top of Peter's patch.
I squished those commits now, and also handled ClusterOption and VacuumOption
in the same style.
Some more thoughts:
- should the structures be named in plural ? "ReindexOptions" etc. Since they
define *all* the options, not just a single bit.
- For vacuum, do we even need a separate structure, or should the members be
directly within VacuumParams ? It's a bit odd to write
params.options.verbose. Especially since there's also ternary options which
are directly within params.
- Then, for cluster, I think it should be called ClusterParams, and eventually
include the tablespaceOid, like what we're doing for Reindex.
I am awaiting feedback on these before going further since I've done too much
rebasing with these ideas going back and forth and back.
--
Justin
>From 34bf8abaca50d39cb9fd00b21752f931b05a62f3 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sat, 12 Dec 2020 09:17:55 +0100
Subject: [PATCH 1/4] Convert reindex options to struct
---
src/backend/catalog/index.c | 24
src/backend/commands/cluster.c | 3 +-
src/backend/commands/indexcmds.c | 96
src/backend/commands/tablecmds.c | 4 +-
src/backend/tcop/utility.c | 10 ++--
src/include/catalog/index.h | 16 +++---
src/include/commands/defrem.h| 9 +--
7 files changed, 83 insertions(+), 79 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 731610c701..da2f45b796 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3594,7 +3594,7 @@ IndexGetRelation(Oid indexId, bool missing_ok)
*/
void
reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
- int options)
+ ReindexOptions *options)
{
Relation iRel,
heapRelation;
@@ -3602,7 +3602,6 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
PGRUsage ru0;
- bool progress = (options & REINDEXOPT_REPORT_PROGRESS) != 0;
pg_rusage_init();
@@ -3611,12 +3610,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
* we only need to be sure no schema or data changes are going on.
*/
heapId = IndexGetRelation(indexId,
- (options & REINDEXOPT_MISSING_OK) != 0);
+ options->REINDEXOPT_MISSING_OK);
/* if relation is missing, leave */
if (!OidIsValid(heapId))
return;
- if ((options & REINDEXOPT_MISSING_OK) != 0)
+ if (options->REINDEXOPT_MISSING_OK)
heapRelation = try_table_open(heapId, ShareLock);
else
heapRelation = table_open(heapId, ShareLock);
@@ -3625,7 +3624,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
if (!heapRelation)
return;
- if (progress)
+ if (options->REINDEXOPT_REPORT_PROGRESS)
{
pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
heapId);
@@ -3641,7 +3640,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
*/
iRel = index_open(indexId, AccessExclusiveLock);
- if (progress)
+ if (options->REINDEXOPT_REPORT_PROGRESS)
pgstat_progress_update_param(PROGRESS_CREATEIDX_ACCESS_METHOD_OID,
iRel->rd_rel->relam);
@@ -3792,14 +3791,14 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
}
/* Log what we did */
- if (options & REINDEXOPT_VERBOSE)
+ if (options->REINDEXOPT_VERBOSE)
ereport(INFO,
(errmsg("index \"%s\" was reindexed",
get_rel_name(indexId)),
errdetail_internal("%s",
pg_rusage_show(;
- if
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
> On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:
>> On 2020-12-11 21:27, Alvaro Herrera wrote:
>>> By the way-- What did you think of the idea of explictly marking the
>>> types used for bitmasks using types bits32 and friends, instead of plain
>>> int, which is harder to spot?
>>
>> If we want to make it clearer, why not turn the thing into a struct, as in
>> the attached patch, and avoid the bit fiddling altogether.
- reindex_relation(OIDOldHeap, reindex_flags, 0);
+ reindex_relation(OIDOldHeap, reindex_flags, (ReindexOptions){});
This is not common style in the PG code. Usually we would do that
with memset(0) or similar.
+ bool REINDEXOPT_VERBOSE;/* print progress info */
+ bool REINDEXOPT_REPORT_PROGRESS;/* report pgstat progress */
+ bool REINDEXOPT_MISSING_OK; /* skip missing relations */
+ bool REINDEXOPT_CONCURRENTLY; /* concurrent mode */
+} ReindexOptions
Neither is this one to use upper-case characters for variable names.
Now, we will need a ReindexOptions in the long-term to store all those
options and there would be much more coming that booleans here (this
thread talks about tablespaces, there is another thread about
collation filtering). Between using bits32 with some hex flags or
just a set of booleans within a structure, I would choose the former
as a matter of habit but yours has the advantage to make debugging a
no-brainer, which is good. For any approach taken, it seems to me
that the same style should be applied to ClusterOption and
Vacuum{Option,Params}.
--
Michael
signature.asc
Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
> On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:
> > On 2020-12-11 21:27, Alvaro Herrera wrote:
> > > By the way-- What did you think of the idea of explictly marking the
> > > types used for bitmasks using types bits32 and friends, instead of plain
> > > int, which is harder to spot?
> >
> > If we want to make it clearer, why not turn the thing into a struct, as in
> > the attached patch, and avoid the bit fiddling altogether.
>
> I like this.
> It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
> In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
> with an "int options" bitmask which is passed to reindex_index() et al. Your
> patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
> which I think is good.
>
> So I've rebased this branch on your patch.
Also, the cfbot's windows VS compilation failed due to "compound literal",
which I don't think is used anywhere else.
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.120108
src/backend/commands/cluster.c(1580): warning C4133: 'return' : incompatible
types - from 'List *' to 'int *' [C:\projects\postgresql\postgres.vcxproj]
"C:\projects\postgresql\pgsql.sln" (default target) (1) ->
"C:\projects\postgresql\cyrillic_and_mic.vcxproj" (default target) (5) ->
"C:\projects\postgresql\postgres.vcxproj" (default target) (6) ->
(ClCompile target) ->
src/backend/commands/cluster.c(1415): error C2059: syntax error : '}'
[C:\projects\postgresql\postgres.vcxproj]
src/backend/commands/cluster.c(1534): error C2143: syntax error : missing '{'
before '*' [C:\projects\postgresql\postgres.vcxproj]
src/backend/commands/cluster.c(1536): error C2371: 'get_tables_to_cluster' :
redefinition; different basic types [C:\projects\postgresql\postgres.vcxproj]
src/backend/commands/indexcmds.c(2462): error C2059: syntax error : '}'
[C:\projects\postgresql\postgres.vcxproj]
src/backend/commands/tablecmds.c(1894): error C2059: syntax error : '}'
[C:\projects\postgresql\postgres.vcxproj]
My fix! patch resolves that.
--
Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-12-11 21:27, Alvaro Herrera wrote:
By the way-- What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?
If we want to make it clearer, why not turn the thing into a struct, as
in the attached patch, and avoid the bit fiddling altogether.
From d3125b3bfb8a3dc23e38f38bcf850ca6fc36f492 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sat, 12 Dec 2020 09:17:55 +0100
Subject: [PATCH] Convert reindex options to struct
---
src/backend/catalog/index.c | 19 ---
src/backend/commands/cluster.c | 2 +-
src/backend/commands/indexcmds.c | 94
src/backend/commands/tablecmds.c | 2 +-
src/backend/tcop/utility.c | 4 +-
src/include/catalog/index.h | 16 +++---
src/include/commands/defrem.h| 9 +--
7 files changed, 74 insertions(+), 72 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 731610c701..06342fddf1 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3594,7 +3594,7 @@ IndexGetRelation(Oid indexId, bool missing_ok)
*/
void
reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
- int options)
+ ReindexOptions options)
{
RelationiRel,
heapRelation;
@@ -3602,7 +3602,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks,
char persistence,
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
PGRUsageru0;
- boolprogress = (options & REINDEXOPT_REPORT_PROGRESS) != 0;
+ boolprogress = options.REINDEXOPT_REPORT_PROGRESS;
pg_rusage_init();
@@ -3611,12 +3611,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks,
char persistence,
* we only need to be sure no schema or data changes are going on.
*/
heapId = IndexGetRelation(indexId,
- (options &
REINDEXOPT_MISSING_OK) != 0);
+
options.REINDEXOPT_MISSING_OK);
/* if relation is missing, leave */
if (!OidIsValid(heapId))
return;
- if ((options & REINDEXOPT_MISSING_OK) != 0)
+ if (options.REINDEXOPT_MISSING_OK)
heapRelation = try_table_open(heapId, ShareLock);
else
heapRelation = table_open(heapId, ShareLock);
@@ -3792,7 +3792,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks,
char persistence,
}
/* Log what we did */
- if (options & REINDEXOPT_VERBOSE)
+ if (options.REINDEXOPT_VERBOSE)
ereport(INFO,
(errmsg("index \"%s\" was reindexed",
get_rel_name(indexId)),
@@ -3846,7 +3846,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks,
char persistence,
* index rebuild.
*/
bool
-reindex_relation(Oid relid, int flags, int options)
+reindex_relation(Oid relid, int flags, ReindexOptions options)
{
Relationrel;
Oid toast_relid;
@@ -3861,7 +3861,7 @@ reindex_relation(Oid relid, int flags, int options)
* to prevent schema and data changes in it. The lock level used here
* should match ReindexTable().
*/
- if ((options & REINDEXOPT_MISSING_OK) != 0)
+ if (options.REINDEXOPT_MISSING_OK)
rel = try_table_open(relid, ShareLock);
else
rel = table_open(relid, ShareLock);
@@ -3965,8 +3965,9 @@ reindex_relation(Oid relid, int flags, int options)
* Note that this should fail if the toast relation is missing,
so
* reset REINDEXOPT_MISSING_OK.
*/
- result |= reindex_relation(toast_relid, flags,
- options &
~(REINDEXOPT_MISSING_OK));
+ ReindexOptions newoptions = options;
+ newoptions.REINDEXOPT_MISSING_OK = false;
+ result |= reindex_relation(toast_relid, flags, newoptions);
}
return result;
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index fd5a6eec86..b0aa3536d1 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1412,7 +1412,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
PROGRESS_CLUSTER_PHASE_REBUILD_INDEX);
- reindex_relation(OIDOldHeap, reindex_flags, 0);
+ reindex_relation(OIDOldHeap, reindex_flags, (ReindexOptions){});
/* Report that we are now doing clean up */
pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Fri, Dec 11, 2020 at 05:27:03PM -0300, Alvaro Herrera wrote: > By the way-- What did you think of the idea of explictly marking the > types used for bitmasks using types bits32 and friends, instead of plain > int, which is harder to spot? Right, we could just do that while the area is changed. It is worth noting that all the REINDEX_REL_* handling could be brushed. Another point that has been raised on a recent thread by Peter was that people preferred an hex style for the declarations rather than bit shifts. What do you think? -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
By the way-- What did you think of the idea of explictly marking the types used for bitmasks using types bits32 and friends, instead of plain int, which is harder to spot?
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-12-05 02:30, Michael Paquier wrote: On Fri, Dec 04, 2020 at 04:28:26PM -0300, Alvaro Herrera wrote: FWIW I'm with Peter on this. Okay, attached is a patch to adjust the enums for the set of utility commands that is the set of things I have touched lately. Should that be extended more? I have not done that as a lot of those structures exist as such for a long time. I think this patch is good. I have in the meantime committed a similar patch for cleaning up this issue in pg_dump.
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Fri, Dec 04, 2020 at 04:28:26PM -0300, Alvaro Herrera wrote:
> FWIW I'm with Peter on this.
Okay, attached is a patch to adjust the enums for the set of utility
commands that is the set of things I have touched lately. Should that
be extended more? I have not done that as a lot of those structures
exist as such for a long time.
--
Michael
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index c041628049..8f80f9f3aa 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -30,13 +30,10 @@ typedef enum
} IndexStateFlagsAction;
/* options for REINDEX */
-typedef enum ReindexOption
-{
- REINDEXOPT_VERBOSE = 1 << 0, /* print progress info */
- REINDEXOPT_REPORT_PROGRESS = 1 << 1, /* report pgstat progress */
- REINDEXOPT_MISSING_OK = 1 << 2, /* skip missing relations */
- REINDEXOPT_CONCURRENTLY = 1 << 3 /* concurrent mode */
-} ReindexOption;
+#define REINDEXOPT_VERBOSE (1 << 0) /* print progress info */
+#define REINDEXOPT_REPORT_PROGRESS (1 << 1) /* report pgstat progress */
+#define REINDEXOPT_MISSING_OK (1 << 2) /* skip missing relations */
+#define REINDEXOPT_CONCURRENTLY (1 << 3) /* concurrent mode */
/* state info for validate_index bulkdelete callback */
typedef struct ValidateIndexState
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 7cfb37c9b2..ded9379818 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -20,11 +20,8 @@
/* options for CLUSTER */
-typedef enum ClusterOption
-{
- CLUOPT_RECHECK = 1 << 0, /* recheck relation state */
- CLUOPT_VERBOSE = 1 << 1 /* print progress info */
-} ClusterOption;
+#define CLUOPT_RECHECK (1 << 0) /* recheck relation state */
+#define CLUOPT_VERBOSE (1 << 1) /* print progress info */
extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
extern void cluster_rel(Oid tableOid, Oid indexOid, int options);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index a4cd721400..1d7b6eaf04 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -174,17 +174,16 @@ typedef struct VacAttrStats
int rowstride;
} VacAttrStats;
-typedef enum VacuumOption
-{
- VACOPT_VACUUM = 1 << 0, /* do VACUUM */
- VACOPT_ANALYZE = 1 << 1, /* do ANALYZE */
- VACOPT_VERBOSE = 1 << 2, /* print progress info */
- VACOPT_FREEZE = 1 << 3, /* FREEZE option */
- VACOPT_FULL = 1 << 4, /* FULL (non-concurrent) vacuum */
- VACOPT_SKIP_LOCKED = 1 << 5, /* skip if cannot get lock */
- VACOPT_SKIPTOAST = 1 << 6, /* don't process the TOAST table, if any */
- VACOPT_DISABLE_PAGE_SKIPPING = 1 << 7 /* don't skip any pages */
-} VacuumOption;
+/* options for VACUUM */
+#define VACOPT_VACUUM (1 << 0) /* do VACUUM */
+#define VACOPT_ANALYZE (1 << 1) /* do ANALYZE */
+#define VACOPT_VERBOSE (1 << 2) /* print progress info */
+#define VACOPT_FREEZE (1 << 3) /* FREEZE option */
+#define VACOPT_FULL (1 << 4) /* FULL (non-concurrent) vacuum */
+#define VACOPT_SKIP_LOCKED (1 << 5) /* skip if cannot get lock */
+#define VACOPT_SKIPTOAST (1 << 6) /* don't process the TOAST table, if
+ * any */
+#define VACOPT_DISABLE_PAGE_SKIPPING (1 << 7) /* don't skip any pages */
/*
* A ternary value used by vacuum parameters.
@@ -207,7 +206,7 @@ typedef enum VacOptTernaryValue
*/
typedef struct VacuumParams
{
- int options; /* bitmask of VacuumOption */
+ int options; /* bitmask of VACOPT_* values */
int freeze_min_age; /* min freeze age, -1 to use default */
int freeze_table_age; /* age at which to scan whole table */
int multixact_freeze_min_age; /* min multixact freeze age, -1 to
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 14d24b3cc4..5e2cbba407 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2453,7 +2453,8 @@ ChooseIndexColumnNames(List *indexElems)
/*
* ReindexParseOptions
- * Parse list of REINDEX options, returning a bitmask of ReindexOption.
+ * Parse list of REINDEX options, returning a bitmask of REINDEXOPT_*
+ * values.
*/
int
ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt)
signature.asc
Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Fri, Dec 04, 2020 at 09:40:31PM +0300, Alexey Kondratov wrote:
> > I liked the bools, but dropped them so the patch is smaller.
>
> I had a look on 0001 and it looks mostly fine to me except some strange
> mixture of tabs/spaces in the ExecReindex(). There is also a couple of
> meaningful comments:
>
> - options =
> - (verbose ? REINDEXOPT_VERBOSE : 0) |
> - (concurrently ? REINDEXOPT_CONCURRENTLY : 0);
> + if (verbose)
> + params.options |= REINDEXOPT_VERBOSE;
>
> Why do we need this intermediate 'verbose' variable here? We only use it
> once to set a bitmask. Maybe we can do it like this:
>
> params.options |= defGetBoolean(opt) ?
> REINDEXOPT_VERBOSE : 0;
That allows *setting* REINDEXOPT_VERBOSE, but doesn't *unset* it if someone
runs (VERBOSE OFF). So I kept the bools like Michael originally had rather
than writing "else: params.options &= ~REINDEXOPT_VERBOSE"
> See also attached txt file with diff (I wonder can I trick cfbot this way,
> so it does not apply the diff).
Yes, I think that works :)
I believe it looks for *.diff and *.patch.
> + int options;/* bitmask of lowlevel REINDEXOPT_* */
>
> I would prefer if the comment says '/* bitmask of ReindexOption */' as in
> the VacuumOptions, since citing the exact enum type make it easier to
> navigate source code.
Yes, thanks.
This also fixes some minor formatting and rebase issues, including broken doc/.
--
Justin
>From 5d874da6f93341cb5aed4d3cc54137cbc341d57c Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Wed, 2 Dec 2020 20:54:47 -0600
Subject: [PATCH v33 1/5] ExecReindex and ReindexParams
TODO: typedef
---
src/backend/commands/indexcmds.c | 145 ---
src/backend/tcop/utility.c | 40 +
src/include/commands/defrem.h| 7 +-
3 files changed, 97 insertions(+), 95 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 14d24b3cc4..c2ee88f2c3 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -67,6 +67,10 @@
#include "utils/syscache.h"
+typedef struct ReindexParams {
+ int options; /* bitmask of ReindexOption */
+} ReindexParams;
+
/* non-export function prototypes */
static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
static void CheckPredicate(Expr *predicate);
@@ -86,12 +90,15 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId,
bool primary, bool isconstraint);
static char *ChooseIndexNameAddition(List *colnames);
static List *ChooseIndexColumnNames(List *indexElems);
+static void ReindexIndex(RangeVar *indexRelation, ReindexParams *params, bool isTopLevel);
+static Oid ReindexTable(RangeVar *relation, ReindexParams *params, bool isTopLevel);
+static void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, ReindexParams *params);
static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
static void reindex_error_callback(void *args);
-static void ReindexPartitions(Oid relid, int options, bool isTopLevel);
-static void ReindexMultipleInternal(List *relids, int options);
-static bool ReindexRelationConcurrently(Oid relationOid, int options);
+static void ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel);
+static void ReindexMultipleInternal(List *relids, ReindexParams *params);
+static bool ReindexRelationConcurrently(Oid relationOid, ReindexParams *params);
static void update_relispartition(Oid relationId, bool newval);
static inline void set_indexsafe_procflags(void);
@@ -100,7 +107,7 @@ static inline void set_indexsafe_procflags(void);
*/
struct ReindexIndexCallbackState
{
- int options; /* options from statement */
+ ReindexParams *params;
Oid locked_table_oid; /* tracks previously locked table */
};
@@ -2452,14 +2459,17 @@ ChooseIndexColumnNames(List *indexElems)
}
/*
- * ReindexParseOptions
- * Parse list of REINDEX options, returning a bitmask of ReindexOption.
+ * Reindex accordinging to stmt.
+ * This calls the intermediate routines: ReindexIndex, ReindexTable, ReindexMultipleTables,
+ * which ultimately call reindex_index, reindex_relation, ReindexRelationConcurrently.
+ * Note that partitioned relations are handled by ReindexPartitions, except that
+ * ReindexRelationConcurrently handles concurrently reindexing a table.
*/
-int
-ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt)
+void
+ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel)
{
- ListCell *lc;
- int options = 0;
+ ListCell *lc;
+ ReindexParams params = {0};
bool concurrently = false;
bool verbose = false;
@@ -2480,19 +2490,53 @@ ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt)
parser_errposition(pstate, opt->location)));
}
- options =
- (verbose ? REINDEXOPT_VERBOSE : 0) |
- (concurrently ? REINDEXOPT_CONCURRENTLY : 0);
+ if
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-Dec-04, Michael Paquier wrote:
> VacuumOption does that since 6776142, and ClusterOption since 9ebe057,
> so switching ReindexOption to just match the two others still looks
> like the most consistent move.
9ebe057 goes to show why this is a bad idea, since it has this:
+typedef enum ClusterOption
+{
+ CLUOPT_RECHECK, /* recheck relation state */
+ CLUOPT_VERBOSE /* print progress info */
+} ClusterOption;
and then you do things like
+ if ($2)
+ n->options |= CLUOPT_VERBOSE;
and then tests like
+ if ((options & VACOPT_VERBOSE) != 0)
Now if you were to ever define third and fourth values in that enum,
this would immediately start malfunctioning.
FWIW I'm with Peter on this.
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-12-04 04:25, Justin Pryzby wrote:
On Thu, Dec 03, 2020 at 04:12:53PM +0900, Michael Paquier wrote:
> +typedef struct ReindexParams {
> + bool concurrently;
> + bool verbose;
> + bool missingok;
> +
> + int options;/* bitmask of lowlevel REINDEXOPT_* */
> +} ReindexParams;
> +
By moving everything into indexcmds.c, keeping ReindexParams within it
makes sense to me. Now, there is no need for the three booleans
because options stores the same information, no?
I liked the bools, but dropped them so the patch is smaller.
I had a look on 0001 and it looks mostly fine to me except some strange
mixture of tabs/spaces in the ExecReindex(). There is also a couple of
meaningful comments:
- options =
- (verbose ? REINDEXOPT_VERBOSE : 0) |
- (concurrently ? REINDEXOPT_CONCURRENTLY : 0);
+ if (verbose)
+ params.options |= REINDEXOPT_VERBOSE;
Why do we need this intermediate 'verbose' variable here? We only use it
once to set a bitmask. Maybe we can do it like this:
params.options |= defGetBoolean(opt) ?
REINDEXOPT_VERBOSE : 0;
See also attached txt file with diff (I wonder can I trick cfbot this
way, so it does not apply the diff).
+ int options;/* bitmask of lowlevel REINDEXOPT_* */
I would prefer if the comment says '/* bitmask of ReindexOption */' as
in the VacuumOptions, since citing the exact enum type make it easier to
navigate source code.
Regarding the REINDEX patch, I think this comment is misleading:
|+* Even if table was moved to new tablespace,
normally toast cannot move.
| */
|+ Oid toasttablespaceOid = allowSystemTableMods ?
tablespaceOid : InvalidOid;
|result |= reindex_relation(toast_relid, flags,
I think it ought to say "Even if a table's indexes were moved to a new
tablespace, its toast table's index is not normally moved"
Right ?
Yes, I think so, we are dealing only with index tablespace changing
here. Thanks for noticing.
Also, I don't know whether we should check for GLOBALTABLESPACE_OID
after
calling get_tablespace_oid(), or in the lowlevel routines. Note that
reindex_relation is called during cluster/vacuum, and in the later
patches, I
moved the test from from cluster() and ExecVacuum() to
rebuild_relation().
IIRC, I wanted to do GLOBALTABLESPACE_OID check as early as possible
(just after getting Oid), since it does not make sense to proceed
further if tablespace is set to that value. So initially there were a
lot of duplicative GLOBALTABLESPACE_OID checks, since there were a lot
of reindex entry-points (index, relation, concurrently, etc.). Now we
are going to have ExecReindex(), so there are much less entry-points and
in my opinion it is fine to keep this validation just after
get_tablespace_oid().
However, this is mostly a sanity check. I can hardly imagine a lot of
users trying to constantly move indexes to the global tablespace, so it
is also OK to put this check deeper into guts.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Companydiff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index a27f8f9d83..0b1884815c 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2472,8 +2472,6 @@ void
ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel)
{
ReindexParams params = {0};
- bool verbose = false,
-concurrently = false;
ListCell *lc;
char *tablespace = NULL;
@@ -2483,9 +2481,11 @@ ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel)
DefElem*opt = (DefElem *) lfirst(lc);
if (strcmp(opt->defname, "verbose") == 0)
- verbose = defGetBoolean(opt);
+ params.options |= defGetBoolean(opt) ?
+REINDEXOPT_VERBOSE : 0;
else if (strcmp(opt->defname, "concurrently") == 0)
- concurrently = defGetBoolean(opt);
+ params.options |= defGetBoolean(opt) ?
+REINDEXOPT_CONCURRENTLY : 0;
else if (strcmp(opt->defname, "tablespace") == 0)
tablespace = defGetString(opt);
else
@@ -2496,18 +2496,12 @@ ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool isTopLevel)
parser_errposition(pstate, opt->location)));
}
- if (verbose)
- params.options |= REINDEXOPT_VERBOSE;
+ params.tablespaceOid = tablespace ?
+ get_tablespace_oid(tablespace, false) : InvalidOid;
- if (concurrently)
- {
- params.options |= REINDEXOPT_CONCURRENTLY;
+ if (params.options & REINDEXOPT_CONCURRENTLY)
PreventInTransactionBlock(isTopLevel,
"REINDEX CONCURRENTLY");
- }
-
- params.tablespaceOid = tablespace ?
- get_tablespace_oid(tablespace, false) : InvalidOid;
switch (stmt->kind)
{
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Thu, Dec 03, 2020 at 08:46:09PM +0100, Peter Eisentraut wrote: > There are a couple of more places like this, including the existing > ClusterOption that this patched moved around, but we should be removing > those. > > My reasoning is that if you look at an enum value of this type, either say > in a switch statement or a debugger, the enum value might not be any of the > defined symbols. So that way you lose all the type checking that an enum > might give you. VacuumOption does that since 6776142, and ClusterOption since 9ebe057, so switching ReindexOption to just match the two others still looks like the most consistent move. Please note that there is more than that, like ScanOptions, relopt_kind, RVROption, InstrumentOption, TableLikeOption. I would not mind changing that, though I am not sure that this improves readability. And if we'd do it, it may make sense to extend that even more to the places where it would apply like the places mentioned one paragraph above. -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Thu, Dec 03, 2020 at 04:12:53PM +0900, Michael Paquier wrote:
> > +typedef struct ReindexParams {
> > + bool concurrently;
> > + bool verbose;
> > + bool missingok;
> > +
> > + int options;/* bitmask of lowlevel REINDEXOPT_* */
> > +} ReindexParams;
> > +
>
> By moving everything into indexcmds.c, keeping ReindexParams within it
> makes sense to me. Now, there is no need for the three booleans
> because options stores the same information, no?
I liked the bools, but dropped them so the patch is smaller.
> > struct ReindexIndexCallbackState
> > {
> > - int options;/* options from
> > statement */
> > + boolconcurrently;
> > Oid locked_table_oid; /* tracks previously
> > locked table */
> > };
>
> Here also, I think that we should just pass down the full
> ReindexParams set.
Ok.
Regarding the REINDEX patch, I think this comment is misleading:
|/*
| * If the relation has a secondary toast rel, reindex that too while we
| * still hold the lock on the main table.
| */
|if ((flags & REINDEX_REL_PROCESS_TOAST) && OidIsValid(toast_relid))
|{
|/*
| * Note that this should fail if the toast relation is
missing, so
| * reset REINDEXOPT_MISSING_OK.
|+*
|+* Even if table was moved to new tablespace, normally toast
cannot move.
| */
|+ Oid toasttablespaceOid = allowSystemTableMods ? tablespaceOid
: InvalidOid;
|result |= reindex_relation(toast_relid, flags,
|- options &
~(REINDEXOPT_MISSING_OK));
|+ options &
~(REINDEXOPT_MISSING_OK),
|+
toasttablespaceOid);
|}
I think it ought to say "Even if a table's indexes were moved to a new
tablespace, its toast table's index is not normally moved"
Right ?
Also, I don't know whether we should check for GLOBALTABLESPACE_OID after
calling get_tablespace_oid(), or in the lowlevel routines. Note that
reindex_relation is called during cluster/vacuum, and in the later patches, I
moved the test from from cluster() and ExecVacuum() to rebuild_relation().
--
Justin
>From df43fe542081178ea74ffb2d1d77342e6c657c2f Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Wed, 2 Dec 2020 20:54:47 -0600
Subject: [PATCH v32 1/5] ExecReindex and ReindexParams
TODO: typedef
---
src/backend/commands/indexcmds.c | 151 ---
src/backend/tcop/utility.c | 40 +---
src/include/commands/defrem.h| 7 +-
3 files changed, 101 insertions(+), 97 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 14d24b3cc4..f0456dcbef 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -67,6 +67,10 @@
#include "utils/syscache.h"
+typedef struct ReindexParams {
+ int options; /* bitmask of lowlevel REINDEXOPT_* */
+} ReindexParams;
+
/* non-export function prototypes */
static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
static void CheckPredicate(Expr *predicate);
@@ -86,12 +90,17 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId,
bool primary, bool isconstraint);
static char *ChooseIndexNameAddition(List *colnames);
static List *ChooseIndexColumnNames(List *indexElems);
+
+static void ReindexIndex(RangeVar *indexRelation, ReindexParams *params, bool isTopLevel);
+static Oid ReindexTable(RangeVar *relation, ReindexParams *params, bool isTopLevel);
+static void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, ReindexParams *params);
+
static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
static void reindex_error_callback(void *args);
-static void ReindexPartitions(Oid relid, int options, bool isTopLevel);
-static void ReindexMultipleInternal(List *relids, int options);
-static bool ReindexRelationConcurrently(Oid relationOid, int options);
+static void ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel);
+static void ReindexMultipleInternal(List *relids, ReindexParams *params);
+static bool ReindexRelationConcurrently(Oid relationOid, ReindexParams *params);
static void update_relispartition(Oid relationId, bool newval);
static inline void set_indexsafe_procflags(void);
@@ -100,7 +109,7 @@ static inline void set_indexsafe_procflags(void);
*/
struct ReindexIndexCallbackState
{
- int options; /* options from statement */
+ ReindexParams *params;
Oid locked_table_oid; /* tracks previously locked table */
};
@@ -2452,16 +2461,19 @@ ChooseIndexColumnNames(List *indexElems)
}
/*
- * ReindexParseOptions
- * Parse list of
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
A side comment on this patch: I think using enums as bit mask values is
bad style. So changing this:
-/* Reindex options */
-#define REINDEXOPT_VERBOSE (1 << 0) /* print progress info */
-#define REINDEXOPT_REPORT_PROGRESS (1 << 1) /* report pgstat progress */
-#define REINDEXOPT_MISSING_OK (1 << 2) /* skip missing relations */
-#define REINDEXOPT_CONCURRENTLY (1 << 3) /* concurrent mode */
to this:
+typedef enum ReindexOption
+{
+ REINDEXOPT_VERBOSE = 1 << 0,/* print progress info */
+ REINDEXOPT_REPORT_PROGRESS = 1 << 1,/* report pgstat progress */
+ REINDEXOPT_MISSING_OK = 1 << 2, /* skip missing relations */
+ REINDEXOPT_CONCURRENTLY = 1 << 3/* concurrent mode */
+} ReindexOption;
seems wrong.
There are a couple of more places like this, including the existing
ClusterOption that this patched moved around, but we should be removing
those.
My reasoning is that if you look at an enum value of this type, either
say in a switch statement or a debugger, the enum value might not be any
of the defined symbols. So that way you lose all the type checking that
an enum might give you.
Let's just keep the #define's like it is done in almost all other places.
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Dec 02, 2020 at 10:30:08PM -0600, Justin Pryzby wrote:
> Good idea. I think you mean like this.
Yes, something like that. Thanks.
> +typedef struct ReindexParams {
> + bool concurrently;
> + bool verbose;
> + bool missingok;
> +
> + int options;/* bitmask of lowlevel REINDEXOPT_* */
> +} ReindexParams;
> +
By moving everything into indexcmds.c, keeping ReindexParams within it
makes sense to me. Now, there is no need for the three booleans
because options stores the same information, no?
> struct ReindexIndexCallbackState
> {
> - int options;/* options from
> statement */
> + boolconcurrently;
> Oid locked_table_oid; /* tracks previously
> locked table */
> };
Here also, I think that we should just pass down the full
ReindexParams set.
--
Michael
signature.asc
Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Thu, Dec 03, 2020 at 10:19:43AM +0900, Michael Paquier wrote:
> OK, this one is now committed. As of this thread, I think that we are
> going to need to do a bit more once we add options that are not just
> booleans for both commands (the grammar rules do not need to be
> changed now):
> - Have a ReindexParams, similarly to VacuumParams except that we store
> the results of the parsing in a single place. With the current HEAD,
> I did not see yet the point in doing so because we just need an
> integer that maps to a bitmask made of ReindexOption.
> - The part related to ReindexStmt in utility.c is getting more and
> more complicated, so we could move most of the execution into
> indexcmds.c with some sort of ExecReindex() doing the option parsing
> job, and go to the correct code path depending on the object type
> dealt with.
Good idea. I think you mean like this.
I don't know where to put the struct.
I thought maybe the lowlevel, integer options should live in the struct, in
addition to bools, but it's not important.
--
Justin
>From 520d1c54d6988d69768178b4abf03c5837654b9a Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Wed, 23 Sep 2020 18:21:16 +0300
Subject: [PATCH v31 3/5] Refactor and reuse set_rel_tablespace()
---
src/backend/catalog/index.c | 74
src/backend/commands/indexcmds.c | 35 ---
src/include/catalog/index.h | 2 +
3 files changed, 49 insertions(+), 62 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 532c11e9dd..b317f556df 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3607,7 +3607,6 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
Relation iRel,
heapRelation;
Oid heapId;
- Oid oldTablespaceOid;
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
PGRUsage ru0;
@@ -3723,41 +3722,17 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
tablespaceOid = InvalidOid;
/*
- * Set the new tablespace for the relation. Do that only in the
- * case where the reindex caller wishes to enforce a new tablespace.
+ * Set the new tablespace for the relation if requested.
*/
- oldTablespaceOid = iRel->rd_rel->reltablespace;
if (set_tablespace &&
- (tablespaceOid != oldTablespaceOid ||
- (tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid
+ set_rel_tablespace(indexId, tablespaceOid))
{
- Relation pg_class;
- Form_pg_class rd_rel;
- HeapTuple tuple;
-
- /* First get a modifiable copy of the relation's pg_class row */
- pg_class = table_open(RelationRelationId, RowExclusiveLock);
-
- tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(indexId));
- if (!HeapTupleIsValid(tuple))
- elog(ERROR, "cache lookup failed for relation %u", indexId);
- rd_rel = (Form_pg_class) GETSTRUCT(tuple);
-
/*
* Mark the relation as ready to be dropped at transaction commit,
* before making visible the new tablespace change so as this won't
* miss things.
*/
RelationDropStorage(iRel);
-
- /* Update the pg_class row */
- rd_rel->reltablespace = tablespaceOid;
- CatalogTupleUpdate(pg_class, >t_self, tuple);
-
- heap_freetuple(tuple);
-
- table_close(pg_class, RowExclusiveLock);
-
RelationAssumeNewRelfilenode(iRel);
/* Make sure the reltablespace change is visible */
@@ -4063,6 +4038,51 @@ reindex_relation(Oid relid, int flags, int options, Oid tablespaceOid)
return result;
}
+/*
+ * set_rel_tablespace - modify relation tablespace in the pg_class entry.
+ *
+ * 'reloid' is an Oid of relation to be modified.
+ * 'tablespaceOid' is an Oid of new tablespace.
+ *
+ * Catalog modification is done only if tablespaceOid is different from
+ * the currently set. Returned bool value is indicating whether any changes
+ * were made or not.
+ */
+bool
+set_rel_tablespace(Oid reloid, Oid tablespaceOid)
+{
+ Relation pg_class;
+ HeapTuple tuple;
+ Form_pg_class rd_rel;
+ bool changed = false;
+ Oid oldTablespaceOid;
+
+ /* Get a modifiable copy of the relation's pg_class row. */
+ pg_class = table_open(RelationRelationId, RowExclusiveLock);
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", reloid);
+ rd_rel = (Form_pg_class) GETSTRUCT(tuple);
+
+ /* No work if no change in tablespace. */
+ oldTablespaceOid = rd_rel->reltablespace;
+ if (tablespaceOid != oldTablespaceOid ||
+ (tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid)))
+ {
+ /* Update the pg_class row. */
+ rd_rel->reltablespace = (tablespaceOid == MyDatabaseTableSpace) ?
+ InvalidOid : tablespaceOid;
+ CatalogTupleUpdate(pg_class, >t_self, tuple);
+
+ changed = true;
+ }
+
+ heap_freetuple(tuple);
+ table_close(pg_class, RowExclusiveLock);
+
+ return changed;
+}
/*
*
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Dec 01, 2020 at 03:10:13PM +0900, Michael Paquier wrote: > Well, my impression is that both of you kept exchanging patches, > touching and reviewing each other's patch (note that Alexei has also > sent a rebase of 0002 just yesterday), so I think that it is fair to > say that both of you should be listed as authors and credited as such > in the release notes for this one. OK, this one is now committed. As of this thread, I think that we are going to need to do a bit more once we add options that are not just booleans for both commands (the grammar rules do not need to be changed now): - Have a ReindexParams, similarly to VacuumParams except that we store the results of the parsing in a single place. With the current HEAD, I did not see yet the point in doing so because we just need an integer that maps to a bitmask made of ReindexOption. - The part related to ReindexStmt in utility.c is getting more and more complicated, so we could move most of the execution into indexcmds.c with some sort of ExecReindex() doing the option parsing job, and go to the correct code path depending on the object type dealt with. -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Mon, Nov 30, 2020 at 11:43:08PM -0600, Justin Pryzby wrote: > I eyeballed the patch and rebased the rest of the series on top if it to play > with. Looks fine - thanks. Cool, thanks. > FYI, the commit messages have the proper "author" for attribution. I proposed > and implemented the grammar changes in 0002, and implemented INDEX_TABLESPACE, > but I'm a reviewer for the main patches. Well, my impression is that both of you kept exchanging patches, touching and reviewing each other's patch (note that Alexei has also sent a rebase of 0002 just yesterday), so I think that it is fair to say that both of you should be listed as authors and credited as such in the release notes for this one. -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Dec 01, 2020 at 11:46:55AM +0900, Michael Paquier wrote: > On Mon, Nov 30, 2020 at 05:12:42PM +0300, Alexey Kondratov wrote: > > Thanks. I have rebased the remaining patches on top of 873ea9ee to use > > 'utility_option_list' instead of 'common_option_list'. > > Thanks, that helps a lot. I have gone through 0002, and tweaked it as > the attached (note that this patch is also interesting for another > thing in development: backend-side reindex filtering of > collation-sensitive indexes). Does that look right to you? I eyeballed the patch and rebased the rest of the series on top if it to play with. Looks fine - thanks. FYI, the commit messages have the proper "author" for attribution. I proposed and implemented the grammar changes in 0002, and implemented INDEX_TABLESPACE, but I'm a reviewer for the main patches. -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Mon, Nov 30, 2020 at 05:12:42PM +0300, Alexey Kondratov wrote:
> Thanks. I have rebased the remaining patches on top of 873ea9ee to use
> 'utility_option_list' instead of 'common_option_list'.
Thanks, that helps a lot. I have gone through 0002, and tweaked it as
the attached (note that this patch is also interesting for another
thing in development: backend-side reindex filtering of
collation-sensitive indexes). Does that look right to you?
These are mostly matters of consistency with the other commands using
DefElem, but I think that it is important to get things right:
- Having the list of options in parsenodes.h becomes incorrect,
because these get now only used at execution time, like VACUUM. So I
have moved that to cluster.h and index.h.
- Let's use an enum for REINDEX, like the others.
- Having parse_reindex_params() in utility.c is wrong for something
aimed at being used only for REINDEX, so I have moved that to
indexcmds.c, and renamed the routine to be more consistent with the
rest. I think that we could more here by having an ExecReindex() that
does all the work based on object types, but I have left that out for
now to keep the change minimal.
- Switched one of the existing tests to stress CONCURRENTLY within
parenthesis.
- Indented the whole.
A couple of extra things below.
* CLUSTER [VERBOSE] [ USING ]
+ * CLUSTER [VERBOSE] [(options)] [ USING ]
This line is wrong, and should be:
CLUSTER [ (options) ] [ USING ]
+CLUSTER [VERBOSE] [ ( option
+CLUSTER [VERBOSE] [ ( option [,
...] ) ]
The docs in cluster.sgml are wrong as well, you can have VERBOSE as a
single option or as a parenthesized option, but never both at the same
time. On the contrary, psql completion got that right. I was first a
bit surprised that you would not allow the parenthesized set for the
case where a relation is not specified in the command, but I agree
that this does not seem worth the extra complexity now as this thread
aims at being able to use TABLESPACE which makes little sense
database-wide.
-VERBOSE
+VERBOSE [ boolean ]
Forgot about CONCURRENTLY as an option here, as this becomes
possible.
--
Michael
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index f4559b09d7..c041628049 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -29,6 +29,15 @@ typedef enum
INDEX_DROP_SET_DEAD
} IndexStateFlagsAction;
+/* options for REINDEX */
+typedef enum ReindexOption
+{
+ REINDEXOPT_VERBOSE = 1 << 0, /* print progress info */
+ REINDEXOPT_REPORT_PROGRESS = 1 << 1, /* report pgstat progress */
+ REINDEXOPT_MISSING_OK = 1 << 2, /* skip missing relations */
+ REINDEXOPT_CONCURRENTLY = 1 << 3 /* concurrent mode */
+} ReindexOption;
+
/* state info for validate_index bulkdelete callback */
typedef struct ValidateIndexState
{
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index e05884781b..7cfb37c9b2 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -14,11 +14,19 @@
#define CLUSTER_H
#include "nodes/parsenodes.h"
+#include "parser/parse_node.h"
#include "storage/lock.h"
#include "utils/relcache.h"
-extern void cluster(ClusterStmt *stmt, bool isTopLevel);
+/* options for CLUSTER */
+typedef enum ClusterOption
+{
+ CLUOPT_RECHECK = 1 << 0, /* recheck relation state */
+ CLUOPT_VERBOSE = 1 << 1 /* print progress info */
+} ClusterOption;
+
+extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
extern void cluster_rel(Oid tableOid, Oid indexOid, int options);
extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
bool recheck, LOCKMODE lockmode);
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 7a079ef07f..1133ae1143 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -34,6 +34,7 @@ extern ObjectAddress DefineIndex(Oid relationId,
bool check_not_in_use,
bool skip_build,
bool quiet);
+extern int ReindexParseOptions(ParseState *pstate, ReindexStmt *stmt);
extern void ReindexIndex(RangeVar *indexRelation, int options, bool isTopLevel);
extern Oid ReindexTable(RangeVar *relation, int options, bool isTopLevel);
extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index d1f9ef29ca..d6a6969b0d 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3196,18 +3196,12 @@ typedef struct AlterSystemStmt
* Cluster Statement (support pbrown's cluster index implementation)
* --
*/
-typedef enum ClusterOption
-{
- CLUOPT_RECHECK = 1 << 0, /* recheck relation state */
- CLUOPT_VERBOSE = 1 << 1 /* print progress info */
-} ClusterOption;
-
typedef struct ClusterStmt
{
NodeTag type;
RangeVar *relation; /* relation being indexed, or NULL if all */
char *indexname;
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-11-30 14:33, Michael Paquier wrote:
On Tue, Nov 24, 2020 at 09:31:23AM -0600, Justin Pryzby wrote:
@cfbot: rebased
Catching up with the activity here, I can see four different things in
the patch set attached:
1) Refactoring of the grammar of CLUSTER, VACUUM, ANALYZE and REINDEX
to support values in parameters.
2) Tablespace change for REINDEX.
3) Tablespace change for VACUUM FULL/CLUSTER.
4) Tablespace change for indexes with VACUUM FULL/CLUSTER.
I am not sure yet about the last three points, so let's begin with 1)
that is dealt with in 0001 and 0002. I have spent some time on 0001,
renaming the rule names to be less generic than "common", and applied
it. 0002 looks to be in rather good shape, still there are a few
things that have caught my eyes. I'll look at that more closely
tomorrow.
Thanks. I have rebased the remaining patches on top of 873ea9ee to use
'utility_option_list' instead of 'common_option_list'.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres CompanyFrom ac3b77aec26a40016784ada9dab8b9059f424fa4 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Tue, 31 Mar 2020 20:35:41 -0500
Subject: [PATCH v31 5/5] Implement vacuum full/cluster (INDEX_TABLESPACE
)
---
doc/src/sgml/ref/cluster.sgml | 12 -
doc/src/sgml/ref/vacuum.sgml | 12 -
src/backend/commands/cluster.c| 64 ++-
src/backend/commands/matview.c| 3 +-
src/backend/commands/tablecmds.c | 2 +-
src/backend/commands/vacuum.c | 46 +++-
src/backend/postmaster/autovacuum.c | 1 +
src/include/commands/cluster.h| 6 ++-
src/include/commands/vacuum.h | 5 +-
src/test/regress/input/tablespace.source | 13 +
src/test/regress/output/tablespace.source | 20 +++
11 files changed, 123 insertions(+), 61 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index cbfc0582be..6781e3a025 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -28,6 +28,7 @@ CLUSTER [VERBOSE] [ ( option [, ...
VERBOSE [ boolean ]
TABLESPACE new_tablespace
+INDEX_TABLESPACE new_tablespace
@@ -105,6 +106,15 @@ CLUSTER [VERBOSE] [ ( option [, ...
+
+INDEX_TABLESPACE
+
+
+ Specifies that the table's indexes will be rebuilt on a new tablespace.
+
+
+
+
table_name
@@ -141,7 +151,7 @@ CLUSTER [VERBOSE] [ ( option [, ...
new_tablespace
- The tablespace where the table will be rebuilt.
+ The tablespace where the table or its indexes will be rebuilt.
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 ] [ boolean ]
PARALLEL integer
TABLESPACE new_tablespace
+INDEX_TABLESPACE new_tablespace
and table_and_columns is:
@@ -265,6 +266,15 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ boolean
@@ -314,7 +324,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ new_tablespace
- The tablespace where the relation will be rebuilt.
+ The tablespace where the relation or its indexes will be rebuilt.
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index b289a76d58..0f9f09a15a 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);
@@ -107,9 +107,11 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
{
ListCell *lc;
int options = 0;
- /* Name and Oid of tablespace to use for clustered relation. */
- char *tablespaceName = NULL;
- Oid tablespaceOid = InvalidOid;
+ /* Name and Oid of tablespaces to use for clustered relations. */
+ char *tablespaceName = NULL,
+*idxtablespaceName = NULL;
+ Oid tablespaceOid,
+idxtablespaceOid;
/* Parse list of generic parameters not handled by the parser */
foreach(lc, stmt->params)
@@ -123,6 +125,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
options &= ~CLUOPT_VERBOSE;
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),
@@ -131,18 +135,11 @@ cluster(ParseState *pstate,
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Nov 24, 2020 at 09:31:23AM -0600, Justin Pryzby wrote: > @cfbot: rebased Catching up with the activity here, I can see four different things in the patch set attached: 1) Refactoring of the grammar of CLUSTER, VACUUM, ANALYZE and REINDEX to support values in parameters. 2) Tablespace change for REINDEX. 3) Tablespace change for VACUUM FULL/CLUSTER. 4) Tablespace change for indexes with VACUUM FULL/CLUSTER. I am not sure yet about the last three points, so let's begin with 1) that is dealt with in 0001 and 0002. I have spent some time on 0001, renaming the rule names to be less generic than "common", and applied it. 0002 looks to be in rather good shape, still there are a few things that have caught my eyes. I'll look at that more closely tomorrow. -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Sat, Oct 31, 2020 at 01:36:11PM -0500, Justin Pryzby wrote:
> > > From the grammar perspective ANY option is available for any command
> > > that uses parenthesized option list. All the checks and validations
> > > are performed at the corresponding command code.
> > > This analyze_keyword is actually doing only an ANALYZE word
> > > normalization if it's used as an option. Why it could be harmful?
> >
> > Michael has not replied since then, but he was relatively positive about
> > 0005 initially, so I put it as a first patch now.
>
> Thanks. I rebased Alexey's latest patch on top of recent changes to
> cluster.c.
> This puts the generic grammar changes first. I wasn't paying much attention
> to
> that part, so still waiting for a committer review.
@cfbot: rebased
>From 4a8e71ac704bd4c58e54703298b5234946c666a3 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Wed, 2 Sep 2020 23:05:16 +0300
Subject: [PATCH v30 1/6] Refactor gram.y in order to add a common
parenthesized option list
Previously there were two identical option lists
(explain_option_list and vac_analyze_option_list) + very similar
reindex_option_list. It does not seem to make
sense to maintain identical option lists in the grammar, since
all new options are added and parsed in the backend code.
That way, new common_option_list added in order to replace all
explain_option_list, vac_analyze_option_list and probably
also reindex_option_list.
---
src/backend/parser/gram.y | 61 +--
1 file changed, 14 insertions(+), 47 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index efc9c99754..5c86063459 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -315,10 +315,10 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
create_extension_opt_item alter_extension_opt_item
%type opt_lock lock_type cast_context
-%type vac_analyze_option_name
-%type vac_analyze_option_elem
-%type vac_analyze_option_list
-%type vac_analyze_option_arg
+%type common_option_name
+%type common_option_elem
+%type common_option_list
+%type common_option_arg
%type drop_option
%type opt_or_replace opt_no
opt_grant_grant_option opt_grant_admin_option
@@ -513,10 +513,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type generic_option_arg
%type generic_option_elem alter_generic_option_elem
%type generic_option_list alter_generic_option_list
-%type explain_option_name
-%type explain_option_arg
-%type explain_option_elem
-%type explain_option_list
%type reindex_target_type reindex_target_multitable
%type reindex_option_list reindex_option_elem
@@ -10483,7 +10479,7 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose opt_analyze opt_vacuum_relati
n->is_vacuumcmd = true;
$$ = (Node *)n;
}
- | VACUUM '(' vac_analyze_option_list ')' opt_vacuum_relation_list
+ | VACUUM '(' common_option_list ')' opt_vacuum_relation_list
{
VacuumStmt *n = makeNode(VacuumStmt);
n->options = $3;
@@ -10504,7 +10500,7 @@ AnalyzeStmt: analyze_keyword opt_verbose opt_vacuum_relation_list
n->is_vacuumcmd = false;
$$ = (Node *)n;
}
- | analyze_keyword '(' vac_analyze_option_list ')' opt_vacuum_relation_list
+ | analyze_keyword '(' common_option_list ')' opt_vacuum_relation_list
{
VacuumStmt *n = makeNode(VacuumStmt);
n->options = $3;
@@ -10514,12 +10510,12 @@ AnalyzeStmt: analyze_keyword opt_verbose opt_vacuum_relation_list
}
;
-vac_analyze_option_list:
- vac_analyze_option_elem
+common_option_list:
+ common_option_elem
{
$$ = list_make1($1);
}
- | vac_analyze_option_list ',' vac_analyze_option_elem
+ | common_option_list ',' common_option_elem
{
$$ = lappend($1, $3);
}
@@ -10530,19 +10526,19 @@ analyze_keyword:
| ANALYSE /* British */
;
-vac_analyze_option_elem:
- vac_analyze_option_name vac_analyze_option_arg
+common_option_elem:
+ common_option_name common_option_arg
{
$$ = makeDefElem($1, $2, @1);
}
;
-vac_analyze_option_name:
+common_option_name:
NonReservedWord { $$ = $1; }
| analyze_keyword { $$ = "analyze"; }
;
-vac_analyze_option_arg:
+common_option_arg:
opt_boolean_or_string { $$ = (Node *) makeString($1); }
| NumericOnly { $$ = (Node *) $1; }
| /* EMPTY */ { $$ = NULL; }
@@ -10624,7 +10620,7 @@ ExplainStmt:
n->options = list_make1(makeDefElem("verbose", NULL, @2));
$$ = (Node *) n;
}
- | EXPLAIN '(' explain_option_list ')' ExplainableStmt
+ | EXPLAIN '(' common_option_list ')' ExplainableStmt
{
ExplainStmt *n = makeNode(ExplainStmt);
n->query = $5;
@@ -10645,35 +10641,6 @@ ExplainableStmt:
| ExecuteStmt /* by default all are $$=$1 */
;
-explain_option_list:
- explain_option_elem
-{
- $$ = list_make1($1);
-}
- |
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Sep 23, 2020 at 07:43:01PM +0300, Alexey Kondratov wrote:
> On 2020-09-09 18:36, Justin Pryzby wrote:
> > Rebased on a6642b3ae: Add support for partitioned tables and indexes in
> > REINDEX
> >
> > So now this includes the new functionality and test for reindexing a
> > partitioned table onto a new tablespace. That part could use some
> > additional
> > review.
>
> I have finally had a look on your changes regarding partitioned tables.
>
> +set_rel_tablespace(Oid indexid, char *tablespace)
> +{
> + Oid tablespaceOid = tablespace ? get_tablespace_oid(tablespace, false) :
> + InvalidOid;
>
> You pass a tablespace name to set_rel_tablespace(), but it is already parsed
> into the Oid before. So I do not see why we need this extra work here
> instead of just passing Oid directly.
>
> Also set_rel_tablespace() does not check for a no-op case, i.e. if requested
> tablespace is the same as before.
>
> + /*
> + * Set the new tablespace for the relation. Do that only in the
> + * case where the reindex caller wishes to enforce a new tablespace.
> + */
> + if (set_tablespace &&
> + tablespaceOid != iRel->rd_rel->reltablespace)
>
> Just noticed that this check is not completely correct as well, since it
> does not check for MyDatabaseTableSpace (stored as InvalidOid) logic.
>
> I put these small fixes directly into the attached 0003.
>
> Also, I thought about your comment above set_rel_tablespace() and did a bit
> 'extreme' refactoring, which is attached as a separated patch 0004. The only
> one doubtful change IMO is reordering of RelationDropStorage() operation
> inside reindex_index(). However, it only schedules unlinking of physical
> storage at transaction commit, so this refactoring seems to be safe.
>
> If there will be no objections I would merge it with 0003.
>
> On 2020-09-09 16:03, Alexey Kondratov wrote:
> > On 2020-09-09 15:22, Michael Paquier wrote:
> > >
> > > By the way, skimming through the patch set, I was wondering if we
> > > could do the refactoring of patch 0005 as a first step
> > >
> >
> > Yes, I did it with intention to put as a first patch, but wanted to
> > get some feedback. It's easier to refactor the last patch without
> > rebasing others.
> >
> > >
> > > until I
> > > noticed this part:
> > > +common_option_name:
> > > NonReservedWord { $$ = $1; }
> > > | analyze_keyword { $$ = "analyze"; }
> > > This is not a good idea as you make ANALYZE an option available for
> > > all the commands involved in the refactoring. A portion of that could
> > > be considered though, like the use of common_option_arg.
> > >
> >
> > From the grammar perspective ANY option is available for any command
> > that uses parenthesized option list. All the checks and validations
> > are performed at the corresponding command code.
> > This analyze_keyword is actually doing only an ANALYZE word
> > normalization if it's used as an option. Why it could be harmful?
> >
>
> Michael has not replied since then, but he was relatively positive about
> 0005 initially, so I put it as a first patch now.
Thanks. I rebased Alexey's latest patch on top of recent changes to cluster.c.
This puts the generic grammar changes first. I wasn't paying much attention to
that part, so still waiting for a committer review.
--
Justin
>From 72f7af2b39587304c945e75d2b82a3a09a2cf7fa Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Wed, 2 Sep 2020 23:05:16 +0300
Subject: [PATCH v29 1/7] Refactor gram.y in order to add a common
parenthesized option list
Previously there were two identical option lists
(explain_option_list and vac_analyze_option_list) + very similar
reindex_option_list. It does not seem to make
sense to maintain identical option lists in the grammar, since
all new options are added and parsed in the backend code.
That way, new common_option_list added in order to replace all
explain_option_list, vac_analyze_option_list and probably
also reindex_option_list.
---
src/backend/parser/gram.y | 61 +--
1 file changed, 14 insertions(+), 47 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 480d168346..0828c27944 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -315,10 +315,10 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
create_extension_opt_item alter_extension_opt_item
%type opt_lock lock_type cast_context
-%type vac_analyze_option_name
-%type vac_analyze_option_elem
-%type vac_analyze_option_list
-%type vac_analyze_option_arg
+%type common_option_name
+%type common_option_elem
+%type common_option_list
+%type common_option_arg
%type drop_option
%type opt_or_replace opt_no
opt_grant_grant_option opt_grant_admin_option
@@ -513,10 +513,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type generic_option_arg
%type
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Sep 09, 2020 at 09:22:00PM +0900, Michael Paquier wrote: > On Tue, Sep 08, 2020 at 07:17:58PM -0500, Justin Pryzby wrote: > > Initially I added List *params, and Michael suggested to retire > > ReindexStmt->concurrent. I provided a patch to do so, initially by leaving > > int > > options and then, after this, removing it to "complete the thought", and get > > rid of the remnants of the "old way" of doing it. This is also how vacuum > > and > > explain are done. > > https://www.postgresql.org/message-id/20200902022410.GA20149%40telsasoft.com > > Defining a set of DefElem when parsing and then using the int > "options" with bitmasks where necessary at the beginning of the > execution looks like a good balance to me. This way, you can extend > the grammar to use things like (verbose = true), etc. It doesn't need to be extended - defGetBoolean already handles that. I don't see what good can come from storing the information in two places in the same structure. |postgres=# CLUSTER (VERBOSE on) pg_attribute USING pg_attribute_relid_attnum_index ; |INFO: clustering "pg_catalog.pg_attribute" using index scan on "pg_attribute_relid_attnum_index" |INFO: "pg_attribute": found 0 removable, 2968 nonremovable row versions in 55 pages |DETAIL: 0 dead row versions cannot be removed yet. |CPU: user: 0.01 s, system: 0.00 s, elapsed: 0.01 s. |CLUSTER -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-09-09 15:22, Michael Paquier wrote:
On Tue, Sep 08, 2020 at 07:17:58PM -0500, Justin Pryzby wrote:
Initially I added List *params, and Michael suggested to retire
ReindexStmt->concurrent. I provided a patch to do so, initially by
leaving int
options and then, after this, removing it to "complete the thought",
and get
rid of the remnants of the "old way" of doing it. This is also how
vacuum and
explain are done.
https://www.postgresql.org/message-id/20200902022410.GA20149%40telsasoft.com
Defining a set of DefElem when parsing and then using the int
"options" with bitmasks where necessary at the beginning of the
execution looks like a good balance to me. This way, you can extend
the grammar to use things like (verbose = true), etc.
By the way, skimming through the patch set, I was wondering if we
could do the refactoring of patch 0005 as a first step
Yes, I did it with intention to put as a first patch, but wanted to get
some feedback. It's easier to refactor the last patch without rebasing
others.
until I
noticed this part:
+common_option_name:
NonReservedWord { $$ = $1; }
| analyze_keyword { $$ = "analyze"; }
This is not a good idea as you make ANALYZE an option available for
all the commands involved in the refactoring. A portion of that could
be considered though, like the use of common_option_arg.
From the grammar perspective ANY option is available for any command
that uses parenthesized option list. All the checks and validations are
performed at the corresponding command code.
This analyze_keyword is actually doing only an ANALYZE word
normalization if it's used as an option. Why it could be harmful?
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Sep 08, 2020 at 07:17:58PM -0500, Justin Pryzby wrote:
> Initially I added List *params, and Michael suggested to retire
> ReindexStmt->concurrent. I provided a patch to do so, initially by leaving
> int
> options and then, after this, removing it to "complete the thought", and get
> rid of the remnants of the "old way" of doing it. This is also how vacuum and
> explain are done.
> https://www.postgresql.org/message-id/20200902022410.GA20149%40telsasoft.com
Defining a set of DefElem when parsing and then using the int
"options" with bitmasks where necessary at the beginning of the
execution looks like a good balance to me. This way, you can extend
the grammar to use things like (verbose = true), etc.
By the way, skimming through the patch set, I was wondering if we
could do the refactoring of patch 0005 as a first step, until I
noticed this part:
+common_option_name:
NonReservedWord { $$ = $1; }
| analyze_keyword { $$ = "analyze"; }
This is not a good idea as you make ANALYZE an option available for
all the commands involved in the refactoring. A portion of that could
be considered though, like the use of common_option_arg.
--
Michael
signature.asc
Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Sep 08, 2020 at 09:02:38PM -0300, Alvaro Herrera wrote: > On 2020-Sep-08, Justin Pryzby wrote: > > > From 992e0121925c74d5c5a4e5b132cddb3d6b31da86 Mon Sep 17 00:00:00 2001 > > From: Justin Pryzby > > Date: Fri, 27 Mar 2020 17:50:46 -0500 > > Subject: [PATCH v27 1/5] Change REINDEX/CLUSTER to accept an option list.. > > > > ..like EXPLAIN (..), VACUUM (..), and ANALYZE (..). > > > > Change docs in the style of VACUUM. See also: > > 52dcfda48778d16683c64ca4372299a099a15b96 > > I don't understand why you change all options to DefElem instead of > keeping the bitmask for those options that can use it. That's originally how I did it, too. Initially I added List *params, and Michael suggested to retire ReindexStmt->concurrent. I provided a patch to do so, initially by leaving int options and then, after this, removing it to "complete the thought", and get rid of the remnants of the "old way" of doing it. This is also how vacuum and explain are done. https://www.postgresql.org/message-id/20200902022410.GA20149%40telsasoft.com -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-Sep-08, Justin Pryzby wrote: > From 992e0121925c74d5c5a4e5b132cddb3d6b31da86 Mon Sep 17 00:00:00 2001 > From: Justin Pryzby > Date: Fri, 27 Mar 2020 17:50:46 -0500 > Subject: [PATCH v27 1/5] Change REINDEX/CLUSTER to accept an option list.. > > ..like EXPLAIN (..), VACUUM (..), and ANALYZE (..). > > Change docs in the style of VACUUM. See also: > 52dcfda48778d16683c64ca4372299a099a15b96 I don't understand why you change all options to DefElem instead of keeping the bitmask for those options that can use it. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Thu, Sep 03, 2020 at 09:43:51PM -0500, Justin Pryzby wrote:
> And rebased on Michael's commit removing ReindexStmt->concurrent.
Rebased on a6642b3ae: Add support for partitioned tables and indexes in REINDEX
So now this includes the new functionality and test for reindexing a
partitioned table onto a new tablespace. That part could use some additional
review.
I guess this patch series will also conflict with the CLUSTER part of this
other one. Once its CLUSTER patch is commited, this patch should to be updated
to test clustering a partitioned table to a new tbspc.
https://commitfest.postgresql.org/29/2584/
REINDEX/CIC/CLUSTER of partitioned tables
--
Justin
>From 992e0121925c74d5c5a4e5b132cddb3d6b31da86 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 27 Mar 2020 17:50:46 -0500
Subject: [PATCH v27 1/5] Change REINDEX/CLUSTER to accept an option list..
..like EXPLAIN (..), VACUUM (..), and ANALYZE (..).
Change docs in the style of VACUUM. See also: 52dcfda48778d16683c64ca4372299a099a15b96
---
doc/src/sgml/ref/cluster.sgml | 27 ++---
doc/src/sgml/ref/reindex.sgml | 43 ++-
src/backend/commands/cluster.c | 28 --
src/backend/nodes/copyfuncs.c | 4 +--
src/backend/nodes/equalfuncs.c | 4 +--
src/backend/parser/gram.y | 54 +++---
src/backend/tcop/utility.c | 38 ++--
src/bin/psql/tab-complete.c| 22 ++
src/include/commands/cluster.h | 3 +-
src/include/nodes/parsenodes.h | 4 +--
10 files changed, 167 insertions(+), 60 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 4da60d8d56..e6ebce27e6 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -21,8 +21,13 @@ PostgreSQL documentation
-CLUSTER [VERBOSE] table_name [ USING index_name ]
-CLUSTER [VERBOSE]
+CLUSTER [VERBOSE] [ ( option [, ...] ) ] table_name [ USING index_name ]
+CLUSTER [VERBOSE] [ ( option [, ...] ) ]
+
+where option can be one of:
+
+VERBOSE [ boolean ]
+
@@ -81,6 +86,15 @@ CLUSTER [VERBOSE]
Parameters
+
+VERBOSE
+
+
+ Prints a progress report as each table is clustered.
+
+
+
+
table_name
@@ -100,10 +114,15 @@ CLUSTER [VERBOSE]
-VERBOSE
+boolean
- Prints a progress report as each table is clustered.
+ Specifies whether the selected option should be turned on or off.
+ You can write TRUE, ON, or
+ 1 to enable the option, and FALSE,
+ OFF, or 0 to disable it. The
+ boolean value can also
+ be omitted, in which case TRUE is assumed.
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 33af4ae02a..593b38a7ee 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -25,7 +25,7 @@ REINDEX [ ( option [, ...] ) ] { IN
where option can be one of:
-VERBOSE
+VERBOSE [ boolean ]
@@ -145,19 +145,6 @@ REINDEX [ ( option [, ...] ) ] { IN
-
-name
-
-
- The name of the specific index, table, or database to be
- reindexed. Index and table names can be schema-qualified.
- Presently, REINDEX DATABASE and REINDEX SYSTEM
- can only reindex the current database, so their parameter must match
- the current database's name.
-
-
-
-
CONCURRENTLY
@@ -185,6 +172,34 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+
+name
+
+
+ The name of the specific index, table, or database to be
+ reindexed. Index and table names can be schema-qualified.
+ Presently, REINDEX DATABASE and REINDEX SYSTEM
+ can only reindex the current database, so their parameter must match
+ the current database's name.
+
+
+
+
+
+boolean
+
+
+ Specifies whether the selected option should be turned on or off.
+ You can write TRUE, ON, or
+ 1 to enable the option, and FALSE,
+ OFF, or 0 to disable it. The
+ boolean value can also
+ be omitted, in which case TRUE is assumed.
+
+
+
+
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 0d647e912c..4a3678831d 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -35,6 +35,7 @@
#include "catalog/pg_am.h"
#include "catalog/toasting.h"
#include "commands/cluster.h"
+#include "commands/defrem.h"
#include "commands/progress.h"
#include "commands/tablecmds.h"
#include "commands/vacuum.h"
@@ -102,8 +103,29 @@ static List *get_tables_to_cluster(MemoryContext cluster_context);
*---
*/
void
-cluster(ClusterStmt *stmt, bool isTopLevel)
+cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
{
+ ListCell *lc;
+ int options = 0;
+
+ /* Parse list of
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Sep 02, 2020 at 06:07:06PM -0500, Justin Pryzby wrote:
> On my side, I've also rearranged function parameters to make the diff more
> readable. And squishes your changes into the respective patches.
This resolves a breakage I failed to notice from a last-minute edit.
And squishes two commits.
And rebased on Michael's commit removing ReindexStmt->concurrent.
--
Justin
>From 7e04caad0d010b5fd3eeca8d9bd436e89b657e4a Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 27 Mar 2020 17:50:46 -0500
Subject: [PATCH v26 1/5] Change REINDEX/CLUSTER to accept an option list..
..like EXPLAIN (..), VACUUM (..), and ANALYZE (..).
Change docs in the style of VACUUM. See also: 52dcfda48778d16683c64ca4372299a099a15b96
---
doc/src/sgml/ref/cluster.sgml | 27 ++---
doc/src/sgml/ref/reindex.sgml | 43 ++-
src/backend/commands/cluster.c | 28 --
src/backend/nodes/copyfuncs.c | 4 +--
src/backend/nodes/equalfuncs.c | 4 +--
src/backend/parser/gram.y | 54 +++---
src/backend/tcop/utility.c | 42 ++
src/bin/psql/tab-complete.c| 22 ++
src/include/commands/cluster.h | 3 +-
src/include/commands/defrem.h | 2 +-
src/include/nodes/parsenodes.h | 4 +--
11 files changed, 170 insertions(+), 63 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 4da60d8d56..e6ebce27e6 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -21,8 +21,13 @@ PostgreSQL documentation
-CLUSTER [VERBOSE] table_name [ USING index_name ]
-CLUSTER [VERBOSE]
+CLUSTER [VERBOSE] [ ( option [, ...] ) ] table_name [ USING index_name ]
+CLUSTER [VERBOSE] [ ( option [, ...] ) ]
+
+where option can be one of:
+
+VERBOSE [ boolean ]
+
@@ -81,6 +86,15 @@ CLUSTER [VERBOSE]
Parameters
+
+VERBOSE
+
+
+ Prints a progress report as each table is clustered.
+
+
+
+
table_name
@@ -100,10 +114,15 @@ CLUSTER [VERBOSE]
-VERBOSE
+boolean
- Prints a progress report as each table is clustered.
+ Specifies whether the selected option should be turned on or off.
+ You can write TRUE, ON, or
+ 1 to enable the option, and FALSE,
+ OFF, or 0 to disable it. The
+ boolean value can also
+ be omitted, in which case TRUE is assumed.
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index c16f223e4e..a32e192a87 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -25,7 +25,7 @@ REINDEX [ ( option [, ...] ) ] { IN
where option can be one of:
-VERBOSE
+VERBOSE [ boolean ]
@@ -141,19 +141,6 @@ REINDEX [ ( option [, ...] ) ] { IN
-
-name
-
-
- The name of the specific index, table, or database to be
- reindexed. Index and table names can be schema-qualified.
- Presently, REINDEX DATABASE and REINDEX SYSTEM
- can only reindex the current database, so their parameter must match
- the current database's name.
-
-
-
-
CONCURRENTLY
@@ -181,6 +168,34 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+
+name
+
+
+ The name of the specific index, table, or database to be
+ reindexed. Index and table names can be schema-qualified.
+ Presently, REINDEX DATABASE and REINDEX SYSTEM
+ can only reindex the current database, so their parameter must match
+ the current database's name.
+
+
+
+
+
+boolean
+
+
+ Specifies whether the selected option should be turned on or off.
+ You can write TRUE, ON, or
+ 1 to enable the option, and FALSE,
+ OFF, or 0 to disable it. The
+ boolean value can also
+ be omitted, in which case TRUE is assumed.
+
+
+
+
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 0d647e912c..4a3678831d 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -35,6 +35,7 @@
#include "catalog/pg_am.h"
#include "catalog/toasting.h"
#include "commands/cluster.h"
+#include "commands/defrem.h"
#include "commands/progress.h"
#include "commands/tablecmds.h"
#include "commands/vacuum.h"
@@ -102,8 +103,29 @@ static List *get_tables_to_cluster(MemoryContext cluster_context);
*---
*/
void
-cluster(ClusterStmt *stmt, bool isTopLevel)
+cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
{
+ ListCell *lc;
+ int options = 0;
+
+ /* Parse list of generic parameters not handled by the parser */
+ foreach(lc, stmt->params)
+ {
+ DefElem *opt = (DefElem *) lfirst(lc);
+
+ if (strcmp(opt->defname, "verbose") == 0)
+ if (defGetBoolean(opt))
+options |= CLUOPT_VERBOSE;
+ else
+options
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Thu, Sep 03, 2020 at 12:00:17AM +0300, Alexey Kondratov wrote:
> On 2020-09-02 07:56, Justin Pryzby wrote:
> >
> > Done in the attached, which is also rebased on 1d6541666.
> >
> > And added RelationAssumeNewRelfilenode() as I mentioned - but I'm hoping
> > to
> > hear from Michael about any reason not to call
> > RelationSetNewRelfilenode()
> > instead of directly calling the things it would itself call.
>
> The latest patch set immediately got a conflict with Michael's fixup
> 01767533, so I've rebased it first of all.
On my side, I've also rearranged function parameters to make the diff more
readable. And squishes your changes into the respective patches.
Michael started a new thread about retiring ReindexStmt->concurrent, which I
guess will cause more conflicts (although I don't see why we wouldn't implement
a generic List grammar now rather than only after a preliminary patch).
--
Justin
>From 704d230463deca5303b0eae6c55e0e197c6fa473 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 27 Mar 2020 17:50:46 -0500
Subject: [PATCH v25 1/6] Change REINDEX/CLUSTER to accept an option list..
..like EXPLAIN (..), VACUUM (..), and ANALYZE (..).
Change docs in the style of VACUUM. See also: 52dcfda48778d16683c64ca4372299a099a15b96
---
doc/src/sgml/ref/cluster.sgml| 27 +---
doc/src/sgml/ref/reindex.sgml| 43 +---
src/backend/commands/cluster.c | 23 -
src/backend/commands/indexcmds.c | 41 --
src/backend/nodes/copyfuncs.c| 2 ++
src/backend/nodes/equalfuncs.c | 2 ++
src/backend/parser/gram.y| 33 +---
src/backend/tcop/utility.c | 36 +++---
src/bin/psql/tab-complete.c | 22
src/include/commands/cluster.h | 3 ++-
src/include/commands/defrem.h| 7 +++---
src/include/nodes/parsenodes.h | 2 ++
12 files changed, 175 insertions(+), 66 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 4da60d8d56..e6ebce27e6 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -21,8 +21,13 @@ PostgreSQL documentation
-CLUSTER [VERBOSE] table_name [ USING index_name ]
-CLUSTER [VERBOSE]
+CLUSTER [VERBOSE] [ ( option [, ...] ) ] table_name [ USING index_name ]
+CLUSTER [VERBOSE] [ ( option [, ...] ) ]
+
+where option can be one of:
+
+VERBOSE [ boolean ]
+
@@ -81,6 +86,15 @@ CLUSTER [VERBOSE]
Parameters
+
+VERBOSE
+
+
+ Prints a progress report as each table is clustered.
+
+
+
+
table_name
@@ -100,10 +114,15 @@ CLUSTER [VERBOSE]
-VERBOSE
+boolean
- Prints a progress report as each table is clustered.
+ Specifies whether the selected option should be turned on or off.
+ You can write TRUE, ON, or
+ 1 to enable the option, and FALSE,
+ OFF, or 0 to disable it. The
+ boolean value can also
+ be omitted, in which case TRUE is assumed.
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index c16f223e4e..a32e192a87 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -25,7 +25,7 @@ REINDEX [ ( option [, ...] ) ] { IN
where option can be one of:
-VERBOSE
+VERBOSE [ boolean ]
@@ -141,19 +141,6 @@ REINDEX [ ( option [, ...] ) ] { IN
-
-name
-
-
- The name of the specific index, table, or database to be
- reindexed. Index and table names can be schema-qualified.
- Presently, REINDEX DATABASE and REINDEX SYSTEM
- can only reindex the current database, so their parameter must match
- the current database's name.
-
-
-
-
CONCURRENTLY
@@ -181,6 +168,34 @@ REINDEX [ ( option [, ...] ) ] { IN
+
+
+name
+
+
+ The name of the specific index, table, or database to be
+ reindexed. Index and table names can be schema-qualified.
+ Presently, REINDEX DATABASE and REINDEX SYSTEM
+ can only reindex the current database, so their parameter must match
+ the current database's name.
+
+
+
+
+
+boolean
+
+
+ Specifies whether the selected option should be turned on or off.
+ You can write TRUE, ON, or
+ 1 to enable the option, and FALSE,
+ OFF, or 0 to disable it. The
+ boolean value can also
+ be omitted, in which case TRUE is assumed.
+
+
+
+
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 0d647e912c..a42dfe98f4 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -35,6 +35,7 @@
#include "catalog/pg_am.h"
#include "catalog/toasting.h"
#include "commands/cluster.h"
+#include "commands/defrem.h"
#include "commands/progress.h"
#include "commands/tablecmds.h"
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Sep 01, 2020 at 09:24:10PM -0500, Justin Pryzby wrote:
> On Wed, Sep 02, 2020 at 10:00:12AM +0900, Michael Paquier wrote:
> > On Tue, Sep 01, 2020 at 11:48:30AM -0400, Alvaro Herrera wrote:
> > > On 2020-Sep-01, Justin Pryzby wrote:
> > >> The question isn't whether to use a parenthesized option list. I
> > >> realized that
> > >> long ago (even though Alexey didn't initially like it). Check 0002,
> > >> which gets
> > >> rid of "bool concurrent" in favour of
> > >> stmt->options_CONCURRENT.
> > >
> > > Ah! I see, sorry for the noise. Well, respectfully, having a separate
> > > boolean to store one option when you already have a bitmask for options
> > > is silly.
> >
> > Yeah, I am all for removing "concurrent" from ReindexStmt, but I don't
> > think that the proposed 0002 is that, because it is based on the
> > assumption that we'd want more than just boolean-based options in
> > those statements, and this case is not justified yet except if it
> > becomes possible to enforce tablespaces. At this stage, I think that
> > it is more sensible to just update gram.y and add a
> > REINDEXOPT_CONCURRENTLY. I also think that it would also make sense
> > to pass down "options" within ReindexIndexCallbackState() (for example
> > imagine a SKIP_LOCKED for REINDEX).
>
> Uh, this whole thread is about implementing REINDEX (TABLESPACE foo), and the
> preliminary patch 0001 is to keep separate the tablespace parts of that
> content. 0002 is a minor tangent which I assume would be squished into 0001
> which cleans up historic cruft, using new params in favour of historic
> options.
>
> I think my change is probably incomplete, and ReindexStmt node should not have
> an int options. parse_reindex_params() would parse it into local int *options
> and char **tablespacename params.
Done in the attached, which is also rebased on 1d6541666.
And added RelationAssumeNewRelfilenode() as I mentioned - but I'm hoping to
hear from Michael about any reason not to call RelationSetNewRelfilenode()
instead of directly calling the things it would itself call.
--
Justin
>From 9d881a6aa401cebef0a2ce781d34a2a5ea8ded60 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 27 Mar 2020 17:50:46 -0500
Subject: [PATCH v23 1/5] Change REINDEX/CLUSTER to accept an option list..
..like EXPLAIN (..), VACUUM (..), and ANALYZE (..).
Change docs in the style of VACUUM. See also: 52dcfda48778d16683c64ca4372299a099a15b96
---
doc/src/sgml/ref/cluster.sgml| 28 ++---
doc/src/sgml/ref/reindex.sgml| 43 +---
src/backend/commands/cluster.c | 23 -
src/backend/commands/indexcmds.c | 41 --
src/backend/nodes/copyfuncs.c| 2 ++
src/backend/nodes/equalfuncs.c | 2 ++
src/backend/parser/gram.y| 35 +++---
src/backend/tcop/utility.c | 36 +++---
src/bin/psql/tab-complete.c | 23 +
src/include/commands/cluster.h | 3 ++-
src/include/commands/defrem.h| 7 +++---
src/include/nodes/parsenodes.h | 2 ++
12 files changed, 179 insertions(+), 66 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 4da60d8d56..110fb3083e 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -21,8 +21,13 @@ PostgreSQL documentation
-CLUSTER [VERBOSE] table_name [ USING index_name ]
-CLUSTER [VERBOSE]
+CLUSTER [VERBOSE] [ ( option [, ...] ) ] table_name [ USING index_name ]
+CLUSTER [VERBOSE] [ ( option [, ...] ) ]
+
+where option can be one of:
+
+VERBOSE [ boolean ]
+
@@ -81,6 +86,16 @@ CLUSTER [VERBOSE]
Parameters
+
+VERBOSE
+
+
+ Prints a progress report as each table is clustered.
+
+
+
+
+
table_name
@@ -100,10 +115,15 @@ CLUSTER [VERBOSE]
-VERBOSE
+boolean
- Prints a progress report as each table is clustered.
+ Specifies whether the selected option should be turned on or off.
+ You can write TRUE, ON, or
+ 1 to enable the option, and FALSE,
+ OFF, or 0 to disable it. The
+ boolean value can also
+ be omitted, in which case TRUE is assumed.
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index c16f223e4e..a32e192a87 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -25,7 +25,7 @@ REINDEX [ ( option [, ...] ) ] { IN
where option can be one of:
-VERBOSE
+VERBOSE [ boolean ]
@@ -141,19 +141,6 @@ REINDEX [ ( option [, ...] ) ] { IN
-
-name
-
-
- The name of the specific index, table, or database to be
- reindexed. Index and table names can be schema-qualified.
- Presently, REINDEX DATABASE and REINDEX SYSTEM
- can only reindex the current database, so their parameter must match
- the current database's
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Wed, Sep 02, 2020 at 10:00:12AM +0900, Michael Paquier wrote: > On Tue, Sep 01, 2020 at 11:48:30AM -0400, Alvaro Herrera wrote: > > On 2020-Sep-01, Justin Pryzby wrote: > >> The question isn't whether to use a parenthesized option list. I realized > >> that > >> long ago (even though Alexey didn't initially like it). Check 0002, which > >> gets > >> rid of "bool concurrent" in favour of stmt->options_CONCURRENT. > > > > Ah! I see, sorry for the noise. Well, respectfully, having a separate > > boolean to store one option when you already have a bitmask for options > > is silly. > > Yeah, I am all for removing "concurrent" from ReindexStmt, but I don't > think that the proposed 0002 is that, because it is based on the > assumption that we'd want more than just boolean-based options in > those statements, and this case is not justified yet except if it > becomes possible to enforce tablespaces. At this stage, I think that > it is more sensible to just update gram.y and add a > REINDEXOPT_CONCURRENTLY. I also think that it would also make sense > to pass down "options" within ReindexIndexCallbackState() (for example > imagine a SKIP_LOCKED for REINDEX). Uh, this whole thread is about implementing REINDEX (TABLESPACE foo), and the preliminary patch 0001 is to keep separate the tablespace parts of that content. 0002 is a minor tangent which I assume would be squished into 0001 which cleans up historic cruft, using new params in favour of historic options. I think my change is probably incomplete, and ReindexStmt node should not have an int options. parse_reindex_params() would parse it into local int *options and char **tablespacename params. -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Sep 01, 2020 at 09:29:28PM -0400, Alvaro Herrera wrote: > Seems sensible, but only to be done when actually needed, right? Of course. -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-Sep-02, Michael Paquier wrote: > Yeah, I am all for removing "concurrent" from ReindexStmt, but I don't > think that the proposed 0002 is that, because it is based on the > assumption that we'd want more than just boolean-based options in > those statements, and this case is not justified yet except if it > becomes possible to enforce tablespaces. At this stage, I think that > it is more sensible to just update gram.y and add a > REINDEXOPT_CONCURRENTLY. Yes, agreed. I had not seen the "params" business. > I also think that it would also make sense to pass down "options" > within ReindexIndexCallbackState() (for example imagine a SKIP_LOCKED > for REINDEX). Seems sensible, but only to be done when actually needed, right? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Sep 01, 2020 at 11:48:30AM -0400, Alvaro Herrera wrote: > On 2020-Sep-01, Justin Pryzby wrote: >> The question isn't whether to use a parenthesized option list. I realized >> that >> long ago (even though Alexey didn't initially like it). Check 0002, which >> gets >> rid of "bool concurrent" in favour of stmt->options_CONCURRENT. > > Ah! I see, sorry for the noise. Well, respectfully, having a separate > boolean to store one option when you already have a bitmask for options > is silly. Yeah, I am all for removing "concurrent" from ReindexStmt, but I don't think that the proposed 0002 is that, because it is based on the assumption that we'd want more than just boolean-based options in those statements, and this case is not justified yet except if it becomes possible to enforce tablespaces. At this stage, I think that it is more sensible to just update gram.y and add a REINDEXOPT_CONCURRENTLY. I also think that it would also make sense to pass down "options" within ReindexIndexCallbackState() (for example imagine a SKIP_LOCKED for REINDEX). -- Michael signature.asc Description: PGP signature
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-Sep-01, Justin Pryzby wrote: > On Tue, Sep 01, 2020 at 11:40:18AM -0400, Alvaro Herrera wrote: > > The advantage of using a parenthesized option list is that you can add > > *further* options without making the new keywords reserved. Of course, > > we already reserve CONCURRENTLY and VERBOSE pretty severely, so there's > > no change. If you wanted REINDEX FLUFFY then it wouldn't work without > > making that at least type_func_name_keyword I think; but REINDEX > > (FLUFFY) would work just fine. And of course the new feature at hand > > can be implemented. > > The question isn't whether to use a parenthesized option list. I realized > that > long ago (even though Alexey didn't initially like it). Check 0002, which > gets > rid of "bool concurrent" in favour of stmt->options_CONCURRENT. Ah! I see, sorry for the noise. Well, respectfully, having a separate boolean to store one option when you already have a bitmask for options is silly. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On Tue, Sep 01, 2020 at 11:40:18AM -0400, Alvaro Herrera wrote: > On 2020-Aug-11, Justin Pryzby wrote: > > On Tue, Aug 11, 2020 at 02:39:45PM +0900, Michael Paquier wrote: > > > > The grammar that has been committed was the one that for the most > > > support, so we need to live with that. I wonder if we should simplify > > > ReindexStmt and move the "concurrent" flag to be under "options", but > > > that may not be worth the time spent on as long as we don't have > > > CONCURRENTLY part of the parenthesized grammar. > > > > I think it's kind of a good idea, since the next patch does exactly that > > (parenthesize (CONCURRENTLY)). > > > > I included that as a new 0002, but it doesn't save anything though, so maybe > > it's not a win. > > The advantage of using a parenthesized option list is that you can add > *further* options without making the new keywords reserved. Of course, > we already reserve CONCURRENTLY and VERBOSE pretty severely, so there's > no change. If you wanted REINDEX FLUFFY then it wouldn't work without > making that at least type_func_name_keyword I think; but REINDEX > (FLUFFY) would work just fine. And of course the new feature at hand > can be implemented. The question isn't whether to use a parenthesized option list. I realized that long ago (even though Alexey didn't initially like it). Check 0002, which gets rid of "bool concurrent" in favour of stmt->options_CONCURRENT. -- Justin
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-Aug-11, Justin Pryzby wrote: > On Tue, Aug 11, 2020 at 02:39:45PM +0900, Michael Paquier wrote: > > The grammar that has been committed was the one that for the most > > support, so we need to live with that. I wonder if we should simplify > > ReindexStmt and move the "concurrent" flag to be under "options", but > > that may not be worth the time spent on as long as we don't have > > CONCURRENTLY part of the parenthesized grammar. > > I think it's kind of a good idea, since the next patch does exactly that > (parenthesize (CONCURRENTLY)). > > I included that as a new 0002, but it doesn't save anything though, so maybe > it's not a win. The advantage of using a parenthesized option list is that you can add *further* options without making the new keywords reserved. Of course, we already reserve CONCURRENTLY and VERBOSE pretty severely, so there's no change. If you wanted REINDEX FLUFFY then it wouldn't work without making that at least type_func_name_keyword I think; but REINDEX (FLUFFY) would work just fine. And of course the new feature at hand can be implemented. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
On 2020-09-01 13:12, Justin Pryzby wrote: This patch seems to be missing a call to RelationAssumeNewRelfilenode() in reindex_index(). That's maybe the related to the cause of the crashes I pointed out earlier this year. Alexey's v4 patch changed RelationSetNewRelfilenode() to accept a tablespace parameter, but Michael seemed to object to that. However that seems cleaner and ~30 line shorter. Michael, would you comment on that ? The v4 patch and your comments are here. https://www.postgresql.org/message-id/attachment/105574/v4-0001-Allow-REINDEX-and-REINDEX-CONCURRENTLY-to-change-tablespace.patch https://www.postgresql.org/message-id/20191127035416.GG5435%40paquier.xyz Actually, the last time we discussed this point I only got the gut feeling that this is a subtle place and it is very easy to break things with these changes. However, it isn't clear for me how exactly. That way, I'd be glad if Michael could reword his explanation, so it'd more clear for me as well. BTW, I've started doing a review of the last patch set yesterday and will try to post some comments later. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
