On 2019-Mar-06, Andres Freund wrote:
> > I also find it far from clear that:
> > <listitem>
> > <para>
> > The <replaceable class="parameter">tablespace_name</replaceable> is
> > the name
> > of the tablespace in which the new table is to be created.
> > If not specified,
> > <xref linkend="guc-default-tablespace"/> is consulted, or
> > <xref linkend="guc-temp-tablespaces"/> if the table is temporary. For
> > partitioned tables, since no storage is required for the table itself,
> > the tablespace specified here only serves to mark the default
> > tablespace
> > for any newly created partitions when no other tablespace is
> > explicitly
> > specified.
> > </para>
> > </listitem>
> > is handled correctly. The above says that the *specified* tablespaces -
> > which seems to exclude the default tablespace - is what's going to
> > determine what partitions use as their default tablespace. But in fact
> > that's not true, the partitioned table's pg_class.retablespace is set to
> > what default_tablespaces was at the time of the creation.
>
> I still think the feature as is doesn't seem to have very well defined
> behaviour.
I have just started looking into this issue. I'm not sure yet what's
the best fix; maybe for the specific case of partitioned tables and
indexes we should deviate from the ages-old behavior of storing zero
tablespace, if the tablespace is specified as the default one. But I
haven't written the code yet.
In the meantime, here's David's patch, rebased to current master and
with the pg_upgrade and pg_dump tests fixed to match the new partition
creation behavior.
> > If we instead did:
> >
> > CREATE TABLE public.listp1 (a integer
> > );
> >
> > ALTER TABLE public.list1 ATTACH PARTITION public.listp FOR VALUES IN (1);
>
> Isn't that a bit more expensive, because now the table needs to be
> scanned for maching the value? That's probably neglegible though, given
> it'd probably always empty.
I think it's always empty. In the standard case, there are two
transactions rather than one, so yeah it's a little bit more expensive.
Maybe we should make this conditional on there actually being an
important tablespace distinction to preserve.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 0f889151e56a3b6ce7e522799bbbb8d644ca54c4 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Mon, 8 Apr 2019 16:40:05 -0400
Subject: [PATCH v2] Make pg_dump emit ATTACH PARTITION instead of PARTITION OF
ca41030 modified how the TABLESPACE option worked with partitioned tables
so that it was possible to specify and store a TABLESPACE for a partitioned
table. This setting served only to mark what the default tablespace should
be for new partitions which were created with the CREATE TABLE PARTITION OF
syntax.
The problem with this was that pg_dump used the PARTITION OF syntax which
meant that if a partition had been explicitly defined to have pg_default
as the tablespace then since reltablespace is set to 0 in this case, pg_dump
would emit a CREATE TABLE .. PARTITION OF statement and cause the partition
to default to the partitioned table's tablespace rather than the one it was
meant to be located in.
We can work around this problem by simply performing the CREATE TABLE first
then perform an ATTACH PARTITION later. This bypasses the check of the
parent partitioned table's tablespace in DefineRelation() as the table is
not a partition when it is defined.
Doing this also means that when restoring partitions they now maintain
their original column ordering rather than switch to their partitioned
table's column ordering. This is perhaps minor, but it is noteworthy
pg_dump in binary upgrade mode already worked this way, so it turns out
this commit removes more code than it adds.
---
src/bin/pg_dump/pg_dump.c | 118 ++++++++++++++-----------------
src/bin/pg_dump/t/002_pg_dump.pl | 12 +++-
2 files changed, 61 insertions(+), 69 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 452f30760bb..a7987790df5 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8618,9 +8618,11 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
* Normally this is always true, but it's false for dropped columns, as well
* as those that were inherited without any local definition. (If we print
* such a column it will mistakenly get pg_attribute.attislocal set to true.)
- * However, in binary_upgrade mode, we must print all such columns anyway and
- * fix the attislocal/attisdropped state later, so as to keep control of the
- * physical column order.
+ *
+ * In binary_upgrade mode, we must print all columns and fix the attislocal/
+ * attisdropped state later, so as to keep control of the physical column
+ * order. We also do this for partitions, for a different reason: we want
+ * them to be created independently, and ATTACH PARTITION used afterwards.
*
* This function exists because there are scattered nonobvious places that
* must be kept in sync with this decision.
@@ -8630,7 +8632,9 @@ shouldPrintColumn(DumpOptions *dopt, TableInfo *tbinfo, int colno)
{
if (dopt->binary_upgrade)
return true;
- return (tbinfo->attislocal[colno] && !tbinfo->attisdropped[colno]);
+ if (tbinfo->attisdropped[colno])
+ return false;
+ return (tbinfo->attislocal[colno] || tbinfo->ispartition);
}
@@ -15599,27 +15603,6 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
if (tbinfo->reloftype && !dopt->binary_upgrade)
appendPQExpBuffer(q, " OF %s", tbinfo->reloftype);
- /*
- * If the table is a partition, dump it as such; except in the case of
- * a binary upgrade, we dump the table normally and attach it to the
- * parent afterward.
- */
- if (tbinfo->ispartition && !dopt->binary_upgrade)
- {
- TableInfo *parentRel = tbinfo->parents[0];
-
- /*
- * With partitions, unlike inheritance, there can only be one
- * parent.
- */
- if (tbinfo->numParents != 1)
- fatal("invalid number of parents %d for table \"%s\"",
- tbinfo->numParents, tbinfo->dobj.name);
-
- appendPQExpBuffer(q, " PARTITION OF %s",
- fmtQualifiedDumpable(parentRel));
- }
-
if (tbinfo->relkind != RELKIND_MATVIEW)
{
/* Dump the attributes */
@@ -15648,12 +15631,9 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
(!tbinfo->inhNotNull[j] ||
dopt->binary_upgrade));
- /*
- * Skip column if fully defined by reloftype or the
- * partition parent.
- */
- if ((tbinfo->reloftype || tbinfo->ispartition) &&
- !has_default && !has_notnull && !dopt->binary_upgrade)
+ /* Skip column if fully defined by reloftype */
+ if (tbinfo->reloftype && !has_default && !has_notnull &&
+ !dopt->binary_upgrade)
continue;
/* Format properly if not first attr */
@@ -15676,7 +15656,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
* clean things up later.
*/
appendPQExpBufferStr(q, " INTEGER /* dummy */");
- /* Skip all the rest, too */
+ /* and skip to the next column */
continue;
}
@@ -15685,11 +15665,9 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
*
* In binary-upgrade mode, we always include the type. If
* we aren't in binary-upgrade mode, then we skip the type
- * when creating a typed table ('OF type_name') or a
- * partition ('PARTITION OF'), since the type comes from
- * the parent/partitioned table.
+ * when creating a typed table ('OF type_name').
*/
- if (dopt->binary_upgrade || (!tbinfo->reloftype && !tbinfo->ispartition))
+ if (dopt->binary_upgrade || !tbinfo->reloftype)
{
appendPQExpBuffer(q, " %s",
tbinfo->atttypnames[j]);
@@ -15746,23 +15724,19 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
if (actual_atts)
appendPQExpBufferStr(q, "\n)");
- else if (!((tbinfo->reloftype || tbinfo->ispartition) &&
- !dopt->binary_upgrade))
+ else if (!(tbinfo->reloftype && !dopt->binary_upgrade))
{
/*
- * We must have a parenthesized attribute list, even though
- * empty, when not using the OF TYPE or PARTITION OF syntax.
+ * No attributes? we must have a parenthesized attribute list,
+ * even though empty, when not using the OF TYPE syntax.
*/
appendPQExpBufferStr(q, " (\n)");
}
- if (tbinfo->ispartition && !dopt->binary_upgrade)
- {
- appendPQExpBufferChar(q, '\n');
- appendPQExpBufferStr(q, tbinfo->partbound);
- }
-
- /* Emit the INHERITS clause, except if this is a partition. */
+ /*
+ * Emit the INHERITS clause, except if this is a partition. When
+ * in binary upgrade mode we'll do this later in this function.
+ */
if (numParents > 0 &&
!tbinfo->ispartition &&
!dopt->binary_upgrade)
@@ -15937,30 +15911,16 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
appendPQExpBufferStr(q, "::pg_catalog.regclass;\n");
}
- if (numParents > 0)
+ if (numParents > 0 && !tbinfo->ispartition)
{
- appendPQExpBufferStr(q, "\n-- For binary upgrade, set up inheritance and partitioning this way.\n");
+ appendPQExpBufferStr(q, "\n-- For binary upgrade, set up inheritance this way.\n");
for (k = 0; k < numParents; k++)
{
TableInfo *parentRel = parents[k];
- /* In the partitioning case, we alter the parent */
- if (tbinfo->ispartition)
- appendPQExpBuffer(q,
- "ALTER TABLE ONLY %s ATTACH PARTITION ",
- fmtQualifiedDumpable(parentRel));
- else
- appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ",
- qualrelname);
-
- /* Partition needs specifying the bounds */
- if (tbinfo->ispartition)
- appendPQExpBuffer(q, "%s %s;\n",
- qualrelname,
- tbinfo->partbound);
- else
- appendPQExpBuffer(q, "%s;\n",
- fmtQualifiedDumpable(parentRel));
+ appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT %s;\n",
+ qualrelname,
+ fmtQualifiedDumpable(parentRel));
}
}
@@ -15973,6 +15933,32 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
}
}
+ /*
+ * For partitioned tables, emit the ATTACH PARTITION clause. Note
+ * that we always want to create partitions in this way instead using
+ * CREATE TABLE .. PARTITION OF, because doing it the other way would
+ * result in an incorrect tablespace setting for the partition, if the
+ * partitioned table has one and the partitioned table is created in
+ * a different one.
+ */
+ if (tbinfo->ispartition)
+ {
+ /*
+ * With partitions, unlike inheritance, there can only be one
+ * parent.
+ */
+ if (tbinfo->numParents != 1)
+ fatal("invalid number of parents %d for table \"%s\"",
+ tbinfo->numParents, tbinfo->dobj.name);
+
+ /* Perform ALTER TABLE on the parent */
+ appendPQExpBuffer(q, "ALTER TABLE ONLY %s ATTACH PARTITION ",
+ fmtQualifiedDumpable(parents[0]));
+
+ /* specify the bounds */
+ appendPQExpBuffer(q, "%s %s;\n", qualrelname, tbinfo->partbound);
+ }
+
/*
* In binary_upgrade mode, arrange to restore the old relfrozenxid and
* relminmxid of all vacuumable relations. (While vacuum.c processes
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 5721882b3b2..5c1acd014bf 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -733,7 +733,12 @@ my %tests = (
\QALTER TABLE ONLY dump_test.measurement ATTACH PARTITION dump_test_second_schema.measurement_y2006m2 \E
\QFOR VALUES FROM ('2006-02-01') TO ('2006-03-01');\E\n
/xm,
- like => { binary_upgrade => 1, },
+ like => {
+ %full_runs,
+ role => 1,
+ section_pre_data => 1,
+ binary_upgrade => 1,
+ },
},
'ALTER TABLE test_table CLUSTER ON test_table_pkey' => {
@@ -2322,12 +2327,13 @@ my %tests = (
\)\n
\QFOR VALUES FROM ('2006-02-01') TO ('2006-03-01');\E\n
/xm,
- like => {
+ like => {},
+ unlike => {
%full_runs,
role => 1,
section_pre_data => 1,
+ binary_upgrade => 1,
},
- unlike => { binary_upgrade => 1, },
},
'CREATE TABLE test_fourth_table_zero_col' => {
--
2.17.1
