Re: alter table set TABLE ACCESS METHOD
On Tue, Aug 10, 2021 at 12:24:13PM +0900, Michael Paquier wrote: > So, on a closer look, it happens that this breaks the regression tests > of sepgsql, as the two following commands in ddl.sql cause a rewrite: > ALTER TABLE regtest_table_4 ALTER COLUMN y TYPE float; > ALTER TABLE regtest_ptable_4 ALTER COLUMN y TYPE float; rhinoceros has reported back, and these are the only two that required an adjustment, so fixed. -- Michael signature.asc Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Sat, Aug 07, 2021 at 07:18:19PM +0900, Michael Paquier wrote: > As a matter of curiosity, I have checked how it would look to handle > the no-op case for the sub-commands other than SET TABLESPACE, and one > would need something like the attached, with a new flag for > AlteredTableInfo. That does not really look good, but it triggers > properly the object access hook when SET LOGGED/UNLOGGED/ACCESS METHOD > are no-ops, so let's just handle the case using the version from > upthread. I'll do that at the beginning of next week. So, on a closer look, it happens that this breaks the regression tests of sepgsql, as the two following commands in ddl.sql cause a rewrite: ALTER TABLE regtest_table_4 ALTER COLUMN y TYPE float; ALTER TABLE regtest_ptable_4 ALTER COLUMN y TYPE float; I have been fighting with SE/Linux for a couple of hours to try to figure out how to run our regression tests, but gave up after running into various segmentation faults even after following all the steps of the documentation to set up things. Perhaps that's because I just set up the environment with a recent Debian, I don't know. Instead of running those tests, I have fallen back to my own module and ran all the SQLs of sepgsql to find out places where there are rewrites where I spotted those two places. One thing I have noticed is that sepgsql-regtest.te fails to compile because /usr/share/selinux/devel/Makefile does not understand auth_read_passwd(). Looking at some of the SE/Linux repos, perhaps this ought to be auth_read_shadow() instead to be able to work with a newer version? Anyway, as the addition of this InvokeObjectPostAlterHook() is consistent for a rewrite caused by SET LOGGED/UNLOGGED/ACCESS METHOD I have applied the patch. I'll fix rhinoceros once it reports back the diffs in output. -- Michael signature.asc Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Fri, Jul 30, 2021 at 02:18:02PM -0700, Jeff Davis wrote:
> It sounds like anything we do here should be part of a larger change to
> make it consistent. So I'm fine with the patch you posted.
As a matter of curiosity, I have checked how it would look to handle
the no-op case for the sub-commands other than SET TABLESPACE, and one
would need something like the attached, with a new flag for
AlteredTableInfo. That does not really look good, but it triggers
properly the object access hook when SET LOGGED/UNLOGGED/ACCESS METHOD
are no-ops, so let's just handle the case using the version from
upthread. I'll do that at the beginning of next week.
--
Michael
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fcd778c62a..eecbde8479 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -179,6 +179,8 @@ typedef struct AlteredTableInfo
Oid newAccessMethod; /* new access method; 0 means no change */
Oid newTableSpace; /* new tablespace; 0 means no change */
bool chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
+ bool rewriteAttempted; /* T if a rewrite operation was attempted,
+ * may be a no-op */
char newrelpersistence; /* if above is true */
Expr *partition_constraint; /* for attach partition validation */
/* true, if validating default due to some other attach/detach */
@@ -4593,6 +4595,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_SetLogged: /* SET LOGGED */
ATSimplePermissions(cmd->subtype, rel, ATT_TABLE);
+ tab->rewriteAttempted = true;
if (tab->chgPersistence)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -4608,6 +4611,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_SetUnLogged: /* SET UNLOGGED */
ATSimplePermissions(cmd->subtype, rel, ATT_TABLE);
+ tab->rewriteAttempted = true;
if (tab->chgPersistence)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -5475,6 +5479,8 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
RecentXmin,
ReadNextMultiXactId(),
persistence);
+
+ InvokeObjectPostAlterHook(RelationRelationId, tab->relid, 0);
}
else
{
@@ -5493,6 +5499,16 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
*/
if (tab->newTableSpace)
ATExecSetTableSpace(tab->relid, tab->newTableSpace, lockmode);
+ else if (tab->rewriteAttempted)
+ {
+/*
+ * Even if no rewrite is going to happen, it may be possible
+ * that one has gone through SET LOGGED/UNLOGGED or ACCESS
+ * METHOD while being a no-op. If that's the case, invoke the
+ * access hook.
+ */
+InvokeObjectPostAlterHook(RelationRelationId, tab->relid, 0);
+ }
}
}
@@ -5971,6 +5987,7 @@ ATGetQueueEntry(List **wqueue, Relation rel)
tab->newTableSpace = InvalidOid;
tab->newrelpersistence = RELPERSISTENCE_PERMANENT;
tab->chgPersistence = false;
+ tab->rewriteAttempted = false;
*wqueue = lappend(*wqueue, tab);
@@ -13660,6 +13677,7 @@ ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname)
/* Check that the table access method exists */
amoid = get_table_am_oid(amname, false);
+ tab->rewriteAttempted = true;
if (rel->rd_rel->relam == amoid)
return;
signature.asc
Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Fri, 2021-07-30 at 16:22 +0900, Michael Paquier wrote: > Looking at the past, it was the intention of 05f3f9c7 to go through > the hook even if SET TABLESPACE does not move the relation, so you > are > right that ALTER TABLE is inconsistent to not do the same for LOGGED, > UNLOGGED and ACCESS METHOD if all of them do nothing to trigger a > relation rewrite. > > Now, I am a bit biased about this change and if we actually need it > for the no-op path. If we were to do that, I think that we need to > add in AlteredTableInfo a way to track down if any of those > subcommands have been used to allow the case of rewrite == 0 to > launch > the hook even if these are no-ops. And I am not sure if that's worth > the code complication for an edge case. We definitely should have a > hook call for the case of rewrite > 0, though. It sounds like anything we do here should be part of a larger change to make it consistent. So I'm fine with the patch you posted. Regards, Jeff Davis
Re: alter table set TABLE ACCESS METHOD
On Thu, Jul 29, 2021 at 08:55:21AM -0700, Jeff Davis wrote: > I see that ATExecSetTableSpace() also invokes the hook even for a no- > op. Should we do the same thing for setting the AM? Looking at the past, it was the intention of 05f3f9c7 to go through the hook even if SET TABLESPACE does not move the relation, so you are right that ALTER TABLE is inconsistent to not do the same for LOGGED, UNLOGGED and ACCESS METHOD if all of them do nothing to trigger a relation rewrite. Now, I am a bit biased about this change and if we actually need it for the no-op path. If we were to do that, I think that we need to add in AlteredTableInfo a way to track down if any of those subcommands have been used to allow the case of rewrite == 0 to launch the hook even if these are no-ops. And I am not sure if that's worth the code complication for an edge case. We definitely should have a hook call for the case of rewrite > 0, though. -- Michael signature.asc Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Thu, 2021-07-29 at 15:27 +0900, Michael Paquier wrote: > Doing any checks around the hooks of objectaccess.h is very annoying, > because we have no modules to check after them easily except sepgsql. > Anyway, I have been checking that, with the hack-ish module attached > and tracked down that swap_relation_files() calls > InvokeObjectPostAlterHookArg() already (as you already spotted?), but > that's an internal change when it comes to SET LOGGED/UNLOGGED/ACCESS > METHOD :( > > Attached is a small module I have used for those tests, for > reference. It passes on HEAD, and with the patch attached you can > see > the extra entries. I see that ATExecSetTableSpace() also invokes the hook even for a no- op. Should we do the same thing for setting the AM? > > > > Also, I agree with Justin that it should fail when there are > > > > multiple > > > > SET ACCESS METHOD subcommands consistently, regardless of > > > > whether > > > > one > > > > is a no-op, and it should probably throw a syntax error to > > > > match > > > > SET > > > > TABLESPACE. > > > > > > Hmm. Okay. > > I'd still disagree with that. OK, I won't press for a change here. Regards, Jeff Davis
Re: alter table set TABLE ACCESS METHOD
On Wed, Jul 28, 2021 at 01:05:10PM -0700, Jeff Davis wrote:
> On Wed, 2021-07-28 at 14:02 +0900, Michael Paquier wrote:
>> Right. Isn't that an older issue though? A rewrite involved after a
>> change of relpersistence does not call the hook either. It looks to
>> me that this should be after finish_heap_swap() to match with
>> ATExecSetTableSpace() in ATRewriteTables(). The only known user of
>> object_access_hook in the core code is sepgsql, so this would
>> involve a change of behavior. And I don't recall any backpatching
>> that added a post-alter hook.
>
> Sounds like it should be a different patch. Thank you.
Doing any checks around the hooks of objectaccess.h is very annoying,
because we have no modules to check after them easily except sepgsql.
Anyway, I have been checking that, with the hack-ish module attached
and tracked down that swap_relation_files() calls
InvokeObjectPostAlterHookArg() already (as you already spotted?), but
that's an internal change when it comes to SET LOGGED/UNLOGGED/ACCESS
METHOD :(
Attached is a small module I have used for those tests, for
reference. It passes on HEAD, and with the patch attached you can see
the extra entries.
>>> Also, I agree with Justin that it should fail when there are
>>> multiple
>>> SET ACCESS METHOD subcommands consistently, regardless of whether
>>> one
>>> is a no-op, and it should probably throw a syntax error to match
>>> SET
>>> TABLESPACE.
>>
>> Hmm. Okay.
I'd still disagree with that. One example is SET LOGGED that would
not fail when repeated multiple times. Also, tracking down if a SET
ACCESS METHOD subcommand has been passed down requires an extra field
in each tab entry so I am not sure that this is worth the extra
complication.
I can see benefits and advantages one way or the other, and I would
tend to keep the code a maximum simple as we never store InvalidOid
for a table AM. Anyway, I won't fight the majority either.
>>> Minor nit: in tab-complete.c, why does it say ""? Is that just
>>> a
>>> typo or is there a reason it's different from everything else,
>>> which
>>> uses ""? And what does "sth" mean anyway?
>>
>> "Something". That should be "" to be consistent with the area.
>
> These two issues are pretty minor.
Fixed this one, while not forgetting about it. Thanks.
--
Michael
object_hooks.tar.gz
Description: application/gzip
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fcd778c62a..b18de38e73 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5475,6 +5475,8 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
RecentXmin,
ReadNextMultiXactId(),
persistence);
+
+ InvokeObjectPostAlterHook(RelationRelationId, tab->relid, 0);
}
else
{
signature.asc
Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Wed, 2021-07-28 at 14:02 +0900, Michael Paquier wrote: > Arg, sorry about that! I was unclear what the situation of the patch > was. No problem, race condition ;-) > Right. Isn't that an older issue though? A rewrite involved after a > change of relpersistence does not call the hook either. It looks to > me that this should be after finish_heap_swap() to match with > ATExecSetTableSpace() in ATRewriteTables(). The only known user of > object_access_hook in the core code is sepgsql, so this would > involve a change of behavior. And I don't recall any backpatching > that added a post-alter hook. Sounds like it should be a different patch. Thank you. > > Also, I agree with Justin that it should fail when there are > > multiple > > SET ACCESS METHOD subcommands consistently, regardless of whether > > one > > is a no-op, and it should probably throw a syntax error to match > > SET > > TABLESPACE. > > Hmm. Okay. > > > Minor nit: in tab-complete.c, why does it say ""? Is that just > > a > > typo or is there a reason it's different from everything else, > > which > > uses ""? And what does "sth" mean anyway? > > "Something". That should be "" to be consistent with the area. These two issues are pretty minor. Regards, Jeff Davis
Re: alter table set TABLE ACCESS METHOD
On Tue, Jul 27, 2021 at 08:40:59PM -0700, Jeff Davis wrote: > I just returned from vacation and I was about ready to commit this > myself, but I noticed that it doesn't seem to be calling > InvokeObjectPostAlterHook(). Arg, sorry about that! I was unclear what the situation of the patch was. > I was in the process of trying to be sure > of where to call it. It looks like it should be called after catalog > changes but before CommandCounterIncrement(), and it also looks like it > should be called even for no-op commands. Right. Isn't that an older issue though? A rewrite involved after a change of relpersistence does not call the hook either. It looks to me that this should be after finish_heap_swap() to match with ATExecSetTableSpace() in ATRewriteTables(). The only known user of object_access_hook in the core code is sepgsql, so this would involve a change of behavior. And I don't recall any backpatching that added a post-alter hook. > Also, I agree with Justin that it should fail when there are multiple > SET ACCESS METHOD subcommands consistently, regardless of whether one > is a no-op, and it should probably throw a syntax error to match SET > TABLESPACE. Hmm. Okay. > Minor nit: in tab-complete.c, why does it say ""? Is that just a > typo or is there a reason it's different from everything else, which > uses ""? And what does "sth" mean anyway? "Something". That should be "" to be consistent with the area. -- Michael signature.asc Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Wed, 2021-07-28 at 12:23 +0900, Michael Paquier wrote: > On Tue, Jul 27, 2021 at 04:38:48PM +0900, Michael Paquier wrote: > > Okay, hearing nothing, I have looked again at 0001 and did some > > light > > adjustments, mainly in the tests. I did not spot any issues in the > > patch, so that looks good to go for me. > > And done as of b048326. I just returned from vacation and I was about ready to commit this myself, but I noticed that it doesn't seem to be calling InvokeObjectPostAlterHook(). I was in the process of trying to be sure of where to call it. It looks like it should be called after catalog changes but before CommandCounterIncrement(), and it also looks like it should be called even for no-op commands. Also, I agree with Justin that it should fail when there are multiple SET ACCESS METHOD subcommands consistently, regardless of whether one is a no-op, and it should probably throw a syntax error to match SET TABLESPACE. Minor nit: in tab-complete.c, why does it say ""? Is that just a typo or is there a reason it's different from everything else, which uses ""? And what does "sth" mean anyway? Regards, Jeff Davis
Re: alter table set TABLE ACCESS METHOD
On Tue, Jul 27, 2021 at 04:38:48PM +0900, Michael Paquier wrote: > Okay, hearing nothing, I have looked again at 0001 and did some light > adjustments, mainly in the tests. I did not spot any issues in the > patch, so that looks good to go for me. And done as of b048326. -- Michael signature.asc Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Thu, Jul 22, 2021 at 04:41:54AM -0500, Justin Pryzby wrote:
> It looks like one hunk was missing/uncommitted from the 0002 patch..
Okay, hearing nothing, I have looked again at 0001 and did some light
adjustments, mainly in the tests. I did not spot any issues in the
patch, so that looks good to go for me.
--
Michael
From 19a658eaa3db26cc6fc47305740035d665648b68 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 27 Jul 2021 16:37:43 +0900
Subject: [PATCH] ALTER TABLE ... SET ACCESS METHOD
Adds support for changing the access method of a table with a
rewrite.
Author: Justin Pryzby, Jeff Davis
---
src/include/commands/cluster.h | 4 +-
src/include/commands/event_trigger.h| 1 +
src/include/nodes/parsenodes.h | 1 +
src/backend/commands/cluster.c | 21 +---
src/backend/commands/matview.c | 5 +-
src/backend/commands/tablecmds.c| 66 +++--
src/backend/parser/gram.y | 8 +++
src/bin/psql/tab-complete.c | 11 -
src/test/regress/expected/create_am.out | 34 +
src/test/regress/sql/create_am.sql | 17 +++
doc/src/sgml/ref/alter_table.sgml | 20
11 files changed, 173 insertions(+), 15 deletions(-)
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index a941f2accd..b64d3bc204 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -35,8 +35,8 @@ 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);
-extern Oid make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
- LOCKMODE lockmode);
+extern Oid make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, Oid NewAccessMethod,
+ char relpersistence, LOCKMODE lockmode);
extern void finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
bool is_system_catalog,
bool swap_toast_by_content,
diff --git a/src/include/commands/event_trigger.h b/src/include/commands/event_trigger.h
index c11bf2d781..e765e67fd1 100644
--- a/src/include/commands/event_trigger.h
+++ b/src/include/commands/event_trigger.h
@@ -32,6 +32,7 @@ typedef struct EventTriggerData
#define AT_REWRITE_ALTER_PERSISTENCE 0x01
#define AT_REWRITE_DEFAULT_VAL 0x02
#define AT_REWRITE_COLUMN_REWRITE 0x04
+#define AT_REWRITE_ACCESS_METHOD 0x08
/*
* EventTriggerData is the node type that is passed as fmgr "context" info
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 947660a4b0..e28248af32 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1901,6 +1901,7 @@ typedef enum AlterTableType
AT_SetLogged,/* SET LOGGED */
AT_SetUnLogged,/* SET UNLOGGED */
AT_DropOids,/* SET WITHOUT OIDS */
+ AT_SetAccessMethod, /* SET ACCESS METHOD */
AT_SetTableSpace, /* SET TABLESPACE */
AT_SetRelOptions, /* SET (...) -- AM specific parameters */
AT_ResetRelOptions, /* RESET (...) -- AM specific parameters */
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 6487a9e3fc..b3d8b6deb0 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -576,6 +576,7 @@ static void
rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
{
Oid tableOid = RelationGetRelid(OldHeap);
+ Oid accessMethod = OldHeap->rd_rel->relam;
Oid tableSpace = OldHeap->rd_rel->reltablespace;
Oid OIDNewHeap;
char relpersistence;
@@ -597,6 +598,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
/* Create the transient table that will receive the re-ordered data */
OIDNewHeap = make_new_heap(tableOid, tableSpace,
+ accessMethod,
relpersistence,
AccessExclusiveLock);
@@ -618,16 +620,16 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
/*
* Create the transient table that will be filled with new data during
* CLUSTER, ALTER TABLE, and similar operations. The transient table
- * duplicates the logical structure of the OldHeap, but is placed in
- * NewTableSpace which might be different from OldHeap's. Also, it's built
- * with the specified persistence, which might differ from the original's.
+ * duplicates the logical structure of the OldHeap; but will have the
+ * specified physical storage properties NewTableSpace, NewAccessMethod, and
+ * relpersistence.
*
* After this, the caller should load the new heap with transferred/modified
* data, then call finish_heap_swap to complete the operation.
*/
Oid
-make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
- LOCKMODE lockmode)
+make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, Oid NewAccessMethod,
+ char relpersistence, LOCKMODE lockmode)
{
TupleDesc OldHeapDesc;
char NewHeapName[NAMEDATALEN];
@@ -686,7 +688,7 @@ make_new_heap(Oid OIDOl
Re: alter table set TABLE ACCESS METHOD
On Thu, Jul 22, 2021 at 10:37:12AM +0530, vignesh C wrote:
> On Fri, Jul 16, 2021 at 9:19 AM Justin Pryzby wrote:
> >
> > rebased.
> >
> > Also, there were two redundant checks for multiple SET ACCESS METHOD
> > commands.
> > But one of them wasn't hit if the ALTER was setting the current AM due to
> > the
> > no-op test.
> >
> > I think it's better to fail in every case, and not just sometimes
> > (especially
> > if we were to use ERRCODE_SYNTAX_ERROR).
> >
> > I included my 2ndary patch allowing to set the AM of partitioned table,
> > same as
> > for a tablespace.
>
> One of the tests is failing, please post an updated patch for this:
> create_am.out 2021-07-22 10:34:56.234654166 +0530
It looks like one hunk was missing/uncommitted from the 0002 patch..
--
Justin
>From 2e6748cadc56fad2a29a5cec895eb6796e890198 Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Wed, 5 May 2021 14:28:59 -0700
Subject: [PATCH 1/2] ALTER TABLE ... SET ACCESS METHOD
Adds support for changing the access method of a table with a
rewrite.
Author: Justin Pryzby, Jeff Davis
---
doc/src/sgml/ref/alter_table.sgml | 20
src/backend/commands/cluster.c | 21 +---
src/backend/commands/matview.c | 5 +-
src/backend/commands/tablecmds.c| 68 +++--
src/backend/parser/gram.y | 8 +++
src/bin/psql/tab-complete.c | 10 ++--
src/include/commands/cluster.h | 4 +-
src/include/commands/event_trigger.h| 1 +
src/include/nodes/parsenodes.h | 1 +
src/test/regress/expected/create_am.out | 34 +
src/test/regress/sql/create_am.sql | 21
11 files changed, 175 insertions(+), 18 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c7f48938b..bf90040aa2 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -75,6 +75,7 @@ ALTER TABLE [ IF EXISTS ] name
CLUSTER ON index_name
SET WITHOUT CLUSTER
SET WITHOUT OIDS
+SET ACCESS METHOD new_access_method
SET TABLESPACE new_tablespace
SET { LOGGED | UNLOGGED }
SET ( storage_parameter [= value] [, ... ] )
@@ -692,6 +693,16 @@ WITH ( MODULUS numeric_literal, REM
+
+SET ACCESS METHOD
+
+
+ This form changes the access method of the table by rewriting it. See
+ for more information.
+
+
+
+
SET TABLESPACE
@@ -1228,6 +1239,15 @@ WITH ( MODULUS numeric_literal, REM
+
+ new_access_method
+
+
+The name of the access method to which the table will be converted.
+
+
+
+
new_tablespace
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 2912b4c257..45bf1276a5 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -641,6 +641,7 @@ static void
rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
{
Oid tableOid = RelationGetRelid(OldHeap);
+ Oid accessMethod = OldHeap->rd_rel->relam;
Oid tableSpace = OldHeap->rd_rel->reltablespace;
Oid OIDNewHeap;
char relpersistence;
@@ -661,6 +662,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
/* Create the transient table that will receive the re-ordered data */
OIDNewHeap = make_new_heap(tableOid, tableSpace,
+ accessMethod,
relpersistence,
AccessExclusiveLock);
@@ -682,16 +684,16 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
/*
* Create the transient table that will be filled with new data during
* CLUSTER, ALTER TABLE, and similar operations. The transient table
- * duplicates the logical structure of the OldHeap, but is placed in
- * NewTableSpace which might be different from OldHeap's. Also, it's built
- * with the specified persistence, which might differ from the original's.
+ * duplicates the logical structure of the OldHeap; but will have the
+ * specified physical storage properties NewTableSpace, NewAccessMethod, and
+ * relpersistence.
*
* After this, the caller should load the new heap with transferred/modified
* data, then call finish_heap_swap to complete the operation.
*/
Oid
-make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
- LOCKMODE lockmode)
+make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, Oid NewAccessMethod,
+ char relpersistence, LOCKMODE lockmode)
{
TupleDesc OldHeapDesc;
char NewHeapName[NAMEDATALEN];
@@ -750,7 +752,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
InvalidOid,
InvalidOid,
OldHeap->rd_rel->relowner,
- OldHeap->rd_rel->relam,
+ NewAccessMethod,
OldHeapDesc,
NIL,
RELKIND_RELATION,
@@ -1100,6 +1102,10 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
relform1->reltables
Re: alter table set TABLE ACCESS METHOD
On Fri, Jul 16, 2021 at 9:19 AM Justin Pryzby wrote:
>
> rebased.
>
> Also, there were two redundant checks for multiple SET ACCESS METHOD commands.
> But one of them wasn't hit if the ALTER was setting the current AM due to the
> no-op test.
>
> I think it's better to fail in every case, and not just sometimes (especially
> if we were to use ERRCODE_SYNTAX_ERROR).
>
> I included my 2ndary patch allowing to set the AM of partitioned table, same
> as
> for a tablespace.
One of the tests is failing, please post an updated patch for this:
create_am.out 2021-07-22 10:34:56.234654166 +0530
@@ -177,6 +177,7 @@
(1 row)
-- CREATE TABLE .. PARTITION BY supports USING
+-- new partitions will inherit from the current default, rather the
partition root
CREATE TABLE tableam_parted_heap2 (a text, b int) PARTITION BY list
(a) USING heap2;
SET default_table_access_method = 'heap';
CREATE TABLE tableam_parted_a_heap2 PARTITION OF tableam_parted_heap2
FOR VALUES IN ('a');
Regards,
Vignesh
Re: alter table set TABLE ACCESS METHOD
On Thu, Jul 15, 2021 at 10:49:23PM -0500, Justin Pryzby wrote:
> Also, there were two redundant checks for multiple SET ACCESS METHOD commands.
> But one of them wasn't hit if the ALTER was setting the current AM due to the
> no-op test.
Yep.
> I think it's better to fail in every case, and not just sometimes (especially
> if we were to use ERRCODE_SYNTAX_ERROR).
Looks rather fine.
- if (tab->newTableSpace)
+ if (OidIsValid(tab->newTableSpace))
This has no need to be part of this patch.
/*
-* If we have ALTER TABLE SET TABLESPACE provide a list of
-* tablespaces
+* Complete with list of tablespaces (for SET TABLESPACE) or table AMs (for
+* SET ACCESS METHOD).
*/
+ else if (Matches("ALTER", "TABLE", MatchAny, "SET", "ACCESS", "METHOD"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_table_access_methods);
else if (Matches("ALTER", "TABLE", MatchAny, "SET", "TABLESPACE"))
COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
Nit, there is no need to merge both block here. Let's keep them
separated.
+-- negative test
[...]
+-- negative test
Those descriptions could be better, and describe what they prevent
(aka no multiple subcommands SET ACCESS METHOD and not allowed on
partitioned tables).
> I included my 2ndary patch allowing to set the AM of partitioned table, same
> as
> for a tablespace.
I would suggest to not hide this topic within a thread unrelated to
it, as this is not going to ease the feedback around it. Let's start
a new thread if you feel this is necessary.
Jeff, you proposed to commit this patch upthread. Are you planning to
look at that and do so?
--
Michael
signature.asc
Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
rebased.
Also, there were two redundant checks for multiple SET ACCESS METHOD commands.
But one of them wasn't hit if the ALTER was setting the current AM due to the
no-op test.
I think it's better to fail in every case, and not just sometimes (especially
if we were to use ERRCODE_SYNTAX_ERROR).
I included my 2ndary patch allowing to set the AM of partitioned table, same as
for a tablespace.
--
Justin
>From a20b924adbcc6e24a88f8d9bd0287c62456720a3 Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Wed, 5 May 2021 14:28:59 -0700
Subject: [PATCH 1/2] ALTER TABLE ... SET ACCESS METHOD
Adds support for changing the access method of a table with a
rewrite.
Author: Justin Pryzby, Jeff Davis
---
doc/src/sgml/ref/alter_table.sgml | 20
src/backend/commands/cluster.c | 21 +---
src/backend/commands/matview.c | 5 +-
src/backend/commands/tablecmds.c| 68 +++--
src/backend/parser/gram.y | 8 +++
src/bin/psql/tab-complete.c | 10 ++--
src/include/commands/cluster.h | 4 +-
src/include/commands/event_trigger.h| 1 +
src/include/nodes/parsenodes.h | 1 +
src/test/regress/expected/create_am.out | 34 +
src/test/regress/sql/create_am.sql | 21
11 files changed, 175 insertions(+), 18 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index c5e5e84e06..1d0f30d366 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -75,6 +75,7 @@ ALTER TABLE [ IF EXISTS ] name
CLUSTER ON index_name
SET WITHOUT CLUSTER
SET WITHOUT OIDS
+SET ACCESS METHOD new_access_method
SET TABLESPACE new_tablespace
SET { LOGGED | UNLOGGED }
SET ( storage_parameter [= value] [, ... ] )
@@ -692,6 +693,16 @@ WITH ( MODULUS numeric_literal, REM
+
+SET ACCESS METHOD
+
+
+ This form changes the access method of the table by rewriting it. See
+ for more information.
+
+
+
+
SET TABLESPACE
@@ -1228,6 +1239,15 @@ WITH ( MODULUS numeric_literal, REM
+
+ new_access_method
+
+
+The name of the access method to which the table will be converted.
+
+
+
+
new_tablespace
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 2912b4c257..45bf1276a5 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -641,6 +641,7 @@ static void
rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
{
Oid tableOid = RelationGetRelid(OldHeap);
+ Oid accessMethod = OldHeap->rd_rel->relam;
Oid tableSpace = OldHeap->rd_rel->reltablespace;
Oid OIDNewHeap;
char relpersistence;
@@ -661,6 +662,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
/* Create the transient table that will receive the re-ordered data */
OIDNewHeap = make_new_heap(tableOid, tableSpace,
+ accessMethod,
relpersistence,
AccessExclusiveLock);
@@ -682,16 +684,16 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
/*
* Create the transient table that will be filled with new data during
* CLUSTER, ALTER TABLE, and similar operations. The transient table
- * duplicates the logical structure of the OldHeap, but is placed in
- * NewTableSpace which might be different from OldHeap's. Also, it's built
- * with the specified persistence, which might differ from the original's.
+ * duplicates the logical structure of the OldHeap; but will have the
+ * specified physical storage properties NewTableSpace, NewAccessMethod, and
+ * relpersistence.
*
* After this, the caller should load the new heap with transferred/modified
* data, then call finish_heap_swap to complete the operation.
*/
Oid
-make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
- LOCKMODE lockmode)
+make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, Oid NewAccessMethod,
+ char relpersistence, LOCKMODE lockmode)
{
TupleDesc OldHeapDesc;
char NewHeapName[NAMEDATALEN];
@@ -750,7 +752,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
InvalidOid,
InvalidOid,
OldHeap->rd_rel->relowner,
- OldHeap->rd_rel->relam,
+ NewAccessMethod,
OldHeapDesc,
NIL,
RELKIND_RELATION,
@@ -1100,6 +1102,10 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
relform1->reltablespace = relform2->reltablespace;
relform2->reltablespace = swaptemp;
+ swaptemp = relform1->relam;
+ relform1->relam = relform2->relam;
+ relform2->relam = swaptemp;
+
swptmpchr = relform1->relpersistence;
relform1->relpersistence = relform2->relpersistence;
relform2->relpersistence = swptmpchr;
@@ -1135,6 +1141,9 @@ swap_relation_files(Oid r1, Oid r2, bool targe
Re: alter table set TABLE ACCESS METHOD
On Thu, Jun 10, 2021 at 1:01 AM Jeff Davis wrote:
>
> On Wed, 2021-06-09 at 13:47 +0900, Michael Paquier wrote:
> > There is a mix of upper and lower-case characters here. It could be
> > more consistent. It seems to me that this test should actually check
> > that pg_class.relam reflects the new value.
>
> Done. I also added a (negative) test for changing the AM of a
> partitioned table, which wasn't caught before.
>
> > +errmsg("cannot have multiple SET ACCESS METHOD
> > subcommands")));
> > Worth adding a test?
>
> Done.
>
> > Nit: the name of the variable looks inconsistent with this comment.
> > The original is weird as well.
>
> Tried to improve wording.
>
> > I am wondering if it would be a good idea to set the new tablespace
> > and new access method fields to InvalidOid within
> > ATGetQueueEntry. We
> > do that for the persistence. Not critical at all, still..
>
> Done.
>
> > + pass = AT_PASS_MISC;
> > Maybe add a comment here?
>
> Done. In that case, it doesn't matter because there's no work to be
> done in Phase 2.
>
There are few compilation issues:
tablecmds.c:4629:52: error: too few arguments to function call,
expected 3, have 2
ATSimplePermissions(rel, ATT_TABLE | ATT_MATVIEW);
~~~ ^
tablecmds.c:402:1: note: 'ATSimplePermissions' declared here
static void ATSimplePermissions(AlterTableType cmdtype, Relation rel,
int allowed_targets);
^
tablecmds.c:5983:10: warning: enumeration value 'AT_SetAccessMethod'
not handled in switch [-Wswitch]
switch (cmdtype)
^
1 warning and 1 error generated.
Also few comments need to be addressed, based on that I'm changing the
status to "Waiting for Author".
Regards,
Vignesh
Re: alter table set TABLE ACCESS METHOD
On Wed, Jun 09, 2021 at 01:45:52PM -0700, Zhihong Yu wrote:
> + /* check if another access method change was already requested
> */
> + if (tab->newAccessMethod)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +errmsg("cannot change access method setting
> twice")));
>
> I think the error message can be refined - changing access method twice is
> supported, as long as the two changes don't overlap.
That language is consistent wtih existing errors.
src/backend/commands/tablecmds.c:
errmsg("cannot change persistence setting twice")));
src/backend/commands/tablecmds.c:
errmsg("cannot change persistence setting twice")));
errmsg("cannot alter type of column \"%s\"
twice",
However, I think that SET TABLESPACE is a better template to follow:
errmsg("cannot have multiple SET TABLESPACE
subcommands")));
Michael pointed out that there's two, redundant checks:
+ if (rel->rd_rel->relam == amoid)
+ return;
+
+ /* Save info for Phase 3 to do the real work */
+ if (OidIsValid(tab->newAccessMethod))
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("cannot have multiple SET ACCESS METHOD
subcommands")));
I think that the "multiple subcommands" test should be before the "no-op" test.
@Jeff: In my original patch, I also proposed patches 2,3:
Subject: [PATCH v2 2/3] Allow specifying acccess method of partitioned tables..
Subject: [PATCH v2 3/3] Implement lsyscache get_rel_relam()
Re: alter table set TABLE ACCESS METHOD
On Wed, Jun 09, 2021 at 01:47:18PM +0900, Michael Paquier wrote: > On Tue, Jun 08, 2021 at 05:33:31PM -0700, Jeff Davis wrote: > > New version attached, with the detoasting code removed. Could use > > another round of validation/cleanup in case I missed something during > > the merge. > > This looks rather sane to me, thanks. > > /* Create the transient table that will receive the re-ordered data */ > OIDNewHeap = make_new_heap(tableOid, tableSpace, > + accessMethod > It strikes me that we could extend CLUSTER/VACUUM FULL to support this > option, in the same vein as TABLESPACE. Perhaps that's not something to > implement or have, just wanted to mention it. It's a good thought. But remember that that c5b28604 handled REINDEX (TABLESPACE) but not CLUSTER/VACUUM FULL (TABLESPACE). You wrote: https://www.postgresql.org/message-id/YBuWbzoW6W7AaMv0%40paquier.xyz > 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. In any case, I think we really want to have an ALTER .. SET ACCESS METHOD. Supporting it also in CLUSTER/VACUUM is an optional, additional feature. -- Justin
Re: alter table set TABLE ACCESS METHOD
On Wed, Jun 09, 2021 at 01:45:52PM -0700, Zhihong Yu wrote:
> + /* check if another access method change was already requested
> */
> + if (tab->newAccessMethod)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +errmsg("cannot change access method setting
> twice")));
>
> I think the error message can be refined - changing access method twice is
> supported, as long as the two changes don't overlap.
Hmm. Do we actually need this one? ATPrepSetAccessMethod() checks
already that this command cannot be specified multiple times, with an
error message consistent with what SET TABLESPACE does.
--
Michael
signature.asc
Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Wed, Jun 9, 2021 at 12:31 PM Jeff Davis wrote:
> On Wed, 2021-06-09 at 13:47 +0900, Michael Paquier wrote:
> > There is a mix of upper and lower-case characters here. It could be
> > more consistent. It seems to me that this test should actually check
> > that pg_class.relam reflects the new value.
>
> Done. I also added a (negative) test for changing the AM of a
> partitioned table, which wasn't caught before.
>
> > +errmsg("cannot have multiple SET ACCESS METHOD
> > subcommands")));
> > Worth adding a test?
>
> Done.
>
> > Nit: the name of the variable looks inconsistent with this comment.
> > The original is weird as well.
>
> Tried to improve wording.
>
> > I am wondering if it would be a good idea to set the new tablespace
> > and new access method fields to InvalidOid within
> > ATGetQueueEntry. We
> > do that for the persistence. Not critical at all, still..
>
> Done.
>
> > + pass = AT_PASS_MISC;
> > Maybe add a comment here?
>
> Done. In that case, it doesn't matter because there's no work to be
> done in Phase 2.
>
> Regards,
> Jeff Davis
>
> Hi,
+ /* check if another access method change was already requested
*/
+ if (tab->newAccessMethod)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("cannot change access method setting
twice")));
I think the error message can be refined - changing access method twice is
supported, as long as the two changes don't overlap.
Cheers
Re: alter table set TABLE ACCESS METHOD
On Wed, 2021-06-09 at 13:47 +0900, Michael Paquier wrote:
> There is a mix of upper and lower-case characters here. It could be
> more consistent. It seems to me that this test should actually check
> that pg_class.relam reflects the new value.
Done. I also added a (negative) test for changing the AM of a
partitioned table, which wasn't caught before.
> +errmsg("cannot have multiple SET ACCESS METHOD
> subcommands")));
> Worth adding a test?
Done.
> Nit: the name of the variable looks inconsistent with this comment.
> The original is weird as well.
Tried to improve wording.
> I am wondering if it would be a good idea to set the new tablespace
> and new access method fields to InvalidOid within
> ATGetQueueEntry. We
> do that for the persistence. Not critical at all, still..
Done.
> + pass = AT_PASS_MISC;
> Maybe add a comment here?
Done. In that case, it doesn't matter because there's no work to be
done in Phase 2.
Regards,
Jeff Davis
From 051d067e07eaec29d221641da3bf28d0dd0731c8 Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Wed, 5 May 2021 14:28:59 -0700
Subject: [PATCH] ALTER TABLE ... SET ACCESS METHOD
Adds support for changing the access method of a table with a
rewrite.
Author: Justin Pryzby, Jeff Davis
---
doc/src/sgml/ref/alter_table.sgml | 20 +++
src/backend/commands/cluster.c | 21 +---
src/backend/commands/matview.c | 5 +-
src/backend/commands/tablecmds.c| 71 +++--
src/backend/parser/gram.y | 8 +++
src/bin/psql/tab-complete.c | 10 ++--
src/include/commands/cluster.h | 4 +-
src/include/commands/event_trigger.h| 1 +
src/include/nodes/parsenodes.h | 1 +
src/test/regress/expected/create_am.out | 34
src/test/regress/sql/create_am.sql | 21
11 files changed, 178 insertions(+), 18 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 939d3fe2739..5ac13a5c0f6 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -75,6 +75,7 @@ ALTER TABLE [ IF EXISTS ] name
CLUSTER ON index_name
SET WITHOUT CLUSTER
SET WITHOUT OIDS
+SET ACCESS METHOD new_access_method
SET TABLESPACE new_tablespace
SET { LOGGED | UNLOGGED }
SET ( storage_parameter [= value] [, ... ] )
@@ -693,6 +694,16 @@ WITH ( MODULUS numeric_literal, REM
+
+SET ACCESS METHOD
+
+
+ This form changes the access method of the table by rewriting it. See
+ for more information.
+
+
+
+
SET TABLESPACE
@@ -1229,6 +1240,15 @@ WITH ( MODULUS numeric_literal, REM
+
+ new_access_method
+
+
+The name of the access method to which the table will be converted.
+
+
+
+
new_tablespace
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 6487a9e3fcb..b3d8b6deb03 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -576,6 +576,7 @@ static void
rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
{
Oid tableOid = RelationGetRelid(OldHeap);
+ Oid accessMethod = OldHeap->rd_rel->relam;
Oid tableSpace = OldHeap->rd_rel->reltablespace;
Oid OIDNewHeap;
char relpersistence;
@@ -597,6 +598,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
/* Create the transient table that will receive the re-ordered data */
OIDNewHeap = make_new_heap(tableOid, tableSpace,
+ accessMethod,
relpersistence,
AccessExclusiveLock);
@@ -618,16 +620,16 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
/*
* Create the transient table that will be filled with new data during
* CLUSTER, ALTER TABLE, and similar operations. The transient table
- * duplicates the logical structure of the OldHeap, but is placed in
- * NewTableSpace which might be different from OldHeap's. Also, it's built
- * with the specified persistence, which might differ from the original's.
+ * duplicates the logical structure of the OldHeap; but will have the
+ * specified physical storage properties NewTableSpace, NewAccessMethod, and
+ * relpersistence.
*
* After this, the caller should load the new heap with transferred/modified
* data, then call finish_heap_swap to complete the operation.
*/
Oid
-make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
- LOCKMODE lockmode)
+make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, Oid NewAccessMethod,
+ char relpersistence, LOCKMODE lockmode)
{
TupleDesc OldHeapDesc;
char NewHeapName[NAMEDATALEN];
@@ -686,7 +688,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
InvalidOid,
InvalidOid,
OldHeap->rd_rel->relowner,
- OldHeap->rd_rel->relam,
+
Re: alter table set TABLE ACCESS METHOD
On Tue, Jun 08, 2021 at 05:33:31PM -0700, Jeff Davis wrote:
> New version attached, with the detoasting code removed. Could use
> another round of validation/cleanup in case I missed something during
> the merge.
This looks rather sane to me, thanks.
/* Create the transient table that will receive the re-ordered data */
OIDNewHeap = make_new_heap(tableOid, tableSpace,
+ accessMethod
It strikes me that we could extend CLUSTER/VACUUM FULL to support this
option, in the same vein as TABLESPACE. Perhaps that's not something to
implement or have, just wanted to mention it.
+ALTER TABLE heaptable SET ACCESS METHOD heap2;
+explain (analyze, costs off, summary off, timing off) SELECT * FROM heaptable;
+SELECT COUNT(a), COUNT(1) FILTER(WHERE a=1) FROM heaptable;
+DROP TABLE heaptable;
There is a mix of upper and lower-case characters here. It could be
more consistent. It seems to me that this test should actually check
that pg_class.relam reflects the new value.
+ /* Save info for Phase 3 to do the real work */
+ if (OidIsValid(tab->newAccessMethod))
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("cannot have multiple SET ACCESS METHOD subcommands")));
Worth adding a test?
- * with the specified persistence, which might differ from the original's.
+ * NewTableSpace/accessMethod/persistence, which might differ from those
Nit: the name of the variable looks inconsistent with this comment.
The original is weird as well.
I am wondering if it would be a good idea to set the new tablespace
and new access method fields to InvalidOid within ATGetQueueEntry. We
do that for the persistence. Not critical at all, still..
+ pass = AT_PASS_MISC;
Maybe add a comment here?
--
Michael
signature.asc
Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Thu, 2021-06-03 at 14:36 -0700, Jeff Davis wrote:
> Do we have general agreement on this point? Did I miss another
> purpose
> of detoasting in tablecmds.c, or can we just remove that part of the
> patch?
Per discussion with Justin, I'll drive this patch. I merged the CF
entries into https://commitfest.postgresql.org/33/3110/
New version attached, with the detoasting code removed. Could use
another round of validation/cleanup in case I missed something during
the merge.
Regards,
Jeff Davis
From 9356b096eb79e34eb5f2740fbff506c43c81843d Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Wed, 5 May 2021 14:28:59 -0700
Subject: [PATCH] ALTER TABLE ... SET ACCESS METHOD
Adds support for changing the access method of a table with a
rewrite.
Author: Justin Pryzby, Jeff Davis
---
doc/src/sgml/ref/alter_table.sgml | 20 +
src/backend/commands/cluster.c | 19 +---
src/backend/commands/matview.c | 5 ++-
src/backend/commands/tablecmds.c| 60 +++--
src/backend/parser/gram.y | 8
src/bin/psql/tab-complete.c | 10 +++--
src/include/commands/cluster.h | 4 +-
src/include/commands/event_trigger.h| 1 +
src/include/nodes/parsenodes.h | 1 +
src/test/regress/expected/create_am.out | 16 +++
src/test/regress/sql/create_am.sql | 6 +++
11 files changed, 133 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 939d3fe2739..5ac13a5c0f6 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -75,6 +75,7 @@ ALTER TABLE [ IF EXISTS ] name
CLUSTER ON index_name
SET WITHOUT CLUSTER
SET WITHOUT OIDS
+SET ACCESS METHOD new_access_method
SET TABLESPACE new_tablespace
SET { LOGGED | UNLOGGED }
SET ( storage_parameter [= value] [, ... ] )
@@ -693,6 +694,16 @@ WITH ( MODULUS numeric_literal, REM
+
+SET ACCESS METHOD
+
+
+ This form changes the access method of the table by rewriting it. See
+ for more information.
+
+
+
+
SET TABLESPACE
@@ -1229,6 +1240,15 @@ WITH ( MODULUS numeric_literal, REM
+
+ new_access_method
+
+
+The name of the access method to which the table will be converted.
+
+
+
+
new_tablespace
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 6487a9e3fcb..2e5c58cf5e4 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -576,6 +576,7 @@ static void
rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
{
Oid tableOid = RelationGetRelid(OldHeap);
+ Oid accessMethod = OldHeap->rd_rel->relam;
Oid tableSpace = OldHeap->rd_rel->reltablespace;
Oid OIDNewHeap;
char relpersistence;
@@ -597,6 +598,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
/* Create the transient table that will receive the re-ordered data */
OIDNewHeap = make_new_heap(tableOid, tableSpace,
+ accessMethod,
relpersistence,
AccessExclusiveLock);
@@ -619,15 +621,15 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
* Create the transient table that will be filled with new data during
* CLUSTER, ALTER TABLE, and similar operations. The transient table
* duplicates the logical structure of the OldHeap, but is placed in
- * NewTableSpace which might be different from OldHeap's. Also, it's built
- * with the specified persistence, which might differ from the original's.
+ * NewTableSpace/accessMethod/persistence, which might differ from those
+ * of the OldHeap.
*
* After this, the caller should load the new heap with transferred/modified
* data, then call finish_heap_swap to complete the operation.
*/
Oid
-make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
- LOCKMODE lockmode)
+make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, Oid NewAccessMethod,
+ char relpersistence, LOCKMODE lockmode)
{
TupleDesc OldHeapDesc;
char NewHeapName[NAMEDATALEN];
@@ -686,7 +688,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
InvalidOid,
InvalidOid,
OldHeap->rd_rel->relowner,
- OldHeap->rd_rel->relam,
+ NewAccessMethod,
OldHeapDesc,
NIL,
RELKIND_RELATION,
@@ -1036,6 +1038,10 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
relform1->reltablespace = relform2->reltablespace;
relform2->reltablespace = swaptemp;
+ swaptemp = relform1->relam;
+ relform1->relam = relform2->relam;
+ relform2->relam = swaptemp;
+
swptmpchr = relform1->relpersistence;
relform1->relpersistence = relform2->relpersistence;
relform2->relpersistence = swptmpchr;
@@ -1071,6 +1077,9 @@ swap_relation_files(Oid r1, Oid r
Re: alter table set TABLE ACCESS METHOD
On Fri, Jun 04, 2021 at 05:34:36PM -0700, Jeff Davis wrote: > I agree that a dummy AM would be good, but implementing even a dummy AM > is a fair amount of work. Not much, honestly, the largest part being to document that properly so as it could be used as a template: https://www.postgresql.org/message-id/YEXm2nh/5j5p2...@paquier.xyz > Also, there are many potential variations, so > we'd probably need several. Not so sure here. GUCs or reloptions could be used to control some of the behaviors. Now this really depends on the use-cases we are looking to support here and the low-level facilities that could benefit from this module (dummy_index_am tests reloptions for example). I agree that this thread is perhaps not enough to justify adding this module for now. > The table AM API is a work in progress, and I think it will be a few > releases (and require a few more table AMs in the wild) to really nail > down the API. Hard to say, we'll see. I'd like to believe that it could be a good to not set something in stone for that forever. -- Michael signature.asc Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Sat, 2021-06-05 at 08:45 +0900, Michael Paquier wrote: > One thing I am wondering is if we should have a dummy_table_am in > src/test/modules/ to be able to stress more this feature. That does > not seem like a hard requirement, but relying only on heap limits a > bit the coverage of this feature even if one changes > default_table_access_method. I agree that a dummy AM would be good, but implementing even a dummy AM is a fair amount of work. Also, there are many potential variations, so we'd probably need several. The table AM API is a work in progress, and I think it will be a few releases (and require a few more table AMs in the wild) to really nail down the API. Regards, Jeff Davis
Re: alter table set TABLE ACCESS METHOD
On Fri, Jun 04, 2021 at 11:26:28AM -0700, Jeff Davis wrote: > Yes. That's a current requirement, and any AM that doesn't do that is > already broken (e.g. for INSERT INTO ... SELECT). Makes sense. I was just looking at the patch, and this was the only part of it that made my spidey sense react. One thing I am wondering is if we should have a dummy_table_am in src/test/modules/ to be able to stress more this feature. That does not seem like a hard requirement, but relying only on heap limits a bit the coverage of this feature even if one changes default_table_access_method. -- Michael signature.asc Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Fri, 2021-06-04 at 14:58 +0900, Michael Paquier wrote: > In short, a table AMs would receive on a rewrite with ALTER TABLE > tuples which may be toasted, still table_insert_tuple() should be > able > to handle both: > - the case where this tuple was already toasted. > - the case where this tuple has been already detoasted. Yes. That's a current requirement, and any AM that doesn't do that is already broken (e.g. for INSERT INTO ... SELECT). Regards, Jeff Davis
Re: alter table set TABLE ACCESS METHOD
On Thu, Jun 03, 2021 at 02:36:15PM -0700, Jeff Davis wrote: > Do we have general agreement on this point? Did I miss another purpose > of detoasting in tablecmds.c, or can we just remove that part of the > patch? Catching up with this thread.. So, what you are suggesting here is that we have no need to let ATRewriteTable() do anything about the detoasting, and just push down the responsability of detoasting the tuple, if necessary, down to the AM layer where the tuple insertion is handled, right? In short, a table AMs would receive on a rewrite with ALTER TABLE tuples which may be toasted, still table_insert_tuple() should be able to handle both: - the case where this tuple was already toasted. - the case where this tuple has been already detoasted. You are right that this would be more consistent with what heap does with heap_prepare_insert(). -- Michael signature.asc Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Thu, 2021-05-06 at 17:24 -0700, Jeff Davis wrote: > It's the table AM's responsibility to detoast out-of-line datums and > toast any values that are too large (see > heapam.c:heap_prepare_insert()). Do we have general agreement on this point? Did I miss another purpose of detoasting in tablecmds.c, or can we just remove that part of the patch? Regards, Jeff Davis
Re: alter table set TABLE ACCESS METHOD
On Thu, 2021-05-06 at 17:19 -0500, Justin Pryzby wrote: > If ATRewriteTable didn't > handle this, > data would become inaccessible if an AM failed to de-toast tuples. If the AM fails to detoast tuples, it's got bigger problems than ALTER TABLE. What about INSERT INTO ... SELECT? It's the table AM's responsibility to detoast out-of-line datums and toast any values that are too large (see heapam.c:heap_prepare_insert()). Regards, Jeff Davis
Re: alter table set TABLE ACCESS METHOD
On Thu, May 06, 2021 at 02:11:31PM -0700, Jeff Davis wrote: > On Thu, 2021-05-06 at 15:23 -0500, Justin Pryzby wrote: > > I think ALTER TABLE SET ACCESS METHOD should move all data off the > > old AM, > > including its toast table. > > Can you explain what you mean, and why? I'm still confused. > > Let's say there are 4 table AMs: A, AT, B, and BT. A's > relation_toast_am() returns AT, and B's relation_toast_am() returns BT. > AT or BT are invalid if A or B have relation_needs_toast_table() return > false. > > Here are the cases that I see: > > If A = B, then AT = BT, and it's all a no-op. > > If A != B and BT is invalid (e.g. converting heap to columnar), then A > should detoast (and perhaps decompress, as in the case of columnar) > whatever it gets as input and do whatever it wants. That's what > columnar does and I don't see why ATRewriteTable needs to handle it. > > If A != B and AT != BT, then B needs to detoast whatever it gets (but > should not decompress, as that would just be wasted effort), and then > re-toast using BT. Again, I don't see a need for ATRewriteTable to do > anything, B can handle it. > > The only case I can really see where ATRewriteTable might be helpful is > if A != B but AT = BT. In that case, in theory, you don't need to do > anything to the toast table, just leave it where it is. But then the > responsibilities get a little confusing to me -- how is B supposed to > know that it doesn't need to toast anything? Is this the problem you > are trying to solve? That's the optimization Michael is suggesting. I was approaching this after having just looked at Dilip's patch (which was originally written using pg_am to support "pluggable" compression "AM"s, but not otherwise related to table AM). Once a table is converted to a new AM, its tuples had better not reference the old AM - it could be dropped. The new table AM (B) shouldn't need to know anything about the old one (A). It should just process incoming tuples. It makes more to me that ATRewriteTable would handle this, rather than every acccess method having the same logic (at best) or different logic (more likely). If ATRewriteTable didn't handle this, data would become inaccessible if an AM failed to de-toast tuples. -- Justin
Re: alter table set TABLE ACCESS METHOD
On Thu, 2021-05-06 at 15:23 -0500, Justin Pryzby wrote: > I think ALTER TABLE SET ACCESS METHOD should move all data off the > old AM, > including its toast table. Can you explain what you mean, and why? I'm still confused. Let's say there are 4 table AMs: A, AT, B, and BT. A's relation_toast_am() returns AT, and B's relation_toast_am() returns BT. AT or BT are invalid if A or B have relation_needs_toast_table() return false. Here are the cases that I see: If A = B, then AT = BT, and it's all a no-op. If A != B and BT is invalid (e.g. converting heap to columnar), then A should detoast (and perhaps decompress, as in the case of columnar) whatever it gets as input and do whatever it wants. That's what columnar does and I don't see why ATRewriteTable needs to handle it. If A != B and AT != BT, then B needs to detoast whatever it gets (but should not decompress, as that would just be wasted effort), and then re-toast using BT. Again, I don't see a need for ATRewriteTable to do anything, B can handle it. The only case I can really see where ATRewriteTable might be helpful is if A != B but AT = BT. In that case, in theory, you don't need to do anything to the toast table, just leave it where it is. But then the responsibilities get a little confusing to me -- how is B supposed to know that it doesn't need to toast anything? Is this the problem you are trying to solve? Regards, Jeff Davis
Re: alter table set TABLE ACCESS METHOD
On Thu, May 06, 2021 at 01:10:53PM -0700, Jeff Davis wrote: > On Mon, 2021-03-08 at 16:30 +0900, Michael Paquier wrote: > > This toast issue is a kind of interesting one, and it seems rather > > right to rely on toast_build_flattened_tuple() to decompress things > > if > > both table AMs support toast with the internals of toast knowing what > > kind of compression has been applied to the stored tuple, rather than > > have the table AM try to extra a toast tuple by itself. I wonder > > whether we should have a table AM API to know what kind of > > compression > > is supported for a given table AMs at hand, because there is no need > > to flatten things if both the origin and the target match their > > compression algos, which would be on HEAD to make sure that both the > > origin and table AMs have toast (relation_toast_am). Your patch, > > here, would flatten each tuples as long as the table AMs don't > > match. That can be made cheaper in some cases. > > I am confused by this. The toast-related table AM API functions are: > relation_needs_toast_table(), relation_toast_am(), and > relation_fetch_toast_slice(). I wrote this shortly after looking at one of Dilip's LZ4 patches. At one point in February/March, pg_attribute.attcompression defined the compression used by *all* tuples in the table, rather than the compression used for new tuples, and ALTER SET COMPRESSION would rewrite the table with the new compression (rather than being only a catalog update). > What cases are we trying to solve here? > > 1. target of conversion is tableam1 main table, heap toast table > 2. target of conversion is tableam1 main table, no toast table > 3. target of conversion is tableam1 main table, tableam1 toast table > 4. target of conversion is tableam1 main table, tableam2 toast table I think ALTER TABLE SET ACCESS METHOD should move all data off the old AM, including its toast table. The optimization Michael suggests is when the new AM and old AM use the same toast AM, then the data doesn't need to be de/re/toasted. Thanks for looking. -- Justin
Re: alter table set TABLE ACCESS METHOD
On Mon, 2021-03-08 at 16:30 +0900, Michael Paquier wrote: > This toast issue is a kind of interesting one, and it seems rather > right to rely on toast_build_flattened_tuple() to decompress things > if > both table AMs support toast with the internals of toast knowing what > kind of compression has been applied to the stored tuple, rather than > have the table AM try to extra a toast tuple by itself. I wonder > whether we should have a table AM API to know what kind of > compression > is supported for a given table AMs at hand, because there is no need > to flatten things if both the origin and the target match their > compression algos, which would be on HEAD to make sure that both the > origin and table AMs have toast (relation_toast_am). Your patch, > here, would flatten each tuples as long as the table AMs don't > match. That can be made cheaper in some cases. I am confused by this. The toast-related table AM API functions are: relation_needs_toast_table(), relation_toast_am(), and relation_fetch_toast_slice(). What cases are we trying to solve here? 1. target of conversion is tableam1 main table, heap toast table 2. target of conversion is tableam1 main table, no toast table 3. target of conversion is tableam1 main table, tableam1 toast table 4. target of conversion is tableam1 main table, tableam2 toast table Or does the problem apply to all of these cases? And if tableam1 can't handle some case, why can't it just detoast the data itself? Shouldn't that be able to decompress anything? For example, in columnar[1], we just always detoast/decompress because we want to compress it ourselves (along with other values from the same column), and we never have a separate toast table. Is that code incorrect, or will it break in v14? Regards, Jeff Davis https://github.com/citusdata/citus/blob/6b1904d37a18e2975b46f0955076f84c8a387cc6/src/backend/columnar/columnar_tableam.c#L1433
Re: alter table set TABLE ACCESS METHOD
On Mon, Mar 08, 2021 at 04:30:23PM +0900, Michael Paquier wrote: > This toast issue is a kind of interesting one, and it seems rather > right to rely on toast_build_flattened_tuple() to decompress things if > both table AMs support toast with the internals of toast knowing what > kind of compression has been applied to the stored tuple, rather than > have the table AM try to extra a toast tuple by itself. I wonder > whether we should have a table AM API to know what kind of compression > is supported for a given table AMs at hand, because there is no need > to flatten things if both the origin and the target match their > compression algos, which would be on HEAD to make sure that both the > origin and table AMs have toast (relation_toast_am). Your patch, > here, would flatten each tuples as long as the table AMs don't > match. That can be made cheaper in some cases. I actually have an idea for this one, able to test the decompression -> insert path when rewriting a relation with a new AM: we could add a dummy_table_am in src/test/modules/. By design, this table AM acts as a blackhole, eating any data we insert into it but print into the logs the data candidate for INSERT. When doing a heap -> dummy_table_am rewrite, the logs would provide coverage with the data from the origin toast table. The opposite operation does not really matter, though it could be tested. In one of my trees, I have something already close to that: https://github.com/michaelpq/pg_plugins/tree/master/blackhole_am/ -- Michael signature.asc Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Sun, Mar 07, 2021 at 07:07:07PM -0600, Justin Pryzby wrote:
> This renames to use SET ACCESS METHOD (resolving a silly typo);
> And handles the tuple slots more directly;
> And adds docs and tab completion;
>
> Also, since 8586bf7ed8889f39a59dd99b292014b73be85342:
> |For now it's not allowed to set a table AM for a partitioned table, as
> |we've not resolved how partitions would inherit that. Disallowing
> |allows us to introduce, if we decide that's the way forward, such a
> |behaviour without a compatibility break.
>
> I propose that it should behave like tablespace for partitioned rels:
> ca4103025dfe, 33e6c34c3267
Sounds sensible from here. Based on the patch at hand, changing the
AM of a partitioned table does nothing for the existing partitions,
and newly-created partitions would inherit the new AM assigned to its
parent. pg_dump is handling things right.
From what I can see, the patch in itself is simple, with central parts
in swap_relation_files() to handle the rewrite and make_new_heap() to
assign the new correct AM.
+ * Since these have no storage the tablespace can be updated with a simple
+ * metadata only operation to update the tablespace.
+ */
+static void
+ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethod
This comment still refers to tablespaces.
+ /*
+* Record dependency on AM. This is only required for relations
+* that have no physical storage.
+*/
+ changeDependencyFor(RelationRelationId, RelationGetRelid(rel),
+ AccessMethodRelationId, oldrelam,
+ newAccessMethod);
And? Relations with storage also require this dependency.
- if (tab->newTableSpace)
+ if (OidIsValid(tab->newTableSpace))
You are right, but this is just a noise diff in this patch.
+ swaptemp = relform1->relam;
+ relform1->relam = relform2->relam;
+ relform2->relam = swaptemp;
swap_relation_files() holds the central logic, but I can see that no
comments of this routine have been updated (header, comment when
looking at valid relfilenode{1,2}).
It seems to me that 0002 and 0003 should just be merged together.
+ /* Need to detoast tuples when changing AM XXX: should
check if one AM is heap and one isn't? */
+ if (newrel->rd_rel->relam != oldrel->rd_rel->relam)
+ {
+ HeapTuple htup = toast_build_flattened_tuple(oldTupDesc,
+ oldslot->tts_values,
+ oldslot->tts_isnull);
This toast issue is a kind of interesting one, and it seems rather
right to rely on toast_build_flattened_tuple() to decompress things if
both table AMs support toast with the internals of toast knowing what
kind of compression has been applied to the stored tuple, rather than
have the table AM try to extra a toast tuple by itself. I wonder
whether we should have a table AM API to know what kind of compression
is supported for a given table AMs at hand, because there is no need
to flatten things if both the origin and the target match their
compression algos, which would be on HEAD to make sure that both the
origin and table AMs have toast (relation_toast_am). Your patch,
here, would flatten each tuples as long as the table AMs don't
match. That can be made cheaper in some cases.
--
Michael
signature.asc
Description: PGP signature
Re: alter table set TABLE ACCESS METHOD
On Mon, Mar 01, 2021 at 11:16:36AM +0900, Michael Paquier wrote:
> On Sun, Feb 28, 2021 at 04:25:30PM -0600, Justin Pryzby wrote:
> > I called this "set TABLE access method" rather than just "set access method"
> > for the reasons given on the LIKE thread:
> > https://www.postgresql.org/message-id/20210119210331.gn8...@telsasoft.com
>
> ALTER TABLE applies to a table (or perhaps a sequence, still..), so
> that sounds a bit weird to me to add again the keyword "TABLE" for
> that.
This renames to use SET ACCESS METHOD (resolving a silly typo);
And handles the tuple slots more directly;
And adds docs and tab completion;
Also, since 8586bf7ed8889f39a59dd99b292014b73be85342:
|For now it's not allowed to set a table AM for a partitioned table, as
|we've not resolved how partitions would inherit that. Disallowing
|allows us to introduce, if we decide that's the way forward, such a
|behaviour without a compatibility break.
I propose that it should behave like tablespace for partitioned rels:
ca4103025dfe, 33e6c34c3267
--
Justin
>From ee9610626927a8438ca1278a891321dc585e9401 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 27 Feb 2021 20:40:54 -0600
Subject: [PATCH v2 1/3] ALTER TABLE SET ACCESS METHOD
This does a tuple-level rewrite to the new AM
---
doc/src/sgml/ref/alter_table.sgml | 11
src/backend/commands/cluster.c | 17 --
src/backend/commands/matview.c | 1 +
src/backend/commands/tablecmds.c| 76 ++---
src/backend/parser/gram.y | 8 +++
src/bin/psql/tab-complete.c | 11 ++--
src/include/commands/cluster.h | 2 +-
src/include/commands/event_trigger.h| 1 +
src/include/nodes/parsenodes.h | 1 +
src/test/regress/expected/create_am.out | 16 ++
src/test/regress/sql/create_am.sql | 6 ++
11 files changed, 131 insertions(+), 19 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index c25ef5abd6..38e416d183 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -74,6 +74,7 @@ ALTER TABLE [ IF EXISTS ] name
CLUSTER ON index_name
SET WITHOUT CLUSTER
SET WITHOUT OIDS
+SET ACCESS METHOD new_access_method
SET TABLESPACE new_tablespace
SET { LOGGED | UNLOGGED }
SET ( storage_parameter [= value] [, ... ] )
@@ -658,6 +659,16 @@ WITH ( MODULUS numeric_literal, REM
+
+SET ACCESS METHOD
+
+
+ This form changes the table's access method to the specified table AM and
+ rewrites the table.
+
+
+
+
SET TABLESPACE
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 096a06f7b3..426a5b83ba 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -577,6 +577,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
{
Oid tableOid = RelationGetRelid(OldHeap);
Oid tableSpace = OldHeap->rd_rel->reltablespace;
+ Oid accessMethod = OldHeap->rd_rel->relam;
Oid OIDNewHeap;
char relpersistence;
bool is_system_catalog;
@@ -598,6 +599,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
/* Create the transient table that will receive the re-ordered data */
OIDNewHeap = make_new_heap(tableOid, tableSpace,
relpersistence,
+ accessMethod,
AccessExclusiveLock);
/* Copy the heap data into the new table in the desired order */
@@ -619,15 +621,15 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
* Create the transient table that will be filled with new data during
* CLUSTER, ALTER TABLE, and similar operations. The transient table
* duplicates the logical structure of the OldHeap, but is placed in
- * NewTableSpace which might be different from OldHeap's. Also, it's built
- * with the specified persistence, which might differ from the original's.
+ * NewTableSpace/accessMethod/persistence, which might differ from those
+ * of the OldHeap.
*
* After this, the caller should load the new heap with transferred/modified
* data, then call finish_heap_swap to complete the operation.
*/
Oid
make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
- LOCKMODE lockmode)
+ Oid relam, LOCKMODE lockmode)
{
TupleDesc OldHeapDesc;
char NewHeapName[NAMEDATALEN];
@@ -686,7 +688,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
InvalidOid,
InvalidOid,
OldHeap->rd_rel->relowner,
- OldHeap->rd_rel->relam,
+ relam,
OldHeapDesc,
NIL,
RELKIND_RELATION,
@@ -1036,6 +1038,10 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
relform1->reltablespace = relform2->reltablespace;
relform2->reltablespace = swaptemp;
+ swaptemp = relform1->relam;
+ relform1->relam = relform2->relam;
+ relform2->relam = sw
Re: alter table set TABLE ACCESS METHOD
On Mon, Mar 01, 2021 at 11:16:36AM +0900, Michael Paquier wrote: > On Sun, Feb 28, 2021 at 04:25:30PM -0600, Justin Pryzby wrote: > > I called this "set TABLE access method" rather than just "set access method" > > for the reasons given on the LIKE thread: > > https://www.postgresql.org/message-id/20210119210331.gn8...@telsasoft.com > > ALTER TABLE applies to a table (or perhaps a sequence, still..), so > that sounds a bit weird to me to add again the keyword "TABLE" for > that. I don't know if you're following along the toast compression patch - Alvaro had suggested that instead of making a new catalog just for a handful of tuples for compression types, to instead store them in pg_am, with a new am_type='c'. So I proposed a patch for | CREATE TABLE .. (LIKE other INCLUDING ACCESS METHOD), but then decided that it should say INCLUDING *TABLE* ACCESS METHOD, since otherwise it was somewhat strange that it didn't include the compression access methods (which have a separate LIKE option). > > I've tested this with zedstore, but the lack of 2nd, in-core table AM limits > > testing possibilities. It also limits at least my own ability to reason > > about > > the AM API. > > > > For example, I was surprised to hear that toast is a concept that's > > intended to be applied to AMs other than heap. > > https://www.postgresql.org/message-id/flat/CA%2BTgmoYTuT4sRtviMLOOO%2B79VnDCpCNyy9rK6UZFb7KEAVt21w%40mail.gmail.com > > What kind of advanced testing do you have in mind? It sounds pretty > much enough to me for a basic patch to use the trick with heap2 as > your patch does. That would be enough to be sure that the rewrite > happens and that data is still around. The issue is that the toast data can be compressed, so it needs to be detoasted before pushing it to the other AM, which otherwise may not know how to decompress it. If it's not detoasted, this works with "COMPRESSION lz4" (since zedstore happens to know how to decompress it) but that's just an accident, and it fails with when using pglz. That's got to do with 2 non-core patches - when core has only heap, then I don't see how something like this can be exercized. postgres=# DROP TABLE t; CREATE TABLE t (a TEXT COMPRESSION pglz) USING heap; INSERT INTO t SELECT repeat(a::text,) FROM generate_series(1,99)a; ALTER TABLE t SET ACCESS METHOD zedstore; SELECT * FROM t; DROP TABLE CREATE TABLE INSERT 0 99 ALTER TABLE 2021-02-28 20:50:42.653 CST client backend[14958] psql ERROR: compressed lz4 data is corrupt 2021-02-28 20:50:42.653 CST client backend[14958] psql STATEMENT: SELECT * FROM t; -- Justin
Re: alter table set TABLE ACCESS METHOD
On Sun, Feb 28, 2021 at 04:25:30PM -0600, Justin Pryzby wrote: > I called this "set TABLE access method" rather than just "set access method" > for the reasons given on the LIKE thread: > https://www.postgresql.org/message-id/20210119210331.gn8...@telsasoft.com ALTER TABLE applies to a table (or perhaps a sequence, still..), so that sounds a bit weird to me to add again the keyword "TABLE" for that. > I've tested this with zedstore, but the lack of 2nd, in-core table AM limits > testing possibilities. It also limits at least my own ability to reason about > the AM API. > > For example, I was surprised to hear that toast is a concept that's > intended to be applied to AMs other than heap. > https://www.postgresql.org/message-id/flat/CA%2BTgmoYTuT4sRtviMLOOO%2B79VnDCpCNyy9rK6UZFb7KEAVt21w%40mail.gmail.com What kind of advanced testing do you have in mind? It sounds pretty much enough to me for a basic patch to use the trick with heap2 as your patch does. That would be enough to be sure that the rewrite happens and that data is still around. If you are worrying about recovery, a TAP test with an immediate stop of the server could equally be used to check after the FPWs produced for the new relfilenode during the rewrite. -- Michael signature.asc Description: PGP signature
alter table set TABLE ACCESS METHOD
o *tab, Relation rel,
ATExecSetTableSpaceNoStorage(rel, tab->newTableSpace);
break;
+ case AT_SetTableAccessMethod: /* SET TABLE ACCESS METHOD */
+ /* Handled specially in Phase 3 */
+ break;
+
case AT_SetRelOptions: /* SET (...) */
case AT_ResetRelOptions: /* RESET (...) */
case AT_ReplaceRelOptions: /* replace entire option list */
@@ -5067,7 +5084,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
/*
* We only need to rewrite the table if at least one column needs to
- * be recomputed, or we are changing its persistence.
+ * be recomputed, or we are changing its persistence or access method.
*
* There are two reasons for requiring a rewrite when changing
* persistence: on one hand, we need to ensure that the buffers
@@ -5082,6 +5099,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
Relation OldHeap;
Oid OIDNewHeap;
Oid NewTableSpace;
+ Oid NewAccessMethod;
char persistence;
OldHeap = table_open(tab->relid, NoLock);
@@ -5116,11 +5134,16 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
* Select destination tablespace (same as original unless user
* requested a change)
*/
- if (tab->newTableSpace)
+ if (OidIsValid(tab->newTableSpace))
NewTableSpace = tab->newTableSpace;
else
NewTableSpace = OldHeap->rd_rel->reltablespace;
+ if (OidIsValid(tab->newAccessMethod))
+NewAccessMethod = tab->newAccessMethod;
+ else
+NewAccessMethod = OldHeap->rd_rel->relam;
+
/*
* Select persistence of transient table (same as original unless
* user requested a change)
@@ -5161,7 +5184,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
* unlogged anyway.
*/
OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, persistence,
- lockmode);
+ NewAccessMethod, lockmode);
/*
* Copy the heap data into the new table with the desired
@@ -5483,13 +5506,35 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
{
/* Extract data from old tuple */
slot_getallattrs(oldslot);
-ExecClearTuple(newslot);
-/* copy attributes */
-memcpy(newslot->tts_values, oldslot->tts_values,
- sizeof(Datum) * oldslot->tts_nvalid);
-memcpy(newslot->tts_isnull, oldslot->tts_isnull,
- sizeof(bool) * oldslot->tts_nvalid);
+/* Need to detoast tuples when changing AM XXX: should check if one AM is heap and one isn't? */
+if (newrel->rd_rel->relam != oldrel->rd_rel->relam)
+{
+ HeapTuple htup = toast_build_flattened_tuple(oldTupDesc,
+ oldslot->tts_values,
+ oldslot->tts_isnull);
+
+ /*
+ * Copy the value/null array to the new slot and materialize it,
+ * before clearing the tuple from the old slot.
+ */
+ ExecClearTuple(newslot);
+ ExecForceStoreHeapTuple(htup, oldslot, true);
+ slot_getallattrs(oldslot); /* again */
+
+ memcpy(newslot->tts_values, oldslot->tts_values, oldslot->tts_nvalid * sizeof(Datum));
+ memcpy(newslot->tts_isnull, oldslot->tts_isnull, oldslot->tts_nvalid * sizeof(bool));
+ ExecStoreVirtualTuple(newslot);
+ ExecMaterializeSlot(newslot);
+ ExecClearTuple(oldslot);
+} else {
+ ExecClearTuple(newslot);
+ /* copy attributes */
+ memcpy(newslot->tts_values, oldslot->tts_values,
+ sizeof(Datum) * oldslot->tts_nvalid);
+ memcpy(newslot->tts_isnull, oldslot->tts_isnull,
+ sizeof(bool) * oldslot->tts_nvalid);
+}
/* Set dropped attributes to null in new tuple */
foreach(lc, dropped_attrs)
@@ -5516,7 +5561,8 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
&newslot->tts_isnull[ex->attnum - 1]);
}
-ExecStoreVirtualTuple(newslot);
+if (newrel->rd_rel->relam == oldrel->rd_rel->relam)
+ ExecStoreVirtualTuple(newslot);
/*
* Now, evaluate any expressions whose inputs come from the
@@ -13057,6 +13103,26 @@ ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, const char *tablespacen
tab->newTableSpace = tablespaceId;
}
+/*
+ * ALTER TABLE SET ACCESS METHOD
+ */
+static void
+ATPrepSeTabletAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname, LOCKMODE lockmode)
+{
+ Oid amoid;
+
+ /* Check that the AM exists */
+ amoid = get_table_am_oid(amname, false);
+
+ /* Save info for Phase 3 to do the real work */
+ if (OidIsValid(tab->newAccessMethod))
+ ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("cannot have multiple SET ACCESS METHOD subcommands")));
+
+ tab->newAccessMethod = amoid;
+}
+
/*
* Set, reset, or replace reloptions.
*/
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
i
