Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Tue, May 21, 2024 at 08:33:51AM -0500, Justin Pryzby wrote:
> It occurred to me that psql \dP+ should show the AM of partitioned
> tables (and other partitioned rels).
> Arguably, this could've been done when \dP was introduced in v12, but
> at that point would've shown the AM only for partitioned indexes.
> But it makes a lot of sense to do it now that partitioned tables support
> AMs. I suggest to consider this for v17.
Not sure that this is a must-have. It is nice to have, but extra
information is a new feature IMO. Any extra opinions?
I would suggest to attach a patch, that makes review easier. And so
here is one.
--
Michael
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f67bf0b892..7c9a1f234c 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -4113,7 +4113,7 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
PQExpBufferData title;
PGresult *res;
printQueryOpt myopt = pset.popt;
- bool translate_columns[] = {false, false, false, false, false, false, false, false, false};
+ bool translate_columns[] = {false, false, false, false, false, false, false, false, false, false};
const char *tabletitle;
bool mixed_output = false;
@@ -4181,6 +4181,13 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
if (verbose)
{
+ /*
+ * Table access methods were introduced in v12, and can be set on
+ * partitioned tables since v17.
+ */
+ appendPQExpBuffer(, ",\n am.amname as \"%s\"",
+ gettext_noop("Access method"));
+
if (showNested)
{
appendPQExpBuffer(,
@@ -4216,6 +4223,9 @@ listPartitionedTables(const char *reltypes, const char *pattern, bool verbose)
if (verbose)
{
+ appendPQExpBufferStr(,
+ "\n LEFT JOIN pg_catalog.pg_am am ON c.relam = am.oid");
+
if (pset.sversion < 12)
{
appendPQExpBufferStr(,
signature.asc
Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Tue, May 21, 2024 at 08:33:51AM -0500, Justin Pryzby wrote: > It occurred to me that psql \dP+ should show the AM of partitioned > tables (and other partitioned rels). ping
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
It occurred to me that psql \dP+ should show the AM of partitioned
tables (and other partitioned rels).
Arguably, this could've been done when \dP was introduced in v12, but
at that point would've shown the AM only for partitioned indexes.
But it makes a lot of sense to do it now that partitioned tables support
AMs. I suggest to consider this for v17.
regression=# \dP+
List of partitioned relations
Schema | Name | Owner | Type| Table |
Access method | Total size | Description
+--+-+---++---++-
public | mlparted | pryzbyj | partitioned table ||
heap2 | 104 kB |
public | tableam_parted_heap2 | pryzbyj | partitioned table ||
| 32 kB |
public | trigger_parted | pryzbyj | partitioned table ||
| 0 bytes|
public | upsert_test | pryzbyj | partitioned table ||
| 8192 bytes |
public | trigger_parted_pkey | pryzbyj | partitioned index | trigger_parted |
btree | 16 kB |
public | upsert_test_pkey | pryzbyj | partitioned index | upsert_test|
btree | 8192 bytes |
---
src/bin/psql/describe.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f67bf0b8925..22a668409e7 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -4113,7 +4113,7 @@ listPartitionedTables(const char *reltypes, const char
*pattern, bool verbose)
PQExpBufferData title;
PGresult *res;
printQueryOpt myopt = pset.popt;
- booltranslate_columns[] = {false, false, false, false,
false, false, false, false, false};
+ booltranslate_columns[] = {false, false, false, false,
false, false, false, false, false, false};
const char *tabletitle;
boolmixed_output = false;
@@ -4181,6 +4181,14 @@ listPartitionedTables(const char *reltypes, const char
*pattern, bool verbose)
if (verbose)
{
+ /*
+* Table access methods were introduced in v12, and can be set
on
+* partitioned tables since v17.
+*/
+ appendPQExpBuffer(,
+ ",\n am.amname as \"%s\"",
+ gettext_noop("Access
method"));
+
if (showNested)
{
appendPQExpBuffer(,
@@ -4216,6 +4224,9 @@ listPartitionedTables(const char *reltypes, const char
*pattern, bool verbose)
if (verbose)
{
+ appendPQExpBufferStr(,
+"\n LEFT JOIN
pg_catalog.pg_am am ON c.relam = am.oid");
+
if (pset.sversion < 12)
{
appendPQExpBufferStr(,
--
2.42.0
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Sat, Apr 20, 2024 at 11:55:46AM +0900, Michael Paquier wrote: > FYI, as this is an open item, I am planning to wrap that at the > beginning of next week after a second lookup. If there are any > comments, feel free. This one is now fixed with f46bee346c3b, and the open item is marked as fixed. -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Fri, Apr 19, 2024 at 10:41:30AM +0900, Michael Paquier wrote: > This comes from the contents of the dump for > regress_pg_dump_table_am_2, that uses heap as table AM. A SET is > issued for it before dumping regress_pg_dump_table_am_parent and its > partitions. One trick that I can think of to make the output parsing > of the test more palatable is to switch the AMs used by the two > partitions, so as we finish with two SET queries before each partition > rather than one before the partitioned table. See the attached for > the idea. FYI, as this is an open item, I am planning to wrap that at the beginning of next week after a second lookup. If there are any comments, feel free. -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Thu, Apr 18, 2024 at 06:17:56PM +0200, Alvaro Herrera wrote: > On 2024-Apr-18, Michael Paquier wrote: >> I was also worrying about a need to dump the protocol version to be >> able to track the relkind in the toc entries, but a45c78e3284b has >> already done one. The difference in AM handling between relations >> without storage and relations with storage pushes the relkind logic >> more within the internals of pg_backup_archiver.c. > > Hmm, does this mean that every dump taking since a45c78e3284b (April > 1st) and before this commit will be unrestorable? This doesn't worry me > too much, because we aren't even in beta yet ... and I think we don't > have a strict policy about it. I've been scanning the history of K_VERS_1_* in the recent years, and it does not seem that we have a case where we would have needed to bump the version twice in the same release cycle. Anyway, yes, any dump taken since 1_16 has been bumped would fail to restore with this patch in place. For an unreleased not-yet-in-beta branch, why should we care? Things are not set in stone, like extensions. If others have comments about this point, feel free of course. >> --- a/src/bin/pg_dump/t/002_pg_dump.pl >> +++ b/src/bin/pg_dump/t/002_pg_dump.pl >> @@ -4591,11 +4591,9 @@ my %tests = ( >> CREATE TABLE dump_test.regress_pg_dump_table_am_child_2 >>PARTITION OF >> dump_test.regress_pg_dump_table_am_parent FOR VALUES IN (2);', >> regexp => qr/^ >> -\QSET default_table_access_method = regress_table_am;\E >> -(\n(?!SET[^;]+;)[^\n]*)* >> -\n\QCREATE TABLE >> dump_test.regress_pg_dump_table_am_parent (\E >> -(.*\n)* >> \QSET default_table_access_method = heap;\E >> +(.*\n)* >> +\QALTER TABLE dump_test.regress_pg_dump_table_am_parent >> SET ACCESS METHOD regress_table_am;\E >> (\n(?!SET[^;]+;)[^\n]*)* >> \n\QCREATE TABLE >> dump_test.regress_pg_dump_table_am_child_1 (\E >> (.*\n)* > > This looks strange -- why did you remove matching for the CREATE TABLE > of the parent table? That line should appear shortly before the ALTER > TABLE SET ACCESS METHOD for the same table, shouldn't it? Yeah, with the ALTER in place that did not seem that mandatory but I don't mind keeping it, as well. > Maybe your > intention was to remove only the SET default_table_access_method > = regress_table_am line ... but it's not clear to me why we have the > "SET default_table_access_method = heap" line before the ALTER TABLE SET > ACCESS METHOD. This comes from the contents of the dump for regress_pg_dump_table_am_2, that uses heap as table AM. A SET is issued for it before dumping regress_pg_dump_table_am_parent and its partitions. One trick that I can think of to make the output parsing of the test more palatable is to switch the AMs used by the two partitions, so as we finish with two SET queries before each partition rather than one before the partitioned table. See the attached for the idea. -- Michael From 009ba9bf376d87ab0aaf0a9e9df380cd801a9c2e Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 19 Apr 2024 10:30:53 +0900 Subject: [PATCH v2] Set properly table AMs of partitioned tables in pg_dump --- src/bin/pg_dump/pg_backup_archiver.c | 70 +++- src/bin/pg_dump/pg_backup_archiver.h | 5 +- src/bin/pg_dump/pg_dump.c| 1 + src/bin/pg_dump/t/002_pg_dump.pl | 12 ++--- 4 files changed, 79 insertions(+), 9 deletions(-) diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index c7a6c918a6..f69f72d731 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -30,6 +30,7 @@ #include #endif +#include "catalog/pg_class_d.h" #include "common/string.h" #include "compress_io.h" #include "dumputils.h" @@ -62,6 +63,8 @@ static void _becomeOwner(ArchiveHandle *AH, TocEntry *te); static void _selectOutputSchema(ArchiveHandle *AH, const char *schemaName); static void _selectTablespace(ArchiveHandle *AH, const char *tablespace); static void _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam); +static void _printTableAccessMethodNoStorage(ArchiveHandle *AH, + TocEntry *te); static void processEncodingEntry(ArchiveHandle *AH, TocEntry *te); static void processStdStringsEntry(ArchiveHandle *AH, TocEntry *te); static void processSearchPathEntry(ArchiveHandle *AH, TocEntry *te); @@ -1222,6 +1225,7 @@ ArchiveEntry(Archive *AHX, CatalogId catalogId, DumpId dumpId, newToc->namespace = opts->namespace ? pg_strdup(opts->namespace) : NULL; newToc->tablespace = opts->tablespace ? pg_strdup(opts->tablespace) : NULL; newToc->tableam = opts->tableam ? pg_strdup(opts->tableam) : NULL; + newToc->relkind = opts->relkind; newToc->owner =
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 2024-Apr-18, Michael Paquier wrote: > On Wed, Apr 17, 2024 at 10:31:52AM +0200, Alvaro Herrera wrote: > > Hmm, maybe we should do a RESET of default_table_access_method before > > printing the CREATE TABLE to avoid the confusion. > > A hard reset would make the business around currTableAM that decides > when to generate the SET default_table_access_method queries slightly > more complicated, while increasing the number of queries run on the > server. Hmm, okay. (I don't think we really care too much about the number of queries, do we?) > Fine by me to use two routines to generate the two different commands. > I am finishing with the attached for now, making dumps, restores and > upgrades work happily as far as I've tested. Great. > I was also worrying about a need to dump the protocol version to be > able to track the relkind in the toc entries, but a45c78e3284b has > already done one. The difference in AM handling between relations > without storage and relations with storage pushes the relkind logic > more within the internals of pg_backup_archiver.c. Hmm, does this mean that every dump taking since a45c78e3284b (April 1st) and before this commit will be unrestorable? This doesn't worry me too much, because we aren't even in beta yet ... and I think we don't have a strict policy about it. > --- a/src/bin/pg_dump/t/002_pg_dump.pl > +++ b/src/bin/pg_dump/t/002_pg_dump.pl > @@ -4591,11 +4591,9 @@ my %tests = ( > CREATE TABLE dump_test.regress_pg_dump_table_am_child_2 > PARTITION OF > dump_test.regress_pg_dump_table_am_parent FOR VALUES IN (2);', > regexp => qr/^ > - \QSET default_table_access_method = regress_table_am;\E > - (\n(?!SET[^;]+;)[^\n]*)* > - \n\QCREATE TABLE > dump_test.regress_pg_dump_table_am_parent (\E > - (.*\n)* > \QSET default_table_access_method = heap;\E > + (.*\n)* > + \QALTER TABLE dump_test.regress_pg_dump_table_am_parent > SET ACCESS METHOD regress_table_am;\E > (\n(?!SET[^;]+;)[^\n]*)* > \n\QCREATE TABLE > dump_test.regress_pg_dump_table_am_child_1 (\E > (.*\n)* This looks strange -- why did you remove matching for the CREATE TABLE of the parent table? That line should appear shortly before the ALTER TABLE SET ACCESS METHOD for the same table, shouldn't it? Maybe your intention was to remove only the SET default_table_access_method = regress_table_am line ... but it's not clear to me why we have the "SET default_table_access_method = heap" line before the ALTER TABLE SET ACCESS METHOD. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "If it is not right, do not do it. If it is not true, do not say it." (Marcus Aurelius, Meditations)
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Wed, Apr 17, 2024 at 10:31:52AM +0200, Alvaro Herrera wrote:
> On 2024-Apr-17, Michael Paquier wrote:
>> Yeah, that would be easy enough to track but I was wondering about
>> adding the relkind instead. Still, one thing that I found confusing
>> is the dump generated in this case, as it would mix the SET and the
>> ALTER TABLE commands so one reading the dumps may wonder why the SET
>> has no effect for a CREATE TABLE PARTITION OF without USING. Perhaps
>> that's fine and I just worry too much ;)
>
> Hmm, maybe we should do a RESET of default_table_access_method before
> printing the CREATE TABLE to avoid the confusion.
A hard reset would make the business around currTableAM that decides
when to generate the SET default_table_access_method queries slightly
more complicated, while increasing the number of queries run on the
server.
>> The extra ALTER commands need to be generated after the object
>> definitions, so we'd need a new subroutine similar to
>> _selectTableAccessMethod() like a _selectTableAccessMethodNoStorage().
>> Or grouping both together is just simpler?
>
> I think there should be two routines, since _select* routines just print
> a SET command; maybe the new one would be _printAlterTableAM() or
> something like that. Having _select() print an ALTER TABLE command
> depending on relkind (or the boolean flag) would be confusing, I think.
Fine by me to use two routines to generate the two different commands.
I am finishing with the attached for now, making dumps, restores and
upgrades work happily as far as I've tested.
I was also worrying about a need to dump the protocol version to be
able to track the relkind in the toc entries, but a45c78e3284b has
already done one. The difference in AM handling between relations
without storage and relations with storage pushes the relkind logic
more within the internals of pg_backup_archiver.c.
What do you think?
--
Michael
From ca3d0c1828336275d86a5c51ed975c0a43ca6af9 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Thu, 18 Apr 2024 09:37:20 +0900
Subject: [PATCH v2] Set properly table AMs of partitioned tables in pg_dump
---
src/bin/pg_dump/pg_backup_archiver.c | 70 +++-
src/bin/pg_dump/pg_backup_archiver.h | 5 +-
src/bin/pg_dump/pg_dump.c| 1 +
src/bin/pg_dump/t/002_pg_dump.pl | 6 +--
4 files changed, 75 insertions(+), 7 deletions(-)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index c7a6c918a6..f69f72d731 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -30,6 +30,7 @@
#include
#endif
+#include "catalog/pg_class_d.h"
#include "common/string.h"
#include "compress_io.h"
#include "dumputils.h"
@@ -62,6 +63,8 @@ static void _becomeOwner(ArchiveHandle *AH, TocEntry *te);
static void _selectOutputSchema(ArchiveHandle *AH, const char *schemaName);
static void _selectTablespace(ArchiveHandle *AH, const char *tablespace);
static void _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam);
+static void _printTableAccessMethodNoStorage(ArchiveHandle *AH,
+ TocEntry *te);
static void processEncodingEntry(ArchiveHandle *AH, TocEntry *te);
static void processStdStringsEntry(ArchiveHandle *AH, TocEntry *te);
static void processSearchPathEntry(ArchiveHandle *AH, TocEntry *te);
@@ -1222,6 +1225,7 @@ ArchiveEntry(Archive *AHX, CatalogId catalogId, DumpId dumpId,
newToc->namespace = opts->namespace ? pg_strdup(opts->namespace) : NULL;
newToc->tablespace = opts->tablespace ? pg_strdup(opts->tablespace) : NULL;
newToc->tableam = opts->tableam ? pg_strdup(opts->tableam) : NULL;
+ newToc->relkind = opts->relkind;
newToc->owner = opts->owner ? pg_strdup(opts->owner) : NULL;
newToc->desc = pg_strdup(opts->description);
newToc->defn = opts->createStmt ? pg_strdup(opts->createStmt) : NULL;
@@ -2602,6 +2606,7 @@ WriteToc(ArchiveHandle *AH)
WriteStr(AH, te->namespace);
WriteStr(AH, te->tablespace);
WriteStr(AH, te->tableam);
+ WriteInt(AH, te->relkind);
WriteStr(AH, te->owner);
WriteStr(AH, "false");
@@ -2707,6 +2712,9 @@ ReadToc(ArchiveHandle *AH)
if (AH->version >= K_VERS_1_14)
te->tableam = ReadStr(AH);
+ if (AH->version >= K_VERS_1_16)
+ te->relkind = ReadInt(AH);
+
te->owner = ReadStr(AH);
is_supported = true;
if (AH->version < K_VERS_1_9)
@@ -3567,6 +3575,51 @@ _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam)
AH->currTableAm = pg_strdup(want);
}
+/*
+ * Set the proper default table access method for a table without storage,
+ * like a partitioned one where a table access method may be set.
+ */
+static void
+_printTableAccessMethodNoStorage(ArchiveHandle *AH, TocEntry *te)
+{
+ RestoreOptions *ropt = AH->public.ropt;
+ const char *tableam = te->tableam;
+ PQExpBuffer cmd;
+
+ /* do nothing in --no-table-access-method mode */
+ if (ropt->noTableAm)
+ return;
+
+ if (!tableam)
+ return;
+
+ Assert(te->relkind
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
BTW if nothing else, this thread led me to discover a 18-month-old typo in the Spanish translation of pg_dump: -msgstr " --no-tablespaces no volcar métodos de acceso de tablas\n" +msgstr " --no-table-access-method no volcar métodos de acceso de tablas\n" Oops. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "Los dioses no protegen a los insensatos. Éstos reciben protección de otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 2024-Apr-17, Michael Paquier wrote: > Yeah, that would be easy enough to track but I was wondering about > adding the relkind instead. Still, one thing that I found confusing > is the dump generated in this case, as it would mix the SET and the > ALTER TABLE commands so one reading the dumps may wonder why the SET > has no effect for a CREATE TABLE PARTITION OF without USING. Perhaps > that's fine and I just worry too much ;) Hmm, maybe we should do a RESET of default_table_access_method before printing the CREATE TABLE to avoid the confusion. > The extra ALTER commands need to be generated after the object > definitions, so we'd need a new subroutine similar to > _selectTableAccessMethod() like a _selectTableAccessMethodNoStorage(). > Or grouping both together is just simpler? I think there should be two routines, since _select* routines just print a SET command; maybe the new one would be _printAlterTableAM() or something like that. Having _select() print an ALTER TABLE command depending on relkind (or the boolean flag) would be confusing, I think. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Wed, Apr 17, 2024 at 09:50:02AM +0200, Alvaro Herrera wrote: > I think it's easy enough to add a "bool ispartitioned" to TableInfo and > use an ALTER TABLE or rely on the GUC depending on that -- seems easy > enough. Yeah, that would be easy enough to track but I was wondering about adding the relkind instead. Still, one thing that I found confusing is the dump generated in this case, as it would mix the SET and the ALTER TABLE commands so one reading the dumps may wonder why the SET has no effect for a CREATE TABLE PARTITION OF without USING. Perhaps that's fine and I just worry too much ;) The extra ALTER commands need to be generated after the object definitions, so we'd need a new subroutine similar to _selectTableAccessMethod() like a _selectTableAccessMethodNoStorage(). Or grouping both together is just simpler? -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 2024-Apr-17, Alvaro Herrera wrote: > Hmm, cannot we simply add a USING clause to the CREATE TABLE command for > partitioned tables? That would override the > default_table_access_method, so it should give the correct result, no? Ah, upthread you noted that pg_restore-time --no-table-access-method needs to be able to elide it, so a dump-time USING clause doesn't work. I think it's easy enough to add a "bool ispartitioned" to TableInfo and use an ALTER TABLE or rely on the GUC depending on that -- seems easy enough. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 2024-Apr-17, Michael Paquier wrote: > 2) We could limit these extra ALTER TABLE commands to be generated for > partitioned tables. This is kind of confusing as resulting dumps > would mix SET commands for default_table_access_method that would > affect tables with physical storage, while partitioned tables would > have their own extra ALTER TABLE commands. Hmm, cannot we simply add a USING clause to the CREATE TABLE command for partitioned tables? That would override the default_table_access_method, so it should give the correct result, no? -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Tue, Apr 16, 2024 at 02:19:56PM +0900, Michael Paquier wrote:
> Actually, I've come up with an idea just after hitting the send
> button: let's use an extra ALTER TABLE SET ACCESS METHOD rather than
> rely on the GUC to set the AM of the partitioned table correctly.
> This extra command should be optional, depending on
> --no-table-access-method. If a partitioned table has 0 as relam,
> let's not add this extra ALTER TABLE at all.
I have explored this idea, and while this is tempting this faces a
couple of challenges:
1) Binary upgrades would fail because the table rewrite created by
ALTER TABLE SET ACCESS METHOD for relkinds with physical storage
expects heap_create_with_catalog to have a fixed OID, but the rewrite
would require extra steps to be able to handle that, and I am not
convinced that more binary_upgrade_set_next_heap_relfilenode() is a
good idea.
2) We could limit these extra ALTER TABLE commands to be generated for
partitioned tables. This is kind of confusing as resulting dumps
would mix SET commands for default_table_access_method that would
affect tables with physical storage, while partitioned tables would
have their own extra ALTER TABLE commands. Another issue that needs
more consideration is that TocEntrys don't hold any relkind
information so pg_backup_archiver.c cannot make a difference with
tables and partitioned tables to select if SET or ALTER TABLE should
be generated.
Several designs are possible, like:
- Mix SET and ALTER TABLE commands in the dumps to set the AM, SET for
tables and matviews, ALTER TABLE for relations without storage. This
would bypass the binary upgrade problem with the fixed relid.
- Use only SET, requiring a new "default" value for
default_table_access_method that would force a partitioned table's
relam to be 0. Be stricter with the "current" table AM tracked in
pg_dump's backup archiver.
- Use only ALTER TABLE commands, with extra binary upgrade tweaks to
force relation OIDs for the second heap_create_with_catalog() done
with the rewrite to update a relation's AM.
With all that in mind, it may be better to revert 374c7a229042 and
e2395cdbe83a from HEAD and reconsider how to tackle the dump issues in
v18 or newer versions as all of the approaches I can think of lead to
more complications of their own.
Please see attached a non-polished POC that switches dumps to use
ALTER TABLE, that I've used to detect the upgrade problems.
Thoughts or comments are welcome.
--
Michael
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index c7a6c918a6..93df260986 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -61,7 +61,7 @@ static void _becomeUser(ArchiveHandle *AH, const char *user);
static void _becomeOwner(ArchiveHandle *AH, TocEntry *te);
static void _selectOutputSchema(ArchiveHandle *AH, const char *schemaName);
static void _selectTablespace(ArchiveHandle *AH, const char *tablespace);
-static void _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam);
+static void _selectTableAccessMethod(ArchiveHandle *AH, TocEntry *te);
static void processEncodingEntry(ArchiveHandle *AH, TocEntry *te);
static void processStdStringsEntry(ArchiveHandle *AH, TocEntry *te);
static void processSearchPathEntry(ArchiveHandle *AH, TocEntry *te);
@@ -2373,7 +2373,6 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
AH->currUser = NULL; /* unknown */
AH->currSchema = NULL; /* ditto */
AH->currTablespace = NULL; /* ditto */
- AH->currTableAm = NULL; /* ditto */
AH->toc = (TocEntry *) pg_malloc0(sizeof(TocEntry));
@@ -3356,9 +3355,6 @@ _reconnectToDB(ArchiveHandle *AH, const char *dbname)
free(AH->currSchema);
AH->currSchema = NULL;
- free(AH->currTableAm);
- AH->currTableAm = NULL;
-
free(AH->currTablespace);
AH->currTablespace = NULL;
@@ -3519,31 +3515,32 @@ _selectTablespace(ArchiveHandle *AH, const char *tablespace)
}
/*
- * Set the proper default_table_access_method value for the table.
+ * Set the proper default table access method for the table.
*/
static void
-_selectTableAccessMethod(ArchiveHandle *AH, const char *tableam)
+_selectTableAccessMethod(ArchiveHandle *AH, TocEntry *te)
{
RestoreOptions *ropt = AH->public.ropt;
+ const char *tableam = te->tableam;
PQExpBuffer cmd;
- const char *want,
- *have;
/* do nothing in --no-table-access-method mode */
if (ropt->noTableAm)
return;
- have = AH->currTableAm;
- want = tableam;
-
- if (!want)
- return;
-
- if (have && strcmp(want, have) == 0)
+ if (!tableam)
return;
cmd = createPQExpBuffer();
- appendPQExpBuffer(cmd, "SET default_table_access_method = %s;", fmtId(want));
+
+ appendPQExpBufferStr(cmd, "ALTER ");
+ if (strcmp(te->desc, "MATERIALIZED VIEW") == 0)
+ appendPQExpBufferStr(cmd, "MATERIALIZED VIEW ");
+ else
+ appendPQExpBufferStr(cmd, "TABLE ");
+ appendPQExpBuffer(cmd, "%s ", fmtQualifiedId(te->namespace, te->tag));
+ appendPQExpBuffer(cmd,
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Tue, Apr 16, 2024 at 02:14:21PM +0900, Michael Paquier wrote: > It seems to me that we are going to extend the GUC > default_table_access_method with a "default" mode to be able to force > relam to 0 and make a difference with the non-0 case, in the same way > as ALTER TABLE SET ACCESS METHOD DEFAULT. The thing is that, like > tablespaces, we have to rely on a GUC and not a USING clause to be > able to handle --no-table-access-method. > > An interesting point comes to what we should do for > default_table_access_method set to "default" when dealing with > something else than a partitioned table, where an error may be > adapted. Still, I'm wondering if there are more flavors I lack > imagination for. This requires more careful design work. > > Perhaps somebody has a good idea? Actually, I've come up with an idea just after hitting the send button: let's use an extra ALTER TABLE SET ACCESS METHOD rather than rely on the GUC to set the AM of the partitioned table correctly. This extra command should be optional, depending on --no-table-access-method. If a partitioned table has 0 as relam, let's not add this extra ALTER TABLE at all. -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Mon, Apr 15, 2024 at 10:46:00AM +0900, Michael Paquier wrote: > There is no need for a catalog here to trigger the failure, and it > would have happened as long as a foreign table is used. The problem > introduced in 374c7a229042 fixed by e2395cdbe83a comes from a thinko > on my side, my apologies for that and the delay in replying. Thanks > for the extra fix done in 13b3b62746ec, Alvaro. While doing more tests with this feature, among other things, I've spotted an incorrect behavior with dump/restore with the handling of the GUC default_table_access_method when it comes to partitions. Imagine the following in database "a": CREATE TABLE parent_tab (id int) PARTITION BY RANGE (id); CREATE TABLE parent_tab_2 (id int) PARTITION BY RANGE (id) USING heap; CREATE TABLE parent_tab_3 (id int) PARTITION BY RANGE (id); This leads to the following in pg_class: =# SELECT relname, relam FROM pg_class WHERE oid > 16000; relname| relam --+--- parent_tab | 0 parent_tab_2 | 2 parent_tab_3 | 0 (3 rows) Now, let's do the following: $ createdb b $ pg_dump | psql b $ psql b =# SELECT relname, relam FROM pg_class WHERE oid > 16000; relname| relam --+--- parent_tab | 0 parent_tab_2 | 0 parent_tab_3 | 0 (3 rows) And parent_tab_2 would now rely on the default GUC when creating new partitions rather than enforce heap. It seems to me that we are going to extend the GUC default_table_access_method with a "default" mode to be able to force relam to 0 and make a difference with the non-0 case, in the same way as ALTER TABLE SET ACCESS METHOD DEFAULT. The thing is that, like tablespaces, we have to rely on a GUC and not a USING clause to be able to handle --no-table-access-method. An interesting point comes to what we should do for default_table_access_method set to "default" when dealing with something else than a partitioned table, where an error may be adapted. Still, I'm wondering if there are more flavors I lack imagination for. This requires more careful design work. Perhaps somebody has a good idea? -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Tue, Apr 02, 2024 at 01:06:06AM -0400, Tom Lane wrote: > AFAICS, e2395cdbe posits that taking exclusive lock on pg_am in the > middle of a bunch of concurrent regression scripts couldn't possibly > cause any problems. Really? There is no need for a catalog here to trigger the failure, and it would have happened as long as a foreign table is used. The problem introduced in 374c7a229042 fixed by e2395cdbe83a comes from a thinko on my side, my apologies for that and the delay in replying. Thanks for the extra fix done in 13b3b62746ec, Alvaro. -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Justin Pryzby writes: > On Sun, Mar 31, 2024 at 12:00:00PM +0300, Alexander Lakhin wrote: >> I've stumbled upon a test failure caused by the test query added in that >> commit: >> +ERROR: deadlock detected >> +DETAIL: Process 3076180 waits for AccessShareLock on relation 1259 of >> database 16386; blocked by process 3076181. >> +Process 3076181 waits for AccessShareLock on relation 2601 of database >> 16386; blocked by process 3076180. > I think means that, although it was cute to use pg_am in the reproducer > given in the problem report, it's not a good choice to use here in the > sql regression tests. Another case here: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sevengill=2024-04-02%2001%3A32%3A17 AFAICS, e2395cdbe posits that taking exclusive lock on pg_am in the middle of a bunch of concurrent regression scripts couldn't possibly cause any problems. Really? regards, tom lane
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Sun, Mar 31, 2024 at 12:00:00PM +0300, Alexander Lakhin wrote: > Hello Alvaro, > > 28.03.2024 18:58, Alvaro Herrera wrote: > > Grumble. I don't like initialization at declare time, so far from the > > code that depends on the value. But the alternative would have been to > > assign right where this blocks starts, an additional line. I pushed it > > like you had it. > > I've stumbled upon a test failure caused by the test query added in that > commit: > +ERROR: deadlock detected > +DETAIL: Process 3076180 waits for AccessShareLock on relation 1259 of > database 16386; blocked by process 3076181. > +Process 3076181 waits for AccessShareLock on relation 2601 of database > 16386; blocked by process 3076180. I think means that, although it was cute to use pg_am in the reproducer given in the problem report, it's not a good choice to use here in the sql regression tests. -- Justin
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Hello Alvaro, 28.03.2024 18:58, Alvaro Herrera wrote: Grumble. I don't like initialization at declare time, so far from the code that depends on the value. But the alternative would have been to assign right where this blocks starts, an additional line. I pushed it like you had it. I've stumbled upon a test failure caused by the test query added in that commit: --- .../src/test/regress/expected/create_am.out 2024-03-28 12:14:11.700764888 -0400 +++ .../src/test/recovery/tmp_check/results/create_am.out 2024-03-31 03:10:28.172244122 -0400 @@ -549,7 +549,10 @@ ERROR: access method "btree" is not of type TABLE -- Other weird invalid cases that cause problems CREATE FOREIGN TABLE fp PARTITION OF pg_am DEFAULT SERVER x; -ERROR: "pg_am" is not partitioned +ERROR: deadlock detected +DETAIL: Process 3076180 waits for AccessShareLock on relation 1259 of database 16386; blocked by process 3076181. +Process 3076181 waits for AccessShareLock on relation 2601 of database 16386; blocked by process 3076180. +HINT: See server log for query details. -- Drop table access method, which fails as objects depends on it DROP ACCESS METHOD heap2; ERROR: cannot drop access method heap2 because other objects depend on it 027_stream_regress_primary.log contains: 2024-03-31 03:10:26.728 EDT [3076181] pg_regress/vacuum LOG: statement: VACUUM FULL pg_class; ... 2024-03-31 03:10:26.797 EDT [3076180] pg_regress/create_am LOG: statement: CREATE FOREIGN TABLE fp PARTITION OF pg_am DEFAULT SERVER x; ... 2024-03-31 03:10:28.183 EDT [3076181] pg_regress/vacuum LOG: statement: VACUUM FULL pg_database; This simple demo confirms the issue: for ((i=1;i<=20;i++)); do echo "iteration $i" echo "VACUUM FULL pg_class;" | psql >psql-1.log & echo "CREATE FOREIGN TABLE fp PARTITION OF pg_am DEFAULT SERVER x;" | psql >psql-2.log & wait done ... iteration 15 ERROR: "pg_am" is not partitioned iteration 16 ERROR: deadlock detected DETAIL: Process 2556377 waits for AccessShareLock on relation 1259 of database 16384; blocked by process 2556378. Process 2556378 waits for AccessShareLock on relation 2601 of database 16384; blocked by process 2556377. HINT: See server log for query details. ... Best regards, Alexander
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 2024-Mar-26, Justin Pryzby wrote: > Looks right. That's how I originally wrote it, except for the > "stmt->accessMethod != NULL" case. > > I prefered my way - the grammar should refuse to set stmt->accessMethod > for inappropriate relkinds. And you could assert that. Hmm, I didn't like this at first sight, because it looked convoluted and baroque, but I compared both versions for a while and I ended up liking yours more than mine, so I adopted it. > I also prefered to set "accessMethodId = InvalidOid" once, rather than > twice. Grumble. I don't like initialization at declare time, so far from the code that depends on the value. But the alternative would have been to assign right where this blocks starts, an additional line. I pushed it like you had it. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "This is what I like so much about PostgreSQL. Most of the surprises are of the "oh wow! That's cool" Not the "oh shit!" kind. :)" Scott Marlowe, http://archives.postgresql.org/pgsql-admin/2008-10/msg00152.php
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Thu, Mar 21, 2024 at 01:07:01PM +0100, Alvaro Herrera wrote:
> Given that Michaël is temporarily gone, I propose to push the attached
> tomorrow.
Thanks.
On Tue, Mar 26, 2024 at 12:05:47PM +0100, Alvaro Herrera wrote:
> On 2024-Mar-26, Alexander Lakhin wrote:
>
> > Hello Alvaro,
> >
> > 21.03.2024 15:07, Alvaro Herrera wrote:
> > > Given that Michaël is temporarily gone, I propose to push the attached
> > > tomorrow.
> >
> > Please look at a new anomaly introduced with 374c7a229.
> > Starting from that commit, the following erroneous query:
> > CREATE FOREIGN TABLE fp PARTITION OF pg_am DEFAULT SERVER x;
> >
> > triggers an assertion failure:
> > TRAP: failed Assert("relation->rd_rel->relam == InvalidOid"), File:
> > "relcache.c", Line: 1219, PID: 3706301
>
> Hmm, yeah, we're setting relam for relations that shouldn't have it.
> I propose the attached.
Looks right. That's how I originally wrote it, except for the
"stmt->accessMethod != NULL" case.
I prefered my way - the grammar should refuse to set stmt->accessMethod
for inappropriate relkinds. And you could assert that.
I also prefered to set "accessMethodId = InvalidOid" once, rather than
twice.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8a02c5b05b6..050be89728f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -962,18 +962,21 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid
ownerId,
* case of a partitioned table) the parent's, if it has one.
*/
if (stmt->accessMethod != NULL)
- accessMethodId = get_table_am_oid(stmt->accessMethod, false);
- else if (stmt->partbound)
{
- Assert(list_length(inheritOids) == 1);
- accessMethodId = get_rel_relam(linitial_oid(inheritOids));
+ Assert(RELKIND_HAS_TABLE_AM(relkind) || relkind ==
RELKIND_PARTITIONED_TABLE);
+ accessMethodId = get_table_am_oid(stmt->accessMethod, false);
}
- else
- accessMethodId = InvalidOid;
+ else if (RELKIND_HAS_TABLE_AM(relkind) || relkind ==
RELKIND_PARTITIONED_TABLE)
+ {
+ if (stmt->partbound)
+ {
+ Assert(list_length(inheritOids) == 1);
+ accessMethodId =
get_rel_relam(linitial_oid(inheritOids));
+ }
- /* still nothing? use the default */
- if (RELKIND_HAS_TABLE_AM(relkind) && !OidIsValid(accessMethodId))
- accessMethodId = get_table_am_oid(default_table_access_method,
false);
+ if (RELKIND_HAS_TABLE_AM(relkind) &&
!OidIsValid(accessMethodId))
+ accessMethodId =
get_table_am_oid(default_table_access_method, false);
+ }
/*
* Create the relation. Inherited defaults and constraints are passed
in
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 2024-Mar-26, Alexander Lakhin wrote:
> Hello Alvaro,
>
> 21.03.2024 15:07, Alvaro Herrera wrote:
> > Given that Michaël is temporarily gone, I propose to push the attached
> > tomorrow.
>
> Please look at a new anomaly introduced with 374c7a229.
> Starting from that commit, the following erroneous query:
> CREATE FOREIGN TABLE fp PARTITION OF pg_am DEFAULT SERVER x;
>
> triggers an assertion failure:
> TRAP: failed Assert("relation->rd_rel->relam == InvalidOid"), File:
> "relcache.c", Line: 1219, PID: 3706301
Hmm, yeah, we're setting relam for relations that shouldn't have it.
I propose the attached.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
>From 8b68591cea43fdc50fe53f11b077fcd42d501946 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 26 Mar 2024 11:58:53 +0100
Subject: [PATCH] relam fixes
---
src/backend/commands/tablecmds.c | 22 ++
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 635d54645d..315d355238 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -714,7 +714,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
Oid ofTypeId;
ObjectAddress address;
LOCKMODE parentLockmode;
- Oid accessMethodId = InvalidOid;
+ Oid accessMethodId;
/*
* Truncate relname to appropriate length (probably a waste of time, as
@@ -958,15 +958,21 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
}
/*
- * Select access method to use: an explicitly indicated one, or (in the
- * case of a partitioned table) the parent's, if it has one.
+ * For relations with table AM and partitioned tables, select access method
+ * to use: an explicitly indicated one, or (in the case of a partitioned
+ * table) the parent's, if it has one.
*/
- if (stmt->accessMethod != NULL)
- accessMethodId = get_table_am_oid(stmt->accessMethod, false);
- else if (stmt->partbound)
+ if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE)
{
- Assert(list_length(inheritOids) == 1);
- accessMethodId = get_rel_relam(linitial_oid(inheritOids));
+ if (stmt->accessMethod != NULL)
+ accessMethodId = get_table_am_oid(stmt->accessMethod, false);
+ else if (stmt->partbound)
+ {
+ Assert(list_length(inheritOids) == 1);
+ accessMethodId = get_rel_relam(linitial_oid(inheritOids));
+ }
+ else
+ accessMethodId = InvalidOid;
}
else
accessMethodId = InvalidOid;
--
2.39.2
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Hello Alvaro,
21.03.2024 15:07, Alvaro Herrera wrote:
Given that Michaël is temporarily gone, I propose to push the attached
tomorrow.
Please look at a new anomaly introduced with 374c7a229.
Starting from that commit, the following erroneous query:
CREATE FOREIGN TABLE fp PARTITION OF pg_am DEFAULT SERVER x;
triggers an assertion failure:
TRAP: failed Assert("relation->rd_rel->relam == InvalidOid"), File:
"relcache.c", Line: 1219, PID: 3706301
with the stack trace:
...
#4 0x7fe53ced67f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x55f28555951e in ExceptionalCondition (conditionName=conditionName@entry=0x55f285744788
"relation->rd_rel->relam == InvalidOid", fileName=fileName@entry=0x55f285743f1c "relcache.c",
lineNumber=lineNumber@entry=1219)
at assert.c:66
#6 0x55f285550450 in RelationBuildDesc (targetRelId=targetRelId@entry=16385, insertIt=insertIt@entry=false) at
relcache.c:1219
#7 0x55f285550769 in RelationClearRelation (relation=relation@entry=0x7fe5310dd178, rebuild=rebuild@entry=true) at
relcache.c:2667
#8 0x55f285550c41 in RelationFlushRelation (relation=0x7fe5310dd178) at
relcache.c:2850
#9 0x55f285550ca0 in RelationCacheInvalidateEntry (relationId=) at relcache.c:2921
#10 0x55f285542551 in LocalExecuteInvalidationMessage (msg=0x55f2861b3160)
at inval.c:738
#11 0x55f28554159b in ProcessInvalidationMessages (group=0x55f2861b2e6c, func=func@entry=0x55f2855424a8
) at inval.c:518
#12 0x55f285542740 in CommandEndInvalidationMessages () at inval.c:1180
#13 0x55f28509cbbd in AtCCI_LocalCache () at xact.c:1550
#14 0x55f28509e88e in CommandCounterIncrement () at xact.c:1116
#15 0x55f2851d0c8b in DefineRelation (stmt=stmt@entry=0x55f2861803b0, relkind=relkind@entry=102 'f', ownerId=10,
ownerId@entry=0, typaddress=typaddress@entry=0x0,
queryString=queryString@entry=0x55f28617f870 "CREATE FOREIGN TABLE fp PARTITION OF pg_am DEFAULT SERVER x;") at
tablecmds.c:1008
#16 0x55f28540945d in ProcessUtilitySlow (pstate=pstate@entry=0x55f2861a9dc0, pstmt=pstmt@entry=0x55f286180510,
queryString=queryString@entry=0x55f28617f870 "CREATE FOREIGN TABLE fp PARTITION OF pg_am DEFAULT SERVER x;",
context=context@entry=PROCESS_UTILITY_TOPLEVEL, params=params@entry=0x0, queryEnv=queryEnv@entry=0x0,
dest=0x55f2861807d0, qc=0x7fff15b5d7c0) at utility.c:1203
#17 0x55f28540911f in standard_ProcessUtility (pstmt=0x55f286180510, queryString=0x55f28617f870 "CREATE FOREIGN
TABLE fp PARTITION OF pg_am DEFAULT SERVER x;", readOnlyTree=, context=PROCESS_UTILITY_TOPLEVEL,
params=0x0, queryEnv=0x0, dest=0x55f2861807d0, qc=0x7fff15b5d7c0) at
utility.c:1067
...
On 374c7a229~1 it fails with
ERROR: "pg_am" is not partitioned
Best regards,
Alexander
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Mar 21, 2024, at 13:07, Alvaro Herrera wrote: > Given that Michaël is temporarily gone, I propose to push the attached > tomorrow. Thanks for doing so. I’m wondering whether I should be an author of this patch at this stage, tbh. I wrote all the tests and rewrote most of the internals to adjust with the consensus reached ;) -- Michael
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Given that Michaël is temporarily gone, I propose to push the attached tomorrow. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ really, I see PHP as like a strange amalgamation of C, Perl, Shell inflex: you know that "amalgam" means "mixture with mercury", more or less, right? i.e., "deadly poison" >From 276deacbe133af10aff2aeba482931d180ce9fe3 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 8 Mar 2024 13:24:14 +0900 Subject: [PATCH v5] Allow specifying access method for partitioned tables Partitioned tables gain support for CREATE TABLE .. USING, where specifying an access method can serve as a hint when creating partitions on it. This includes support for ALTER TABLE .. SET ACCESS METHOD, where specifying DEFAULT is equivalent to resetting the partitioned table's relam to 0. Specifying a non-DEFAULT access method will set its relam to not be 0. The historical default of using pg_class.relam as InvalidOid is preserved, corresponding to the case where USING clause is not used. In this case, like previous releases, a partition is created with the access method set by default_table_access_method if its CREATE TABLE query does not specify an access method with a USING clause. The relcache of partitioned tables is not changed: rd_tableam is not set, even if a partitioned table has a relam set. Authors: Justin Pryzby, Soumyadeep Chakraborty Reviewed-by: Michael Paquier, and a few others.. Discussion: https://postgr.es/m/ --- doc/src/sgml/catalogs.sgml | 8 +- doc/src/sgml/ref/alter_table.sgml | 8 ++ doc/src/sgml/ref/create_table.sgml | 4 + src/backend/commands/tablecmds.c| 165 +++- src/backend/utils/cache/lsyscache.c | 22 src/backend/utils/cache/relcache.c | 7 + src/bin/pg_dump/pg_dump.c | 3 +- src/bin/pg_dump/t/002_pg_dump.pl| 35 + src/include/catalog/pg_class.h | 4 +- src/include/utils/lsyscache.h | 1 + src/test/regress/expected/create_am.out | 158 ++- src/test/regress/sql/create_am.sql | 91 - 12 files changed, 462 insertions(+), 44 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index b7980eb499..03815e10e8 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -1989,8 +1989,12 @@ SCRAM-SHA-256$iteration count: If this is a table or an index, the access method used (heap, - B-tree, hash, etc.); otherwise zero (zero occurs for sequences, - as well as relations without storage, such as views) + B-tree, hash, etc.); for partitioned tables, the access method + for newly created partitions, when one is not specified in the + creation command, or zero to make creation fall back to + default_table_access_method. + Zero for sequences, as well as other relations without + storage, such as views. diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 96e3d77605..b6e794e6ce 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -737,6 +737,14 @@ WITH ( MODULUS numeric_literal, REM DEFAULT changes the access method of the table to . + + When applied to a partitioned table, there is no data to rewrite, but any + partitions created afterwards will use that access method unless + overridden by a USING clause. Using + DEFAULT switches the partitioned table to use + default_table_access_method when new partitions are + created. + diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml index 4cbaaccaf7..b79081a5ec 100644 --- a/doc/src/sgml/ref/create_table.sgml +++ b/doc/src/sgml/ref/create_table.sgml @@ -1330,6 +1330,10 @@ WITH ( MODULUS numeric_literal, REM method is chosen for the new table. See for more information. + + When creating a partition, the table access method is the access method + of its partitioned table, if set. + diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 6c0c899210..9e36aae425 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -183,7 +183,9 @@ typedef struct AlteredTableInfo List *afterStmts; /* List of utility command parsetrees */ bool verify_new_notnull; /* T if we should recheck NOT NULL */ int rewrite; /* Reason for forced rewrite, if any */ - Oid newAccessMethod; /* new access method; 0 means no change */ + bool chgAccessMethod; /* T if SET ACCESS METHOD is used */ + Oid newAccessMethod; /* new access method; 0 means no change, + * if above is true */ Oid newTableSpace; /* new tablespace; 0 means no change */ bool chgPersistence; /* T if SET LOGGED/UNLOGGED is used */ char
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 2024-Mar-19, Alvaro Herrera wrote: > 0001 is Michaël's patch, 0002 are my proposed changes. Doh, I sent the wrong set of attachments. But I see no reason to post again: what I attached as 0001 is what I wrote was going to be 0002, Michaël's patch is already in archives, and the CI tests with both applied on current master are running here: https://cirrus-ci.com/build/6404370015715328 Michaël, I'll leave this for you to push ... Thanks! -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 2024-Mar-08, Michael Paquier wrote:
> I have spent more time reviewing the whole and the tests (I didn't see
> much value in testing the DEFAULT clause twice for the partitioned
> table case and there is a test in d61a6cad6418), tweaked a few
> comments and the documentation, did an indentation and a commit
> message draft.
>
> How does that look to you? The test coverage and the semantics do
> what we want them to do, so that looks rather reasonable here. A
> second or even third pair of eyes would not hurt.
I gave this a look. I found some of the comments a bit confusing or
overly long, so I propose to reword them. I also propose a small doc
change (during writing which I noticed that the docs for tablespace had
been neglected and one comment too many; patch to be committed
separately soon). I ended up also moving code in tablecmds.c so that
all the AT*SetAccessMethod* routines appear together rather than mixed
with the ones for tablespaces, and removing one CCI that seems
unnecessary, at the bottom of ATExecSetAccessMethodNoStorage.
0001 is Michaël's patch, 0002 are my proposed changes.
--
Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
"Hay que recordar que la existencia en el cosmos, y particularmente la
elaboración de civilizaciones dentro de él no son, por desgracia,
nada idílicas" (Ijon Tichy)
>From 8b4408ac6cc46af2257e9c613d95a0eb38238a8e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 19 Mar 2024 09:51:58 +0100
Subject: [PATCH] comment updates, remove one CCI
---
doc/src/sgml/catalogs.sgml | 17 ++-
src/backend/commands/tablecmds.c | 223 ++-
2 files changed, 114 insertions(+), 126 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index c86ff19754..476a7b70dd 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1989,9 +1989,12 @@ SCRAM-SHA-256$iteration
count:
If this is a table or an index, the access method used (heap,
- B-tree, hash, etc.); otherwise zero. Zero occurs for sequences,
- as well as relations without storage, such as views. Partitioned
- tables may use a non-zero value.
+ B-tree, hash, etc.); for partitioned tables, the access method
+ for newly created partitions, when one is not specified in the
+ creation command, or zero to make creation fall back to
+ default_table_access_method.
+ Zero for sequences, as well as other relations without
+ storage, such as views.
@@ -2012,9 +2015,11 @@ SCRAM-SHA-256$iteration
count:
(references pg_tablespace.oid)
- The tablespace in which this relation is stored. If zero,
- the database's default tablespace is implied. (Not meaningful
- if the relation has no on-disk file.)
+ The tablespace in which this relation is stored; for partitioned
+ tables, the tablespace in which partitions are created
+ when one is not specified in the creation command.
+ If zero, the database's default tablespace is implied.
+ (Not meaningful if the relation has no on-disk file.)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 54c58d1764..9e36aae425 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -950,27 +950,20 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid
ownerId,
}
/*
-* If the statement hasn't specified an access method, but we're
defining
-* a type of relation that needs one, use the default.
+* Select access method to use: an explicitly indicated one, or (in the
+* case of a partitioned table) the parent's, if it has one.
*/
if (stmt->accessMethod != NULL)
accessMethodId = get_table_am_oid(stmt->accessMethod, false);
else if (stmt->partbound)
{
- /*
-* For partitions, if no access method is specified, use the AM
of the
-* parent table.
-*/
Assert(list_length(inheritOids) == 1);
accessMethodId = get_rel_relam(linitial_oid(inheritOids));
}
else
accessMethodId = InvalidOid;
- /*
-* Still nothing? Use the default. Partitioned tables default to
-* InvalidOid without an access method specified.
-*/
+ /* still nothing? use the default */
if (RELKIND_HAS_TABLE_AM(relkind) && !OidIsValid(accessMethodId))
accessMethodId = get_table_am_oid(default_table_access_method,
false);
@@ -5403,7 +5396,6 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
/* nothing to do here, oid columns don't exist anymore
*/
break;
case AT_SetAccessMethod:/* SET ACCESS METHOD */
- /* handled
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Thu, Mar 07, 2024 at 08:02:00PM -0600, Justin Pryzby wrote: > As you wrote it, you pass the "defaultAccessMethod" bool to > ATExecSetAccessMethodNoStorage(), which seems odd. Why not just pass > the target amoid as newAccessMethod ? + /* +* Check that the table access method exists. +* Use the access method, if specified, otherwise (when not specified) 0 +* for partitioned tables or the configured default AM. +*/ + if (amname != NULL) + amoid = get_table_am_oid(amname, false); + else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + amoid = 0; + else + amoid = get_table_am_oid(default_table_access_method, false); .. While using this flow to choose the AM oid, that's neater than the versions I could come up with, pretty cool. > When I fooled with this on my side, I called it "chgAccessMethod" to > follow "chgPersistence". I think "is default" isn't the right data > structure. > > Attached a relative patch with my version. Thanks. I've applied the patch to add the DEFAULT clause for now, to ease the work on this patch. > Also: I just realized that instead of adding a bool, we could test > (tab->rewrite & AT_REWRITE_ACCESS_METHOD) != 0 Hmm. I've considered that, but the boolean style is able to do the work, while being more consistent, so I'm OK with what you are doing in your 0002. > +-- Default and AM set in in clause are the same, relam should be set. > > in in? Oops, fixed. I have spent more time reviewing the whole and the tests (I didn't see much value in testing the DEFAULT clause twice for the partitioned table case and there is a test in d61a6cad6418), tweaked a few comments and the documentation, did an indentation and a commit message draft. How does that look to you? The test coverage and the semantics do what we want them to do, so that looks rather reasonable here. A second or even third pair of eyes would not hurt. -- Michael From 4114c7944c78dbb6eb85320434d942daa77fa4d8 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 8 Mar 2024 13:24:14 +0900 Subject: [PATCH v4] Allow specifying access method for partitioned tables Partitioned tables gain support for CREATE TABLE .. USING, where specifying an access method can serve as a hint when creating partitions on it. This includes support for ALTER TABLE .. SET ACCESS METHOD, where specifying DEFAULT is equivalent to resetting the partitioned table's relam to 0. Specifying a non-DEFAULT access method will set its relam to not be 0. The historical default of using pg_class.relam as InvalidOid is preserved, corresponding to the case where USING clause is not used. In this case, like previous releases, a partition is created with the access method set by default_table_access_method if its CREATE TABLE query does not specify an access method with a USING clause. The relcache of partitioned tables is not changed: rd_tableam is not set, even if a partitioned table has a relam set. Authors: Justin Pryzby, Soumyadeep Chakraborty Reviewed-by: Michael Paquier, and a few others.. Discussion: https://postgr.es/m/ --- src/include/catalog/pg_class.h | 4 +- src/include/utils/lsyscache.h | 1 + src/backend/commands/tablecmds.c| 172 src/backend/utils/cache/lsyscache.c | 22 +++ src/backend/utils/cache/relcache.c | 7 + src/bin/pg_dump/pg_dump.c | 3 +- src/bin/pg_dump/t/002_pg_dump.pl| 35 + src/test/regress/expected/create_am.out | 158 +- src/test/regress/sql/create_am.sql | 91 - doc/src/sgml/catalogs.sgml | 5 +- doc/src/sgml/ref/alter_table.sgml | 8 ++ doc/src/sgml/ref/create_table.sgml | 4 + 12 files changed, 471 insertions(+), 39 deletions(-) diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h index 3b7533e7bb..0fc2c093b0 100644 --- a/src/include/catalog/pg_class.h +++ b/src/include/catalog/pg_class.h @@ -219,7 +219,9 @@ MAKE_SYSCACHE(RELNAMENSP, pg_class_relname_nsp_index, 128); /* * Relation kinds with a table access method (rd_tableam). Although sequences * use the heap table AM, they are enough of a special case in most uses that - * they are not included here. + * they are not included here. Likewise, partitioned tables can have an access + * method defined so that their partitions can inherit it, but they do not set + * rd_tableam; hence, this is handled specially outside of this macro. */ #define RELKIND_HAS_TABLE_AM(relkind) \ ((relkind) == RELKIND_RELATION || \ diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h index e4a200b00e..35a8dec2b9 100644 --- a/src/include/utils/lsyscache.h +++ b/src/include/utils/lsyscache.h @@ -139,6 +139,7 @@ extern char get_rel_relkind(Oid relid); extern bool get_rel_relispartition(Oid relid); extern Oid get_rel_tablespace(Oid relid); extern char
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Mon, Mar 04, 2024 at 05:46:56PM +0900, Michael Paquier wrote: > > Since InvalidOid is already taken, I guess you might need to introduce a > > boolean flag, like set_relam, indicating that the statement has an > > ACCESS METHOD clause. > > Yes, I don't see an alternative. The default needs a different field > to be tracked down to the execution. The data structure you used (defaultAccessMethod) allows this, which is intended to be prohibited: postgres=# ALTER TABLE a SET access method default, SET access method default; ALTER TABLE As you wrote it, you pass the "defaultAccessMethod" bool to ATExecSetAccessMethodNoStorage(), which seems odd. Why not just pass the target amoid as newAccessMethod ? When I fooled with this on my side, I called it "chgAccessMethod" to follow "chgPersistence". I think "is default" isn't the right data structure. Attached a relative patch with my version. Also: I just realized that instead of adding a bool, we could test (tab->rewrite & AT_REWRITE_ACCESS_METHOD) != 0 +-- Default and AM set in in clause are the same, relam should be set. in in? -- Justin >From 8aca5e443ebb41b7de1ba18b519a33a97a41a897 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 4 Mar 2024 17:42:04 +0900 Subject: [PATCH 1/2] Allow specifying access method of partitioned tables ..to be inherited by partitions See also: ca4103025dfe26eaaf6a500dec9170fbb176eebc 8586bf7ed8889f39a59dd99b292014b73be85342 ebfe2dbd6b624e2a428e14b7ee9322cc096f63f7 - prevent DROP AM Authors: Justin Pryzby, Soumyadeep Chakraborty --- doc/src/sgml/catalogs.sgml | 5 +- doc/src/sgml/ref/alter_table.sgml | 8 ++ doc/src/sgml/ref/create_table.sgml | 4 + src/backend/commands/tablecmds.c| 183 +--- src/backend/utils/cache/lsyscache.c | 22 +++ src/backend/utils/cache/relcache.c | 7 + src/bin/pg_dump/pg_dump.c | 3 +- src/bin/pg_dump/t/002_pg_dump.pl| 35 + src/include/catalog/pg_class.h | 4 +- src/include/utils/lsyscache.h | 1 + src/test/regress/expected/create_am.out | 158 +++- src/test/regress/sql/create_am.sql | 91 +++- 12 files changed, 487 insertions(+), 34 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 0ae97d1adaa..e4fcacb610a 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -1989,8 +1989,9 @@ SCRAM-SHA-256$iteration count: If this is a table or an index, the access method used (heap, - B-tree, hash, etc.); otherwise zero (zero occurs for sequences, - as well as relations without storage, such as views) + B-tree, hash, etc.); otherwise zero. Zero occurs for sequences, + as well as relations without storage, such as views. Partitioned + tables may use a non-zero value. diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 96e3d776051..779c8b31226 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -737,6 +737,14 @@ WITH ( MODULUS numeric_literal, REM DEFAULT changes the access method of the table to . + + When applied to a partitioned table, there is no data to rewrite, but any + partitions created afterwards will use that access method unless + overridden by a USING clause. Using + DEFAULT switches the partitioned table to rely on + the value of default_table_access_method set + when new partitions are created, instead. + diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml index 4cbaaccaf7c..b79081a5ec1 100644 --- a/doc/src/sgml/ref/create_table.sgml +++ b/doc/src/sgml/ref/create_table.sgml @@ -1330,6 +1330,10 @@ WITH ( MODULUS numeric_literal, REM method is chosen for the new table. See for more information. + + When creating a partition, the table access method is the access method + of its partitioned table, if set. + diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 7014be80396..cf595329b6c 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -184,6 +184,7 @@ typedef struct AlteredTableInfo bool verify_new_notnull; /* T if we should recheck NOT NULL */ int rewrite; /* Reason for forced rewrite, if any */ Oid newAccessMethod; /* new access method; 0 means no change */ + bool defaultAccessMethod; /* true if SET ACCESS METHOD DEFAULT */ Oid newTableSpace; /* new tablespace; 0 means no change */ bool chgPersistence; /* T if SET LOGGED/UNLOGGED is used */ char newrelpersistence; /* if above is true */ @@ -589,6 +590,8 @@ static ObjectAddress ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode); static void ATExecDropCluster(Relation rel, LOCKMODE lockmode); static void
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Fri, Mar 01, 2024 at 03:03:14PM -0600, Justin Pryzby wrote:
> I think if the user sets something "explicitly", the catalog should
> reflect what they set. Tablespaces have dattablespace, but AMs don't --
> it's a simpler case.
Okay.
> For 001: we don't *need* to support "ALTER SET AM default" for leaf
> tables. It doesn't do anything that's not already possible. But, if
> AMs for partitioned tables are optional rather than required, then seems
> to be needed to allow (re)settinng relam=0.
Indeed, for non-partitioned tables DEFAULT is a sugar flavor. Not
mandatory, still it's nice to have to not have to type an AM.
> But for partitioned tables, I think it should set relam=0 directly.
> Currently it 1) falls through to default_table_am; and 2) detects that
> it's the default, so then sets relam to 0.
>
> Since InvalidOid is already taken, I guess you might need to introduce a
> boolean flag, like set_relam, indicating that the statement has an
> ACCESS METHOD clause.
Yes, I don't see an alternative. The default needs a different field
to be tracked down to the execution.
>> + * method defined so as their children can inherit it; however, this is
>> handled
>
> so that
>
>> + * Do nothing: access methods is a setting that partitions can
>
> method (singular), or s/is/are/
Indeed. Fixed both.
> In any case, it'd be a bit confusing for the error message to still say:
>
> postgres=# CREATE TABLE a(i int) PARTITION BY RANGE(a) USING heap2;
> ERROR: specifying a table access method is not supported on a partitioned
> table
I was looking at this one as well and I don't see why we could not
remove it, so you are right (missed the tablespace part last week). A
partitioned table created as a partition of a partitioned table would
inherit the relam of its parent (0 if default is set, or non-0 is
something is set). I have added some regression tests for that.
And I'm finishing with the attached. To summarize SET ACCESS METHOD
on a partitioned table, the semantics are:
- DEFAULT sets the relam to 0, any partitions with storage would use
the GUC at creation time. Partitioned tables use a relam of 0.
- If a value is set for the am, relam becomes non-0. Any partitions
created on it inherit it (partitioned as well as non-partitioned
tables).
- No USING clause means to set its relam to 0.
0001 seems OK here, 0002 needs more eyes. The bulk of the changes is
in the regression tests to cover all the cases I could think of.
--
Michael
From 4d8a5de79b829e4f1be875c668f85bbfa6d3f37a Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 1 Mar 2024 10:22:22 +0900
Subject: [PATCH v3 1/2] Add DEFAULT option to ALTER TABLE SET ACCESS METHOD
---
src/backend/commands/tablecmds.c| 3 ++-
src/backend/parser/gram.y | 10 --
src/test/regress/expected/create_am.out | 21 +
src/test/regress/sql/create_am.sql | 11 +++
doc/src/sgml/ref/alter_table.sgml | 6 --
5 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f798794556..3b1c2590fd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15212,7 +15212,8 @@ ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname)
Oid amoid;
/* Check that the table access method exists */
- amoid = get_table_am_oid(amname, false);
+ amoid = get_table_am_oid(amname ? amname : default_table_access_method,
+ false);
if (rel->rd_rel->relam == amoid)
return;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 130f7fc7c3..c6e2f679fd 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -338,6 +338,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type alter_identity_column_option_list
%type alter_identity_column_option
%type set_statistics_value
+%type set_access_method_name
%type createdb_opt_list createdb_opt_items copy_opt_list
transaction_mode_list
@@ -2859,8 +2860,8 @@ alter_table_cmd:
n->newowner = $3;
$$ = (Node *) n;
}
- /* ALTER TABLE SET ACCESS METHOD */
- | SET ACCESS METHOD name
+ /* ALTER TABLE SET ACCESS METHOD { | DEFAULT } */
+ | SET ACCESS METHOD set_access_method_name
{
AlterTableCmd *n = makeNode(AlterTableCmd);
@@ -3076,6 +3077,11 @@ set_statistics_value:
| DEFAULT { $$ = NULL; }
;
+set_access_method_name:
+ ColId { $$ = $1; }
+ | DEFAULT { $$ = NULL; }
+ ;
+
PartitionBoundSpec:
/* a HASH partition */
FOR VALUES WITH '(' hash_partbound ')'
diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out
index b50293d514..e843d39ee7 100644
--- a/src/test/regress/expected/create_am.out
+++ b/src/test/regress/expected/create_am.out
@@ -283,6 +283,27 @@ SELECT COUNT(a), COUNT(1) FILTER(WHERE a=1) FROM heaptable;
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Fri, Mar 01, 2024 at 10:56:50AM +0900, Michael Paquier wrote: > On Thu, Feb 29, 2024 at 08:51:31AM -0600, Justin Pryzby wrote: > > On Wed, Feb 28, 2024 at 05:08:49PM +0900, Michael Paquier wrote: > >> I have implemented that so as we keep the default, historical > >> behavior: if pg_class.relam is 0 for a partitioned table, use the AM > >> defined by default_table_access_method. The patch only adds a path to > >> switch to a different AM than the GUC when creating a new partition if > >> and only if a partitioned table has been manipulated with ALTER TABLE > >> SET ACCESS METHOD to update its AM to something else than the GUC. > >> Similarly to tablespaces, CREATE TABLE USING is *not* supported for > >> partitioned tables, same behavior as previously. > > > > This patch allows resetting relam=0 by running ALTER TABLE SET AM to the > > same value as the GUC. Maybe it'd be better to have an explicit SET > > DEFAULT (as in b9424d01 and 4f622503). > > Outside the scope of this patch's thread, this looks like a good idea > even for tables/matviews. And the semantics are pretty easy: if DEFAULT > is specified, just set the access method to NULL in the parser and let > tablecmds.c go the AM OID lookup in the prep phase if set to NULL. > See 0001 attached. This one looks pretty good taken as an independent > piece. > > When it comes to partitioned tables, there is a still a tricky case: > what should we do when a user specifies a non-default value in the SET > ACCESS METHOD clause and it matches default_table_access_method? I don't think it's tricky - it seems more like a weird hack in the previous patch version to make AMs behave like tablespaces, despite not being completely parallel, due to the absence of a pg_default AM. With the new 001, the hack can go away, and so it should. > Should the relam be 0 or should we force relam to be the OID of the > given value given by the query? You said "force" it to be the user-specified value, but I think that's not "forcing", it's respecting (but to take the user's desired value, and conditionally store 0 instead, that could be described as "forcing.") > Implementation-wise, forcing the value to 0 is simpler, but I can get > why it could be confusing as well, because the state of the catalogs > does not reflect what was provided in the query. > At the same time, the user has explicitly set the access method to be > the same as the default, so perhaps 0 makes sense anyway in this case. I think if the user sets something "explicitly", the catalog should reflect what they set. Tablespaces have dattablespace, but AMs don't -- it's a simpler case. For 001: we don't *need* to support "ALTER SET AM default" for leaf tables. It doesn't do anything that's not already possible. But, if AMs for partitioned tables are optional rather than required, then seems to be needed to allow (re)settinng relam=0. But for partitioned tables, I think it should set relam=0 directly. Currently it 1) falls through to default_table_am; and 2) detects that it's the default, so then sets relam to 0. Since InvalidOid On Fri, Mar 01, 2024 at 02:03:48PM +0900, Michael Paquier wrote: > Fun topic, especially once coupled with the internals of tablecmds.c > that uses InvalidOid for the new access AM as a special value to work > as a no-op. Since InvalidOid is already taken, I guess you might need to introduce a boolean flag, like set_relam, indicating that the statement has an ACCESS METHOD clause. > + * method defined so as their children can inherit it; however, this is > handled so that > + * Do nothing: access methods is a setting that partitions can method (singular), or s/is/are/ On Wed, Feb 28, 2024 at 05:08:49PM +0900, Michael Paquier wrote: > Similarly to tablespaces, CREATE TABLE USING is *not* supported for > partitioned tables, same behavior as previously. Maybe I misunderstood what you're trying to say, but CREATE..TABLESPACE *is* supported for partitioned tables. I'm not sure why it wouldn't be supported to set the AM, too. In any case, it'd be a bit confusing for the error message to still say: postgres=# CREATE TABLE a(i int) PARTITION BY RANGE(a) USING heap2; ERROR: specifying a table access method is not supported on a partitioned table -- Justin
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Fri, 1 Mar 2024 at 06:15, Michael Paquier wrote: > > On Fri, Mar 01, 2024 at 05:43:25AM +0100, Jelte Fennema-Nio wrote: > > I think we should set the AM OID explicitly. Because an important > > thing to consider is: What behaviour makes sense when later > > default_table_access_method is changed? > > I think if someone sets it explicitly on the partitioned table, they > > would want the AM of the partitioned table to stay the same when > > default_table_access_method is changed. Which requires storing the AM > > OID afaict. > > Oops, I think I misread that. You just mean to always set relam when > using an AM in the SET ACCESS METHOD clause. Apologies for the noise. Correct, I intended to say that "SET ACCESS METHOD heap" on a partitioned table should store heap its OID. Because while storing 0 might be simpler, it will result in (imho) wrong behaviour when later the default_table_access_method is changed. behavior won't result in the (imho) intended. i.e. it's not simply a small detail in what the catolog looks like, but there's an actual behavioural change.
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Fri, Mar 01, 2024 at 05:43:25AM +0100, Jelte Fennema-Nio wrote: > I think we should set the AM OID explicitly. Because an important > thing to consider is: What behaviour makes sense when later > default_table_access_method is changed? > I think if someone sets it explicitly on the partitioned table, they > would want the AM of the partitioned table to stay the same when > default_table_access_method is changed. Which requires storing the AM > OID afaict. Oops, I think I misread that. You just mean to always set relam when using an AM in the SET ACCESS METHOD clause. Apologies for the noise. -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Fri, Mar 01, 2024 at 05:43:25AM +0100, Jelte Fennema-Nio wrote: > I think we should set the AM OID explicitly. Because an important > thing to consider is: What behaviour makes sense when later > default_table_access_method is changed? Per the latest discussion of the thread, we've kind of reached a consensus that we should keep the current historical bevahior on default, where relam remains at 0, causing new partitions to grab the GUC as AM. If we create a partitioned table attached to a partitioned table, it should be 0 as well. If the partitioned table has a non-0 relam, a new partitioned table created on it will inherit the same non-0 value. > I think if someone sets it explicitly on the partitioned table, they > would want the AM of the partitioned table to stay the same when > default_table_access_method is changed. Which requires storing the AM > OID afaict. If we allow relam to be non-0 for a partitioned table, it is equally important to give users a way to reset it at will. My point was a bit more subtle than that. For example, this sequence is clear to me: SET default_table_access_method = 'foo'; ALTER TABLE part SET ACCESS METHOD DEFAULT; The user wants to rely on the GUC, so relam should be 0, new partitions created on it will use the GUC. Now, what should this sequence mean? See: SET default_table_access_method = 'foo'; ALTER TABLE part SET ACCESS METHOD foo; Should the relam be 0 because the user requested a match with the GUC, or use the OID of the AM? There has to be some difference with tablespaces, because relations with physical storage (tables, matviews) can use a reltablespace of 0, but AMs have to be set for tables and matviews. Fun topic, especially once coupled with the internals of tablecmds.c that uses InvalidOid for the new access AM as a special value to work as a no-op. -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Fri, 1 Mar 2024 at 02:57, Michael Paquier wrote: > When it comes to partitioned tables, there is a still a tricky case: > what should we do when a user specifies a non-default value in the SET > ACCESS METHOD clause and it matches default_table_access_method? > Should the relam be 0 or should we force relam to be the OID of the > given value given by the query? Implementation-wise, forcing the > value to 0 is simpler, but I can get why it could be confusing as > well, because the state of the catalogs does not reflect what was > provided in the query. At the same time, the user has explicitly set > the access method to be the same as the default, so perhaps 0 makes > sense anyway in this case. I think we should set the AM OID explicitly. Because an important thing to consider is: What behaviour makes sense when later default_table_access_method is changed? I think if someone sets it explicitly on the partitioned table, they would want the AM of the partitioned table to stay the same when default_table_access_method is changed. Which requires storing the AM OID afaict.
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Thu, Feb 29, 2024 at 08:51:31AM -0600, Justin Pryzby wrote:
> On Wed, Feb 28, 2024 at 05:08:49PM +0900, Michael Paquier wrote:
>> I have implemented that so as we keep the default, historical
>> behavior: if pg_class.relam is 0 for a partitioned table, use the AM
>> defined by default_table_access_method. The patch only adds a path to
>> switch to a different AM than the GUC when creating a new partition if
>> and only if a partitioned table has been manipulated with ALTER TABLE
>> SET ACCESS METHOD to update its AM to something else than the GUC.
>> Similarly to tablespaces, CREATE TABLE USING is *not* supported for
>> partitioned tables, same behavior as previously.
>
> This patch allows resetting relam=0 by running ALTER TABLE SET AM to the
> same value as the GUC. Maybe it'd be better to have an explicit SET
> DEFAULT (as in b9424d01 and 4f622503).
Outside the scope of this patch's thread, this looks like a good idea
even for tables/matviews. And the semantics are pretty easy: if DEFAULT
is specified, just set the access method to NULL in the parser and let
tablecmds.c go the AM OID lookup in the prep phase if set to NULL.
See 0001 attached. This one looks pretty good taken as an independent
piece.
When it comes to partitioned tables, there is a still a tricky case:
what should we do when a user specifies a non-default value in the SET
ACCESS METHOD clause and it matches default_table_access_method?
Should the relam be 0 or should we force relam to be the OID of the
given value given by the query? Implementation-wise, forcing the
value to 0 is simpler, but I can get why it could be confusing as
well, because the state of the catalogs does not reflect what was
provided in the query. At the same time, the user has explicitly set
the access method to be the same as the default, so perhaps 0 makes
sense anyway in this case.
0002 does that, as that's simpler. I'm not sure if there is a case
for forcing a value in relam if the query has the same value as the
default. Thoughts?
--
Michael
From d8f1094415c34eaaae46c8d05b8abc83693b Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 1 Mar 2024 10:22:22 +0900
Subject: [PATCH v2 1/2] Add DEFAULT option to ALTER TABLE SET ACCESS METHOD
---
src/backend/commands/tablecmds.c| 3 ++-
src/backend/parser/gram.y | 10 --
src/test/regress/expected/create_am.out | 21 +
src/test/regress/sql/create_am.sql | 11 +++
doc/src/sgml/ref/alter_table.sgml | 6 --
5 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f798794556..3b1c2590fd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15212,7 +15212,8 @@ ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname)
Oid amoid;
/* Check that the table access method exists */
- amoid = get_table_am_oid(amname, false);
+ amoid = get_table_am_oid(amname ? amname : default_table_access_method,
+ false);
if (rel->rd_rel->relam == amoid)
return;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 130f7fc7c3..c6e2f679fd 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -338,6 +338,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type alter_identity_column_option_list
%type alter_identity_column_option
%type set_statistics_value
+%type set_access_method_name
%type createdb_opt_list createdb_opt_items copy_opt_list
transaction_mode_list
@@ -2859,8 +2860,8 @@ alter_table_cmd:
n->newowner = $3;
$$ = (Node *) n;
}
- /* ALTER TABLE SET ACCESS METHOD */
- | SET ACCESS METHOD name
+ /* ALTER TABLE SET ACCESS METHOD { | DEFAULT } */
+ | SET ACCESS METHOD set_access_method_name
{
AlterTableCmd *n = makeNode(AlterTableCmd);
@@ -3076,6 +3077,11 @@ set_statistics_value:
| DEFAULT { $$ = NULL; }
;
+set_access_method_name:
+ ColId { $$ = $1; }
+ | DEFAULT { $$ = NULL; }
+ ;
+
PartitionBoundSpec:
/* a HASH partition */
FOR VALUES WITH '(' hash_partbound ')'
diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out
index b50293d514..e843d39ee7 100644
--- a/src/test/regress/expected/create_am.out
+++ b/src/test/regress/expected/create_am.out
@@ -283,6 +283,27 @@ SELECT COUNT(a), COUNT(1) FILTER(WHERE a=1) FROM heaptable;
9 | 1
(1 row)
+-- DEFAULT access method
+BEGIN;
+SET LOCAL default_table_access_method TO heap2;
+ALTER TABLE heaptable SET ACCESS METHOD DEFAULT;
+SELECT amname FROM pg_class c, pg_am am
+ WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass;
+ amname
+
+ heap2
+(1 row)
+
+SET LOCAL default_table_access_method TO heap;
+ALTER TABLE heaptable SET ACCESS METHOD DEFAULT;
+SELECT amname FROM pg_class c, pg_am am
+ WHERE c.relam =
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Wed, Feb 28, 2024 at 05:08:49PM +0900, Michael Paquier wrote: > On Wed, Feb 21, 2024 at 08:46:48AM +0100, Peter Eisentraut wrote: > > Yes, I think most people agreed that that would be the preferred behavior. > > Challenge accepted. As of the patch attached. Thanks for picking it up. I find it pretty hard to switch back to put the needed effort into a patch after a long period. > I have implemented that so as we keep the default, historical > behavior: if pg_class.relam is 0 for a partitioned table, use the AM > defined by default_table_access_method. The patch only adds a path to > switch to a different AM than the GUC when creating a new partition if > and only if a partitioned table has been manipulated with ALTER TABLE > SET ACCESS METHOD to update its AM to something else than the GUC. > Similarly to tablespaces, CREATE TABLE USING is *not* supported for > partitioned tables, same behavior as previously. This patch allows resetting relam=0 by running ALTER TABLE SET AM to the same value as the GUC. Maybe it'd be better to have an explicit SET DEFAULT (as in b9424d01 and 4f622503). -- Justin
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Wed, Feb 21, 2024 at 08:46:48AM +0100, Peter Eisentraut wrote: > Yes, I think most people agreed that that would be the preferred behavior. Challenge accepted. As of the patch attached. Tablespaces rely MyDatabaseTableSpace to fallback to the database's default if not specified, but we cannot do that for table AMs as there is no equivalent to dattablespace. I have implemented that so as we keep the default, historical behavior: if pg_class.relam is 0 for a partitioned table, use the AM defined by default_table_access_method. The patch only adds a path to switch to a different AM than the GUC when creating a new partition if and only if a partitioned table has been manipulated with ALTER TABLE SET ACCESS METHOD to update its AM to something else than the GUC. Similarly to tablespaces, CREATE TABLE USING is *not* supported for partitioned tables, same behavior as previously. There is a bit more regarding the handling of the entries in pg_depend, but nothing really complicated, knowing that there can be three possible patterns: - Add a new dependency if changing the AM to be something different than the GUC. - Remove the dependency if changing the AM to the value of the GUC, when something existing previously. - Update the dependency if switching between AMs that don't refer to the GUC at all. If the AM of a partitioned table is not changed, there is no need to update the catalogs at all. The prep phase of the sub-command is already aware of that, setting the new AM OID to InvalidOid in this case. The attached includes regression tests that check all the dependency entries, the contents of pg_class for partitioned tables, as well as the creation of partitions when pg_class.relam is not 0. I'd welcome more eyes regarding these changes. pg_dump needs to be tweaked to save the AM information of a partitioned table, like the previous versions. There are tests for these dump patterns, that needed a slight tweak to work. Docs have been refreshed. Thoughts, comments? -- Michael From 4309cd0806f92c573bb362f0da92eba95c34a827 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 28 Feb 2024 17:06:16 +0900 Subject: [PATCH v2] Allow specifying access method of partitioned tables ..to be inherited by partitions See also: ca4103025dfe26eaaf6a500dec9170fbb176eebc 8586bf7ed8889f39a59dd99b292014b73be85342 ebfe2dbd6b624e2a428e14b7ee9322cc096f63f7 - prevent DROP AM Authors: Justin Pryzby, Soumyadeep Chakraborty --- src/include/catalog/pg_class.h | 4 +- src/include/utils/lsyscache.h | 1 + src/backend/commands/tablecmds.c| 138 +--- src/backend/utils/cache/lsyscache.c | 22 src/backend/utils/cache/relcache.c | 7 ++ src/bin/pg_dump/pg_dump.c | 3 +- src/bin/pg_dump/t/002_pg_dump.pl| 35 ++ src/test/regress/expected/create_am.out | 100 - src/test/regress/sql/create_am.sql | 54 +- doc/src/sgml/catalogs.sgml | 5 +- doc/src/sgml/ref/alter_table.sgml | 8 ++ doc/src/sgml/ref/create_table.sgml | 4 + 12 files changed, 357 insertions(+), 24 deletions(-) diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h index 3b7533e7bb..304241923b 100644 --- a/src/include/catalog/pg_class.h +++ b/src/include/catalog/pg_class.h @@ -219,7 +219,9 @@ MAKE_SYSCACHE(RELNAMENSP, pg_class_relname_nsp_index, 128); /* * Relation kinds with a table access method (rd_tableam). Although sequences * use the heap table AM, they are enough of a special case in most uses that - * they are not included here. + * they are not included here. Likewise, partitioned tables can have an access + * method defined so as their children can inherit it; however, this is handled + * specially outside of this macro. */ #define RELKIND_HAS_TABLE_AM(relkind) \ ((relkind) == RELKIND_RELATION || \ diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h index e4a200b00e..35a8dec2b9 100644 --- a/src/include/utils/lsyscache.h +++ b/src/include/utils/lsyscache.h @@ -139,6 +139,7 @@ extern char get_rel_relkind(Oid relid); extern bool get_rel_relispartition(Oid relid); extern Oid get_rel_tablespace(Oid relid); extern char get_rel_persistence(Oid relid); +extern Oid get_rel_relam(Oid relid); extern Oid get_transform_fromsql(Oid typid, Oid langid, List *trftypes); extern Oid get_transform_tosql(Oid typid, Oid langid, List *trftypes); extern bool get_typisdefined(Oid typid); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index f798794556..4b4b34278e 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -592,6 +592,7 @@ static ObjectAddress ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode); static void ATExecDropCluster(Relation rel, LOCKMODE lockmode); static void ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname);
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 21.02.24 07:40, Michael Paquier wrote: This means that all partitioned tables would have pg_class.relam set, and that relam would never be 0: - The USING clause takes priority over default_table_access_method. - If no USING clause, default_table_access_method is the AM used Any partitions created from this partitioned table would inherit the AM set, ignoring default_table_access_method. Alvaro has made a very good point a couple of days ago at [1] where we should try to make the behavior stick closer to tablespaces, where it could be possible to set relam to 0 for a partitioned table, where a partition would inherit the AM set in the GUC when a USING clause is not defined (if USING specifies the AM, we'd just use it). Yes, I think most people agreed that that would be the preferred behavior.
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Tue, Feb 20, 2024 at 03:47:46PM +0100, Peter Eisentraut wrote:
> It would be helpful if this patch could more extensively document in its
> commit message what semantic changes it makes. Various options of possible
> behaviors were discussed in this thread, but it's not clear which behaviors
> were chosen in this particular patch version.
>
> The general idea is that you can set an access method on a partitioned
> table. That much seems very agreeable. But then what happens with this
> setting, how can you override it, how can you change it, what happens when
> you change it, what happens with existing partitions and new partitions,
> etc. -- and which of these behaviors are new and old. Many things to
> specify.
The main point in this patch is the following code block in
DefineRelation(), that defines the semantics about the AM set for a
partitioned table:
+else if (RELKIND_HAS_TABLE_AM(relkind) || relkind ==
RELKIND_PARTITIONED_TABLE)
{
+if (stmt->partbound)
+{
+/*
+ * For partitions, if no access method is specified, use the AM of
+ * the parent table.
+ */
+Assert(list_length(inheritOids) == 1);
+accessMethodId = get_rel_relam(linitial_oid(inheritOids));
+Assert(OidIsValid(accessMethodId));
+}
+else
+accessMethodId = get_table_am_oid(default_table_access_method,
false);
}
This means that all partitioned tables would have pg_class.relam set,
and that relam would never be 0:
- The USING clause takes priority over default_table_access_method.
- If no USING clause, default_table_access_method is the AM used
Any partitions created from this partitioned table would inherit the
AM set, ignoring default_table_access_method.
Alvaro has made a very good point a couple of days ago at [1] where we
should try to make the behavior stick closer to tablespaces, where it
could be possible to set relam to 0 for a partitioned table, where a
partition would inherit the AM set in the GUC when a USING clause is
not defined (if USING specifies the AM, we'd just use it).
Existing partitions should not be changed if the AM of their
partitioned table changes, so you can think of the AM as a hint for
the creation of new partitions.
[1]:
https://www.postgresql.org/message-id/202402011550.sfszd46247zi@alvherre.pgsql
--
Michael
signature.asc
Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On 19.07.23 20:13, Justin Pryzby wrote: On Thu, Jun 01, 2023 at 08:50:50AM -0400, Michael Paquier wrote: On Wed, May 31, 2023 at 06:35:34PM -0500, Justin Pryzby wrote: What do you think the comment ought to say ? It already says: src/backend/catalog/heap.c- * Make a dependency link to force the relation to be deleted if its src/backend/catalog/heap.c- * access method is. This is the third location where we rely on the fact that RELKIND_HAS_TABLE_AM() does not include RELKIND_PARTITIONED_TABLE, so it seems worth documenting what we are relying on as a comment? Say: * Make a dependency link to force the relation to be deleted if its * access method is. * * No need to add an explicit dependency for the toast table, as the * main table depends on it. Partitioned tables have a table access * method defined, and RELKIND_HAS_TABLE_AM ignores them. You said that this location "relies on" the macro not including partitioned tables, but I would say the opposite: the places that use RELKIND_HAS_TABLE_AM() and do *not* say "or relkind==PARTITIONED_TABLE" are the ones that "rely on" that... Anyway, this updates various comments. No other changes. It would be helpful if this patch could more extensively document in its commit message what semantic changes it makes. Various options of possible behaviors were discussed in this thread, but it's not clear which behaviors were chosen in this particular patch version. The general idea is that you can set an access method on a partitioned table. That much seems very agreeable. But then what happens with this setting, how can you override it, how can you change it, what happens when you change it, what happens with existing partitions and new partitions, etc. -- and which of these behaviors are new and old. Many things to specify.
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Thu, Feb 1, 2024 at 10:51 AM Alvaro Herrera wrote: > I think this works similarly but not identically to tablespace defaults, > and the difference could be confusing. You seem to have made it so that > the partitioned table _always_ have a table AM, so the partitions can > always inherit from it. I think it would be more sensible to _allow_ > partitioned tables to have one, but not mandatory; if they don't have > it, then a partition created from it would use default_table_access_method. I agree that we don't want this feature to invent any new behavior. If it's clearly and fully parallel to what we do for tablespaces, then I think it's probably OK, but anything less than that would be a cause for concern for me. -- Robert Haas EDB: http://www.enterprisedb.com
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Thu, Feb 01, 2024 at 04:50:49PM +0100, Alvaro Herrera wrote: > I think this works similarly but not identically to tablespace defaults, > and the difference could be confusing. You seem to have made it so that > the partitioned table _always_ have a table AM, so the partitions can > always inherit from it. I think it would be more sensible to _allow_ > partitioned tables to have one, but not mandatory; if they don't have > it, then a partition created from it would use default_table_access_method. You mean to allow a value of 0 in pg_class.relam on a partitioned table to allow any partitions created on it to use the default AM in the GUC when the partition is created? Yes, this inconsistency was bothering me as well in the patch. -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
> @@ -947,20 +947,22 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid
> ownerId,
>* a type of relation that needs one, use the default.
>*/
> if (stmt->accessMethod != NULL)
> + accessMethodId = get_table_am_oid(stmt->accessMethod, false);
> + else if (RELKIND_HAS_TABLE_AM(relkind) || relkind ==
> RELKIND_PARTITIONED_TABLE)
> {
> - accessMethod = stmt->accessMethod;
> -
> - if (partitioned)
> - ereport(ERROR,
> - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> - errmsg("specifying a table access
> method is not supported on a partitioned table")));
> + if (stmt->partbound)
> + {
> + /*
> + * For partitions, if no access method is specified,
> use the AM of
> + * the parent table.
> + */
> + Assert(list_length(inheritOids) == 1);
> + accessMethodId =
> get_rel_relam(linitial_oid(inheritOids));
> + Assert(OidIsValid(accessMethodId));
> + }
> + else
> + accessMethodId =
> get_table_am_oid(default_table_access_method, false);
> }
I think this works similarly but not identically to tablespace defaults,
and the difference could be confusing. You seem to have made it so that
the partitioned table _always_ have a table AM, so the partitions can
always inherit from it. I think it would be more sensible to _allow_
partitioned tables to have one, but not mandatory; if they don't have
it, then a partition created from it would use default_table_access_method.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree. (Don Knuth)
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
2024-01 Commitfest.
Hi, this patch was marked in CF as "Needs Review" [1], but there has
been no activity on this thread for 6+ months.
Is anything else planned, or can you post something to elicit more
interest in reviews for the latest patch? Otherwise, if nothing
happens then the CF entry will be closed ("Returned with feedback") at
the end of this CF.
==
[1] https://commitfest.postgresql.org/46/3727/
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Thu, Jun 01, 2023 at 08:50:50AM -0400, Michael Paquier wrote:
> On Wed, May 31, 2023 at 06:35:34PM -0500, Justin Pryzby wrote:
> > What do you think the comment ought to say ? It already says:
> >
> > src/backend/catalog/heap.c- * Make a dependency link to force
> > the relation to be deleted if its
> > src/backend/catalog/heap.c- * access method is.
>
> This is the third location where we rely on the fact that
> RELKIND_HAS_TABLE_AM() does not include RELKIND_PARTITIONED_TABLE, so
> it seems worth documenting what we are relying on as a comment? Say:
> * Make a dependency link to force the relation to be deleted if its
> * access method is.
> *
> * No need to add an explicit dependency for the toast table, as the
> * main table depends on it. Partitioned tables have a table access
> * method defined, and RELKIND_HAS_TABLE_AM ignores them.
You said that this location "relies on" the macro not including
partitioned tables, but I would say the opposite: the places that use
RELKIND_HAS_TABLE_AM() and do *not* say "or relkind==PARTITIONED_TABLE"
are the ones that "rely on" that...
Anyway, this updates various comments. No other changes.
--
Justin
>From e96a2a109d25bb9ed7cc9c0bf80c0824128dceac Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sun, 7 Mar 2021 00:11:38 -0600
Subject: [PATCH] Allow specifying access method of partitioned tables..
..to be inherited by partitions
See also:
ca4103025dfe26eaaf6a500dec9170fbb176eebc
8586bf7ed8889f39a59dd99b292014b73be85342
ebfe2dbd6b624e2a428e14b7ee9322cc096f63f7 - prevent DROP AM
Authors: Justin Pryzby, Soumyadeep Chakraborty
---
doc/src/sgml/ref/alter_table.sgml | 4 ++
doc/src/sgml/ref/create_table.sgml | 9 ++-
src/backend/catalog/heap.c | 6 +-
src/backend/commands/tablecmds.c| 89 +++--
src/backend/utils/cache/lsyscache.c | 22 ++
src/backend/utils/cache/relcache.c | 5 ++
src/bin/pg_dump/pg_dump.c | 3 +-
src/bin/pg_dump/t/002_pg_dump.pl| 34 ++
src/include/catalog/pg_class.h | 3 +-
src/include/utils/lsyscache.h | 1 +
src/test/regress/expected/create_am.out | 62 -
src/test/regress/sql/create_am.sql | 25 +--
12 files changed, 213 insertions(+), 50 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index d4d93eeb7c6..d32d4c44b10 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -719,6 +719,10 @@ WITH ( MODULUS numeric_literal, REM
This form changes the access method of the table by rewriting it. See
for more information.
+ When applied to a partitioned table, there is no data to rewrite, but any
+ partitions created afterwards with
+ CREATE TABLE PARTITION OF will use that access method,
+ unless overridden by an USING clause.
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 10ef699fab9..b20d272b151 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1295,9 +1295,12 @@ WITH ( MODULUS numeric_literal, REM
This optional clause specifies the table access method to use to store
the contents for the new table; the method needs be an access method of
type TABLE. See for more
- information. If this option is not specified, the default table access
- method is chosen for the new table. See for more information.
+ information. If this option is not specified, a default table access
+ method is chosen for the new table.
+ When creating a partition, the default table access method is the
+ access method of its parent.
+ For other tables, the default is determined by
+ .
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 4c30c7d461f..82e003cabc1 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1450,9 +1450,11 @@ heap_create_with_catalog(const char *relname,
* access method is.
*
* No need to add an explicit dependency for the toast table, as the
- * main table depends on it.
+ * main table depends on it. Partitioned tables have an access method
+ * defined, but are not handled by RELKIND_HAS_TABLE_AM.
*/
- if (RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE)
+ if ((RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) ||
+relkind == RELKIND_PARTITIONED_TABLE)
{
ObjectAddressSet(referenced, AccessMethodRelationId, accessmtd);
add_exact_object_address(, addrs);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4dc029f91f1..bc343539aeb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -573,6 +573,7 @@ static ObjectAddress ATExecClusterOn(Relation rel, const char *indexName,
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Sun, Jul 16, 2023 at 09:37:28PM -0500, Justin Pryzby wrote: > I understand that it's possible for it to be conditional, but I don't > undertand why it's desirable/important ? Because it's cheaper on repeated commands, like no CCI necessary. > It's not conditional in the tablespace code that this follows, nor > others like ATExecSetStatistics(), ATExecForceNoForceRowSecurity(), > ATExecSetCompression(), etc, some of which are recently-added. If it's > important to do here, isn't it also important to do everywhere else ? Good point here. I am OK to discard this point. -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Thu, Jun 01, 2023 at 08:50:50AM -0400, Michael Paquier wrote: > >> Speaking of which, ATExecSetAccessMethodNoStorage() does a catalog > >> update even if ALTER TABLE is defined to use the same table AM as what > >> is currently set. There is no need to update the relation's pg_class > >> entry in this case. Be careful that InvokeObjectPostAlterHook() still > >> needs to be checked in this case. Perhaps some tests should be added > >> in test_oat_hooks.sql? It would be tempted to add a new SQL file for > >> that. > > > > Are you suggesting to put this in a conditional: if > > oldrelam!=newAccessMethod ? > > Yes, that's what I would add with a few lines close to the beginning > of ATExecSetTableSpaceNoStorage() to save from catalog updates if > these are not needed. I understand that it's possible for it to be conditional, but I don't undertand why it's desirable/important ? It still seems preferable to be unconditional. On Wed, May 31, 2023 at 06:35:34PM -0500, Justin Pryzby wrote: >> Why is that desirable ? I'd prefer to see it written with fewer >> conditionals, not more; then, it hits the same path every time. It's not conditional in the tablespace code that this follows, nor others like ATExecSetStatistics(), ATExecForceNoForceRowSecurity(), ATExecSetCompression(), etc, some of which are recently-added. If it's important to do here, isn't it also important to do everywhere else ? -- Justin
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Have you had a chance to address the comments raised by Michael in his last review such that a new patch revision can be submitted? -- Daniel Gustafsson
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Wed, May 31, 2023 at 06:35:34PM -0500, Justin Pryzby wrote: > What do you think the comment ought to say ? It already says: > > src/backend/catalog/heap.c- * Make a dependency link to force > the relation to be deleted if its > src/backend/catalog/heap.c- * access method is. This is the third location where we rely on the fact that RELKIND_HAS_TABLE_AM() does not include RELKIND_PARTITIONED_TABLE, so it seems worth documenting what we are relying on as a comment? Say: * Make a dependency link to force the relation to be deleted if its * access method is. * * No need to add an explicit dependency for the toast table, as the * main table depends on it. Partitioned tables have a table access * method defined, and RELKIND_HAS_TABLE_AM ignores them. >> Speaking of which, ATExecSetAccessMethodNoStorage() does a catalog >> update even if ALTER TABLE is defined to use the same table AM as what >> is currently set. There is no need to update the relation's pg_class >> entry in this case. Be careful that InvokeObjectPostAlterHook() still >> needs to be checked in this case. Perhaps some tests should be added >> in test_oat_hooks.sql? It would be tempted to add a new SQL file for >> that. > > Are you suggesting to put this in a conditional: if oldrelam!=newAccessMethod > ? Yes, that's what I would add with a few lines close to the beginning of ATExecSetTableSpaceNoStorage() to save from catalog updates if these are not needed. -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Thu, May 25, 2023 at 03:49:12PM +0900, Michael Paquier wrote: > looking at the patch. Here are a few comments. ... > * No need to add an explicit dependency for the toast table, as the > * main table depends on it. > */ > - if (RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) > + if ((RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) > || > + relkind == RELKIND_PARTITIONED_TABLE) > > The comment at the top of this code block needs an update. What do you think the comment ought to say ? It already says: src/backend/catalog/heap.c- * Make a dependency link to force the relation to be deleted if its src/backend/catalog/heap.c- * access method is. > Speaking of which, ATExecSetAccessMethodNoStorage() does a catalog > update even if ALTER TABLE is defined to use the same table AM as what > is currently set. There is no need to update the relation's pg_class > entry in this case. Be careful that InvokeObjectPostAlterHook() still > needs to be checked in this case. Perhaps some tests should be added > in test_oat_hooks.sql? It would be tempted to add a new SQL file for > that. Are you suggesting to put this in a conditional: if oldrelam!=newAccessMethod ? + ((Form_pg_class) GETSTRUCT(tuple))->relam = newAccessMethod; + CatalogTupleUpdate(pg_class, >t_self, tuple); + + /* Update dependency on new AM */ + changeDependencyFor(RelationRelationId, relid, AccessMethodRelationId, + oldrelam, newAccessMethod); Why is that desirable ? I'd prefer to see it written with fewer conditionals, not more; then, it hits the same path every time. This ought to address your other comments. -- Justin >From a726bd7ca8844f6eee639d672afa7edace329caf Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Sun, 7 Mar 2021 00:11:38 -0600 Subject: [PATCH] Allow specifying access method of partitioned tables.. ..to be inherited by partitions See also: ca4103025dfe26eaaf6a500dec9170fbb176eebc 8586bf7ed8889f39a59dd99b292014b73be85342 ebfe2dbd6b624e2a428e14b7ee9322cc096f63f7 - prevent DROP AM Authors: Justin Pryzby, Soumyadeep Chakraborty --- doc/src/sgml/ref/alter_table.sgml | 4 ++ doc/src/sgml/ref/create_table.sgml | 9 ++- src/backend/catalog/heap.c | 3 +- src/backend/commands/tablecmds.c| 89 +++-- src/backend/utils/cache/lsyscache.c | 22 ++ src/backend/utils/cache/relcache.c | 5 ++ src/bin/pg_dump/pg_dump.c | 3 +- src/bin/pg_dump/t/002_pg_dump.pl| 34 ++ src/include/catalog/pg_class.h | 4 +- src/include/utils/lsyscache.h | 1 + src/test/regress/expected/create_am.out | 62 - src/test/regress/sql/create_am.sql | 25 +-- 12 files changed, 212 insertions(+), 49 deletions(-) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index d4d93eeb7c6..d32d4c44b10 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -719,6 +719,10 @@ WITH ( MODULUS numeric_literal, REM This form changes the access method of the table by rewriting it. See for more information. + When applied to a partitioned table, there is no data to rewrite, but any + partitions created afterwards with + CREATE TABLE PARTITION OF will use that access method, + unless overridden by an USING clause. diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml index 10ef699fab9..b20d272b151 100644 --- a/doc/src/sgml/ref/create_table.sgml +++ b/doc/src/sgml/ref/create_table.sgml @@ -1295,9 +1295,12 @@ WITH ( MODULUS numeric_literal, REM This optional clause specifies the table access method to use to store the contents for the new table; the method needs be an access method of type TABLE. See for more - information. If this option is not specified, the default table access - method is chosen for the new table. See for more information. + information. If this option is not specified, a default table access + method is chosen for the new table. + When creating a partition, the default table access method is the + access method of its parent. + For other tables, the default is determined by + . diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 2a0d82aedd7..bbf8e08618b 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -1452,7 +1452,8 @@ heap_create_with_catalog(const char *relname, * No need to add an explicit dependency for the toast table, as the * main table depends on it. */ - if (RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) + if ((RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) || +relkind ==
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Mon, Apr 24, 2023 at 07:18:54PM -0500, Justin Pryzby wrote:
> On Thu, Mar 30, 2023 at 12:07:58AM -0500, Justin Pryzby wrote:
>> Actually .. I think it'd be a mistake if the relam needed to be
>> interpretted differently for partitioned tables than other relkinds.
>>
>> I updated the patch to allow intermediate partitioned tables to inherit
>> relam from their parent.
>
> Michael ?
Sorry for dropping the subject for so long. I have spent some time
looking at the patch. Here are a few comments.
pg_class.h includes the following:
/*
* Relation kinds that support tablespaces: All relation kinds with storage
* support tablespaces, except that we don't support moving sequences around
* into different tablespaces. Partitioned tables and indexes don't have
* physical storage, but they have a tablespace settings so that their
* children can inherit it.
*/
#define RELKIND_HAS_TABLESPACE(relkind) \
((RELKIND_HAS_STORAGE(relkind) || RELKIND_HAS_PARTITIONS(relkind)) \
&& (relkind) != RELKIND_SEQUENCE)
[...]
/*
* Relation kinds with a table access method (rd_tableam). Although sequences
* use the heap table AM, they are enough of a special case in most uses that
* they are not included here.
*/
#define RELKIND_HAS_TABLE_AM(relkind) \
((relkind) == RELKIND_RELATION || \
(relkind) == RELKIND_TOASTVALUE || \
(relkind) == RELKIND_MATVIEW)
It would look much more consistent with the tablespace case if we
included partitioned tables in this case, but this refers to
rd_tableam for the relcache which we surely don't want to fill for
partitioned tables. I guess that at minimum a comment is in order?
RELKIND_HAS_TABLE_AM() is much more spread than
RELKIND_HAS_TABLESPACE().
* No need to add an explicit dependency for the toast table, as the
* main table depends on it.
*/
- if (RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE)
+ if ((RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) ||
+ relkind == RELKIND_PARTITIONED_TABLE)
The comment at the top of this code block needs an update.
if (stmt->accessMethod != NULL)
+ accessMethodId = get_table_am_oid(stmt->accessMethod, false);
else if (stmt->partbound &&
+ (RELKIND_HAS_TABLE_AM(relkind) || relkind ==
RELKIND_PARTITIONED_TABLE))
{
+ /*
+* For partitions, if no access method is specified, default to the AM
+* of the parent table.
+*/
+ Assert(list_length(inheritOids) == 1);
+ accessMethodId = get_rel_relam(linitial_oid(inheritOids));
+ if (!OidIsValid(accessMethodId))
+ accessMethodId = get_table_am_oid(default_table_access_method,
false);
}
+ else if (RELKIND_HAS_TABLE_AM(relkind) || relkind ==
RELKIND_PARTITIONED_TABLE)
+ accessMethodId = get_table_am_oid(default_table_access_method, false);
This structure seems a bit weird. Could it be cleaner to group the
second and third blocks together? I would imagine:
if (accessMethod != NULL)
{
//Extract the AM defined in the statement
}
else
{
//This is a relkind that can use a default table AM.
if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE)
{
if (stmt->partbound)
{
//This is a partition, so look at what its partitioned
//table holds.
}
else
{
//No partition, grab the default.
}
}
}
+ /*
+* Only do this for partitioned tables, for which this is just a
+* catalog change. Tables with storage are handled by Phase 3.
+*/
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ATExecSetAccessMethodNoStorage(rel, tab->newAccessMethod);
Okay, there is a parallel with tablespaces in this logic.
Speaking of which, ATExecSetAccessMethodNoStorage() does a catalog
update even if ALTER TABLE is defined to use the same table AM as what
is currently set. There is no need to update the relation's pg_class
entry in this case. Be careful that InvokeObjectPostAlterHook() still
needs to be checked in this case. Perhaps some tests should be added
in test_oat_hooks.sql? It would be tempted to add a new SQL file for
that.
+ else if (relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ {
+ /* Do nothing: it's a catalog settings for partitions to inherit */
+ }
Actually, we could add an assertion telling that rd_rel->relam will
always be valid.
- if (RELKIND_HAS_TABLE_AM(tbinfo->relkind))
+ if (RELKIND_HAS_TABLE_AM(tbinfo->relkind) ||
+ tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
tableam = tbinfo->amname;
I have spent some time pondering on this particular change, concluding
that it should be OK. It passes my tests, as well.
+-- partition hierarchies
+-- upon ALTER, new children will inherit the new am, whereas the existing
+-- children will remain untouched
CREATE
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Thu, Mar 30, 2023 at 12:07:58AM -0500, Justin Pryzby wrote: > What more points ? There's nothing that's been raised here. In fact, > your message last week is the first comment since last June .. When I wrote this message, I felt like this may still be missing something in the area of dump/restore. Perhaps my feeling on the matter is wrong, so consider this as a self-reminder not to be taken seriously until I can have a closer look at what's proposed here for v17. :p -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Thu, Mar 30, 2023 at 12:07:58AM -0500, Justin Pryzby wrote: > On Mon, Mar 27, 2023 at 11:34:36PM -0500, Justin Pryzby wrote: > > On Tue, Mar 28, 2023 at 09:13:10AM +0900, Michael Paquier wrote: > > > On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote: > > > > Did you check dump and restore flows with partition > > > > trees and --no-table-access-method? Perhaps there should be > > > > some regression tests with partitioned tables? > > > > > > I was looking at the patch, and as I suspected the dumps generated > > > are forgetting to apply the AM to the partitioned tables. > > > > The patch said: > > > > + else if (RELKIND_HAS_TABLE_AM(relkind) || relkind == > > RELKIND_PARTITIONED_TABLE) > > > > pg_dump was missing a similar change that's conditional on > > RELKIND_HAS_TABLE_AM(). It looks like those are the only two places > > that need be be specially handled. > > > > I dug up my latest patch from 2021 and incorporated the changes from the > > 0001 patch here, and added a test case. > > > > I realized that one difference with tablespaces is that, as written, > > partitioned tables will *always* have an AM specified, and partitions > > will never use default_table_access_method. Is that what's intended ? > > > > Or do we need logic similar tablespaces, such that the relam of a > > partitioned table is set only if it differs from default_table_am ? > > Actually .. I think it'd be a mistake if the relam needed to be > interpretted differently for partitioned tables than other relkinds. > > I updated the patch to allow intermediate partitioned tables to inherit > relam from their parent. > > Michael wrote: > > .. and there are quite more points to consider. > > What more points ? There's nothing that's been raised here. In fact, > your message last week is the first comment since last June .. Michael ?
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Mon, Mar 27, 2023 at 11:34:36PM -0500, Justin Pryzby wrote:
> On Tue, Mar 28, 2023 at 09:13:10AM +0900, Michael Paquier wrote:
> > On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote:
> > > Did you check dump and restore flows with partition
> > > trees and --no-table-access-method? Perhaps there should be
> > > some regression tests with partitioned tables?
> >
> > I was looking at the patch, and as I suspected the dumps generated
> > are forgetting to apply the AM to the partitioned tables.
>
> The patch said:
>
> + else if (RELKIND_HAS_TABLE_AM(relkind) || relkind ==
> RELKIND_PARTITIONED_TABLE)
>
> pg_dump was missing a similar change that's conditional on
> RELKIND_HAS_TABLE_AM(). It looks like those are the only two places
> that need be be specially handled.
>
> I dug up my latest patch from 2021 and incorporated the changes from the
> 0001 patch here, and added a test case.
>
> I realized that one difference with tablespaces is that, as written,
> partitioned tables will *always* have an AM specified, and partitions
> will never use default_table_access_method. Is that what's intended ?
>
> Or do we need logic similar tablespaces, such that the relam of a
> partitioned table is set only if it differs from default_table_am ?
Actually .. I think it'd be a mistake if the relam needed to be
interpretted differently for partitioned tables than other relkinds.
I updated the patch to allow intermediate partitioned tables to inherit
relam from their parent.
Michael wrote:
> .. and there are quite more points to consider.
What more points ? There's nothing that's been raised here. In fact,
your message last week is the first comment since last June ..
--
Justin
>From a9f357d98b90df9ee0ec5f9f9f1ac083d653daaa Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sun, 7 Mar 2021 00:11:38 -0600
Subject: [PATCH] Allow specifying access method of partitioned tables..
..to be inherited by partitions
See also:
ca4103025dfe26eaaf6a500dec9170fbb176eebc
8586bf7ed8889f39a59dd99b292014b73be85342
ebfe2dbd6b624e2a428e14b7ee9322cc096f63f7 - prevent DROP AM
Authors: Justin Pryzby, Soumyadeep Chakraborty
---
doc/src/sgml/ref/create_table.sgml | 9 ++-
src/backend/catalog/heap.c | 3 +-
src/backend/commands/tablecmds.c| 88 +++--
src/backend/utils/cache/lsyscache.c | 22 +++
src/backend/utils/cache/relcache.c | 4 ++
src/bin/pg_dump/pg_dump.c | 3 +-
src/bin/pg_dump/t/002_pg_dump.pl| 16 -
src/include/utils/lsyscache.h | 1 +
src/test/regress/expected/create_am.out | 45 +
src/test/regress/sql/create_am.sql | 18 +++--
10 files changed, 167 insertions(+), 42 deletions(-)
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 10ef699fab9..b20d272b151 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1295,9 +1295,12 @@ WITH ( MODULUS numeric_literal, REM
This optional clause specifies the table access method to use to store
the contents for the new table; the method needs be an access method of
type TABLE. See for more
- information. If this option is not specified, the default table access
- method is chosen for the new table. See for more information.
+ information. If this option is not specified, a default table access
+ method is chosen for the new table.
+ When creating a partition, the default table access method is the
+ access method of its parent.
+ For other tables, the default is determined by
+ .
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 2a0d82aedd7..bbf8e08618b 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1452,7 +1452,8 @@ heap_create_with_catalog(const char *relname,
* No need to add an explicit dependency for the toast table, as the
* main table depends on it.
*/
- if (RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE)
+ if ((RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) ||
+relkind == RELKIND_PARTITIONED_TABLE)
{
ObjectAddressSet(referenced, AccessMethodRelationId, accessmtd);
add_exact_object_address(, addrs);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3147dddf286..71b8b0b2317 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -571,6 +571,7 @@ static ObjectAddress ATExecClusterOn(Relation rel, const char *indexName,
LOCKMODE lockmode);
static void ATExecDropCluster(Relation rel, LOCKMODE lockmode);
static void ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname);
+static void ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethod);
static bool ATPrepChangePersistence(Relation rel, bool toLogged);
static void
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Tue, Mar 28, 2023 at 02:56:28PM +0900, Michael Paquier wrote: > Hmm. This is a good point. It is true that the patch feels > incomplete on this side. I don't see why we could not be flexible, > and allow a value of 0 in a partitioned table's relam to mean that we > would pick up the database default in this case when a partition is > is created on it. This would have the advantage to be consistent with > older versions where we fallback on the default. We cannot be > completely consistent with the reltablespace of the leaf partitions > unfortunately, as relam should always be set if a relation has > storage. And allowing a value of 0 means that there are likely other > tricky cases with dumps? > > Another thing: would it make sense to allow an empty string in > default_table_access_method so as we'd always fallback to a database > default? FYI, I am not sure that I will be able to look more at this patch by the end of the commit fest, and there are quite more points to consider. Perhaps at this stage we'd better mark it as returned with feedback? I understand that I've arrived late at this party :/ -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Mon, Mar 27, 2023 at 11:34:35PM -0500, Justin Pryzby wrote: > I realized that one difference with tablespaces is that, as written, > partitioned tables will *always* have an AM specified, and partitions > will never use default_table_access_method. Is that what's intended ? > > Or do we need logic similar tablespaces, such that the relam of a > partitioned table is set only if it differs from default_table_am ? Hmm. This is a good point. It is true that the patch feels incomplete on this side. I don't see why we could not be flexible, and allow a value of 0 in a partitioned table's relam to mean that we would pick up the database default in this case when a partition is is created on it. This would have the advantage to be consistent with older versions where we fallback on the default. We cannot be completely consistent with the reltablespace of the leaf partitions unfortunately, as relam should always be set if a relation has storage. And allowing a value of 0 means that there are likely other tricky cases with dumps? Another thing: would it make sense to allow an empty string in default_table_access_method so as we'd always fallback to a database default? -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Tue, Mar 28, 2023 at 09:13:10AM +0900, Michael Paquier wrote:
> On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote:
> > Did you check dump and restore flows with partition
> > trees and --no-table-access-method? Perhaps there should be
> > some regression tests with partitioned tables?
>
> I was looking at the patch, and as I suspected the dumps generated
> are forgetting to apply the AM to the partitioned tables.
The patch said:
+ else if (RELKIND_HAS_TABLE_AM(relkind) || relkind ==
RELKIND_PARTITIONED_TABLE)
pg_dump was missing a similar change that's conditional on
RELKIND_HAS_TABLE_AM(). It looks like those are the only two places
that need be be specially handled.
I dug up my latest patch from 2021 and incorporated the changes from the
0001 patch here, and added a test case.
I realized that one difference with tablespaces is that, as written,
partitioned tables will *always* have an AM specified, and partitions
will never use default_table_access_method. Is that what's intended ?
Or do we need logic similar tablespaces, such that the relam of a
partitioned table is set only if it differs from default_table_am ?
--
Justin
>From 2c0c5068cf849ad91de8f5276a430b022d1243b0 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sun, 7 Mar 2021 00:11:38 -0600
Subject: [PATCH] Allow specifying access method of partitioned tables..
..to be inherited by partitions
See also:
ca4103025dfe26eaaf6a500dec9170fbb176eebc
8586bf7ed8889f39a59dd99b292014b73be85342
ebfe2dbd6b624e2a428e14b7ee9322cc096f63f7 - prevent DROP AM
Authors: Justin Pryzby, Soumyadeep Chakraborty
---
doc/src/sgml/ref/create_table.sgml | 9 ++-
src/backend/catalog/heap.c | 3 +-
src/backend/commands/tablecmds.c| 87 +++--
src/backend/utils/cache/lsyscache.c | 22 +++
src/backend/utils/cache/relcache.c | 4 ++
src/bin/pg_dump/pg_dump.c | 3 +-
src/bin/pg_dump/t/002_pg_dump.pl| 16 -
src/include/utils/lsyscache.h | 1 +
src/test/regress/expected/create_am.out | 39 +++
src/test/regress/sql/create_am.sql | 16 +++--
10 files changed, 158 insertions(+), 42 deletions(-)
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 10ef699fab9..b45632619e5 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1295,9 +1295,12 @@ WITH ( MODULUS numeric_literal, REM
This optional clause specifies the table access method to use to store
the contents for the new table; the method needs be an access method of
type TABLE. See for more
- information. If this option is not specified, the default table access
- method is chosen for the new table. See for more information.
+ information. If this option is not specified, a default table access
+ method is chosen for the new table.
+ When creating a partitioned table, the default table access method
+ is the access method of the parent.
+ For other tables, the default is determined by
+ .
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 4f006820b85..146d8b36b09 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1452,7 +1452,8 @@ heap_create_with_catalog(const char *relname,
* No need to add an explicit dependency for the toast table, as the
* main table depends on it.
*/
- if (RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE)
+ if ((RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) ||
+relkind == RELKIND_PARTITIONED_TABLE)
{
ObjectAddressSet(referenced, AccessMethodRelationId, accessmtd);
add_exact_object_address(, addrs);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c510a01fd8d..de1fe15206a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -571,6 +571,7 @@ static ObjectAddress ATExecClusterOn(Relation rel, const char *indexName,
LOCKMODE lockmode);
static void ATExecDropCluster(Relation rel, LOCKMODE lockmode);
static void ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname);
+static void ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethod);
static bool ATPrepChangePersistence(Relation rel, bool toLogged);
static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel,
const char *tablespacename, LOCKMODE lockmode);
@@ -680,7 +681,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
Oid ofTypeId;
ObjectAddress address;
LOCKMODE parentLockmode;
- const char *accessMethod = NULL;
Oid accessMethodId = InvalidOid;
/*
@@ -945,20 +945,20 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
* a type of relation that needs one, use the default.
*/
if (stmt->accessMethod != NULL)
+ accessMethodId =
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote: > Did you check dump and restore flows with partition > trees and --no-table-access-method? Perhaps there should be > some regression tests with partitioned tables? I was looking at the patch, and as I suspected the dumps generated are forgetting to apply the AM to the partitioned tables. For example, assuming that the default AM is heap, the patch creates child_0_10 with heap2 as AM, which is what we want: CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler; CREATE TABLE parent_tab (id int) PARTITION BY RANGE (id) USING heap2; CREATE TABLE child_0_10 PARTITION OF parent_tab FOR VALUES FROM (0) TO (10); However a dump produces that (content cut except for its most relevant parts): CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler; SET default_tablespace = ''; CREATE TABLE public.parent_tab ( id integer ) PARTITION BY RANGE (id); SET default_table_access_method = heap2; CREATE TABLE public.child_0_10 ( id integer ); This would restore the previous contents incorrectly, where parent_tab would use heap and child_0_10 would use heap2, causing any partitions created after the restore to use silently heap. This is going to require a logic similar to tablespaces, where generate SET commands on default_table_access_method so as --no-table-access-method in pg_dump and pg_restore are able to work correctly. Having tests to cover all that is a must-have. -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Thu, Jun 09, 2022 at 11:21:58AM -0700, Soumyadeep Chakraborty wrote: > Thanks. Fixed and rebased. I think that I am OK with the concept of this patch to use a partitioned table's relam as a reference when creating a partition rather than relying on the default GUC, in a way similar to tablespaces. One worry I see is that forcing a recursion on the leaves on ALTER TABLE could silently break partitions where multiple table AMs are used across separate partitions if ALTER TABLE SET ACCESS METHOD is used on one of the parents, though it seems like this is not something I would much worry about as now the command is an error. A second worry is that we would just break existing creation flows that rely on the GUC defining the default AM. This is worth a close lookup at pg_dump to make sure that we do things correctly with this patch in place.. Did you check dump and restore flows with partition trees and --no-table-access-method? Perhaps there should be some regression tests with partitioned tables? +/* + * For partitioned tables, when no access method is specified, we + * default to the parent table's AM. + */ +Assert(list_length(inheritOids) == 1); +/* XXX: should implement get_rel_relam? */ +relid = linitial_oid(inheritOids); +tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); +if (!HeapTupleIsValid(tup)) +elog(ERROR, "cache lookup failed for relation %u", relid); Having a wrapper for that could be useful, yes. We don't have any code paths that would directly need that now, from what I can see, though. This patch gives one reason to have one. -- Michael signature.asc Description: PGP signature
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Wed, May 18, 2022 at 6:26 PM Zhihong Yu wrote:
> + accessMethodId = ((Form_pg_class) GETSTRUCT(tup))->relam;
>
> - /* look up the access method, verify it is for a table */
> - if (accessMethod != NULL)
> - accessMethodId = get_table_am_oid(accessMethod, false);
> + if (!HeapTupleIsValid(tup))
> + elog(ERROR, "cache lookup failed for relation %u", relid);
>
> The validity check of tup should be done before fetching the value of
relam field.
Thanks. Fixed and rebased.
Regards,
Soumyadeep (VMware)
From a9fc146192430aa45224fb1e64bd95e0bb64fc00 Mon Sep 17 00:00:00 2001
From: Soumyadeep Chakraborty
Date: Thu, 9 Jun 2022 10:57:35 -0700
Subject: [PATCH v3 1/2] Allow ATSETAM on partition roots
Setting the access method on partition roots was disallowed. This commit
relaxes that restriction.
Summary of changes over original patch:
* Rebased
* Minor comment updates
* Add more test coverage
Authors: Justin Pryzby , Soumyadeep Chakraborty
---
src/backend/catalog/heap.c | 3 +-
src/backend/commands/tablecmds.c| 103 +++-
src/backend/utils/cache/relcache.c | 4 +
src/test/regress/expected/create_am.out | 50 +++-
src/test/regress/sql/create_am.sql | 23 +++---
5 files changed, 129 insertions(+), 54 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 1803194db94..4561f3b8c98 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1449,7 +1449,8 @@ heap_create_with_catalog(const char *relname,
* No need to add an explicit dependency for the toast table, as the
* main table depends on it.
*/
- if (RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE)
+ if ((RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) ||
+ relkind == RELKIND_PARTITIONED_TABLE)
{
ObjectAddressSet(referenced, AccessMethodRelationId, accessmtd);
add_exact_object_address(, addrs);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2de0ebacec3..9a5eabcac49 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -570,6 +570,7 @@ static ObjectAddress ATExecClusterOn(Relation rel, const char *indexName,
LOCKMODE lockmode);
static void ATExecDropCluster(Relation rel, LOCKMODE lockmode);
static void ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname);
+static void ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethod);
static bool ATPrepChangePersistence(Relation rel, bool toLogged);
static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel,
const char *tablespacename, LOCKMODE lockmode);
@@ -676,7 +677,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
Oid ofTypeId;
ObjectAddress address;
LOCKMODE parentLockmode;
- const char *accessMethod = NULL;
Oid accessMethodId = InvalidOid;
/*
@@ -938,20 +938,32 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
* a type of relation that needs one, use the default.
*/
if (stmt->accessMethod != NULL)
+ accessMethodId = get_table_am_oid(stmt->accessMethod, false);
+ else if (stmt->partbound && relkind == RELKIND_RELATION)
{
- accessMethod = stmt->accessMethod;
+ HeapTuple tup;
+ Oidrelid;
- if (partitioned)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("specifying a table access method is not supported on a partitioned table")));
- }
- else if (RELKIND_HAS_TABLE_AM(relkind))
- accessMethod = default_table_access_method;
+ /*
+ * For partitioned tables, when no access method is specified, we
+ * default to the parent table's AM.
+ */
+ Assert(list_length(inheritOids) == 1);
+ /* XXX: should implement get_rel_relam? */
+ relid = linitial_oid(inheritOids);
+ tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for relation %u", relid);
- /* look up the access method, verify it is for a table */
- if (accessMethod != NULL)
- accessMethodId = get_table_am_oid(accessMethod, false);
+ accessMethodId = ((Form_pg_class) GETSTRUCT(tup))->relam;
+
+ ReleaseSysCache(tup);
+
+ if (!OidIsValid(accessMethodId))
+ accessMethodId = get_table_am_oid(default_table_access_method, false);
+ }
+ else if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE)
+ accessMethodId = get_table_am_oid(default_table_access_method, false);
/*
* Create the relation. Inherited defaults and constraints are passed in
@@ -4693,13 +4705,6 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_SetAccessMethod: /* SET ACCESS METHOD */
ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_MATVIEW);
-
- /* partitioned tables don't have an access method */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-ereport(ERROR,
-
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Wed, May 18, 2022 at 5:49 PM Soumyadeep Chakraborty < soumyadeep2...@gmail.com> wrote: > > On Wed, May 18, 2022 at 4:14 PM Justin Pryzby > wrote: > > > I didn't look closely yet, but this comment is wrong: > > > > + * Since these have no storage the tablespace can be updated with a > simple > > > > + * metadata only operation to update the tablespace. > > > > > Good catch. Fixed. > > > It'd be convenient if AMs worked the same way (and a bit odd that they > don't). > > Note that in v15, pg_dump/restore now allow --no-table-am, an exact > parallel to > > --no-tablespace. > > I agree that ATSET AM should behave in a similar fashion to ATSET > tablespaces. > However, the way that ATSET tablespace currently behaves is not consistent > with > the ONLY clause. > > On a given partition root: > ALTER TABLE ONLY am_partitioned SET TABLESPACE ts; > has the same effect as: > ALTER TABLE am_partitioned SET TABLESPACE ts; > > We are missing out on the feature to set the AM/tablespace throughout the > partition hierarchy, with one command. > > Regards, > Soumyadeep (VMware) > > Hi, + accessMethodId = ((Form_pg_class) GETSTRUCT(tup))->relam; - /* look up the access method, verify it is for a table */ - if (accessMethod != NULL) - accessMethodId = get_table_am_oid(accessMethod, false); + if (!HeapTupleIsValid(tup)) + elog(ERROR, "cache lookup failed for relation %u", relid); The validity check of tup should be done before fetching the value of relam field. Cheers
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Wed, May 18, 2022 at 4:14 PM Justin Pryzby wrote:
> I didn't look closely yet, but this comment is wrong:
>
> + * Since these have no storage the tablespace can be updated with a
simple
> + * metadata only operation to update the tablespace.
Good catch. Fixed.
> It'd be convenient if AMs worked the same way (and a bit odd that they
don't).
> Note that in v15, pg_dump/restore now allow --no-table-am, an exact
parallel to
> --no-tablespace.
I agree that ATSET AM should behave in a similar fashion to ATSET
tablespaces.
However, the way that ATSET tablespace currently behaves is not consistent
with
the ONLY clause.
On a given partition root:
ALTER TABLE ONLY am_partitioned SET TABLESPACE ts;
has the same effect as:
ALTER TABLE am_partitioned SET TABLESPACE ts;
We are missing out on the feature to set the AM/tablespace throughout the
partition hierarchy, with one command.
Regards,
Soumyadeep (VMware)
From 3a4a78d46fc74bb3e9b7ac9aefc689d250e1ecf4 Mon Sep 17 00:00:00 2001
From: Soumyadeep Chakraborty
Date: Tue, 17 May 2022 15:22:48 -0700
Subject: [PATCH v2 2/2] Make ATSETAM recurse by default
ATSETAM now recurses to partition children by default. To prevent
recursion, and have the new am apply to future children only, users must
specify the ONLY clause.
---
src/backend/commands/tablecmds.c| 2 ++
src/test/regress/expected/create_am.out | 13 -
src/test/regress/sql/create_am.sql | 5 -
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5febd834d5b..c5bbb71c66d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4711,6 +4711,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot have multiple SET ACCESS METHOD subcommands")));
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
ATPrepSetAccessMethod(tab, rel, cmd->name);
pass = AT_PASS_MISC; /* does not matter; no work in Phase 2 */
break;
diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out
index a48199df9a2..e99de1ac912 100644
--- a/src/test/regress/expected/create_am.out
+++ b/src/test/regress/expected/create_am.out
@@ -284,7 +284,7 @@ CREATE TABLE am_partitioned(x INT, y INT)
CREATE TABLE am_partitioned_1 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 0);
CREATE TABLE am_partitioned_2 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 1);
ALTER TABLE am_partitioned_1 SET ACCESS METHOD heap2;
-ALTER TABLE am_partitioned SET ACCESS METHOD heap2;
+ALTER TABLE ONLY am_partitioned SET ACCESS METHOD heap2;
CREATE TABLE am_partitioned_3 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 2);
SELECT relname, amname FROM pg_class c, pg_am am
WHERE c.relam = am.oid AND c.relname LIKE 'am_partitioned%' ORDER BY 1;
@@ -296,6 +296,17 @@ SELECT relname, amname FROM pg_class c, pg_am am
am_partitioned_3 | heap2
(4 rows)
+ALTER TABLE am_partitioned SET ACCESS METHOD heap;
+SELECT relname, amname FROM pg_class c, pg_am am
+WHERE c.relam = am.oid AND c.relname LIKE 'am_partitioned%' ORDER BY 1;
+ relname | amname
+--+
+ am_partitioned | heap
+ am_partitioned_1 | heap
+ am_partitioned_2 | heap
+ am_partitioned_3 | heap
+(4 rows)
+
DROP TABLE am_partitioned;
-- Second, create objects in the new AM by changing the default AM
BEGIN;
diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/create_am.sql
index 8f3db03bba2..f4e22684782 100644
--- a/src/test/regress/sql/create_am.sql
+++ b/src/test/regress/sql/create_am.sql
@@ -184,10 +184,13 @@ CREATE TABLE am_partitioned(x INT, y INT)
CREATE TABLE am_partitioned_1 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 0);
CREATE TABLE am_partitioned_2 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 1);
ALTER TABLE am_partitioned_1 SET ACCESS METHOD heap2;
-ALTER TABLE am_partitioned SET ACCESS METHOD heap2;
+ALTER TABLE ONLY am_partitioned SET ACCESS METHOD heap2;
CREATE TABLE am_partitioned_3 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 2);
SELECT relname, amname FROM pg_class c, pg_am am
WHERE c.relam = am.oid AND c.relname LIKE 'am_partitioned%' ORDER BY 1;
+ALTER TABLE am_partitioned SET ACCESS METHOD heap;
+SELECT relname, amname FROM pg_class c, pg_am am
+WHERE c.relam = am.oid AND c.relname LIKE 'am_partitioned%' ORDER BY 1;
DROP TABLE am_partitioned;
-- Second, create objects in the new AM by changing the default AM
--
2.25.1
From 6edce14b5becb30ca7c12224acaabea62a4d999f Mon Sep 17 00:00:00 2001
From: Soumyadeep Chakraborty
Date: Mon, 16 May 2022 14:46:20 -0700
Subject: [PATCH v2 1/2] Allow ATSETAM on partition roots
Setting the access method on partition roots was disallowed.
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Thanks for copying me. I didn't look closely yet, but this comment is wrong: + * Since these have no storage the tablespace can be updated with a simple + * metadata only operation to update the tablespace. As I see it, AMs are a strong parallel to tablespaces. The default tablespace is convenient: 1) explicitly specified tablespace; 2) tablespace of parent, partitioned table; 3) DB tablespace; 4) default_tablespace: https://www.postgresql.org/message-id/20190423222633.GA8364%40alvherre.pgsql It'd be convenient if AMs worked the same way (and a bit odd that they don't). Note that in v15, pg_dump/restore now allow --no-table-am, an exact parallel to --no-tablespace. -- Justin
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Hi, + tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); + accessMethodId = ((Form_pg_class) GETSTRUCT(tup))->relam; - /* look up the access method, verify it is for a table */ - if (accessMethod != NULL) - accessMethodId = get_table_am_oid(accessMethod, false); + if (!HeapTupleIsValid(tup)) + elog(ERROR, "cache lookup failed for relation %u", relid); Shouldn't the validity of tup be checked before relam field is accessed ? Cheers
