public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v7 1/7] pg_dump: make CLUSTER ON a separate dump object..
8+ messages / 6 participants
[nested] [flat]
* [PATCH v7 1/7] pg_dump: make CLUSTER ON a separate dump object..
@ 2020-11-26 20:37 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Justin Pryzby @ 2020-11-26 20:37 UTC (permalink / raw)
..since it needs to be restored after any child indexes are restored *and
attached*. The order needs to be:
1) restore child and parent index (order doesn't matter);
2) attach child index;
3) set cluster on child and parent index (order doesn't matter);
---
src/bin/pg_dump/pg_dump.c | 86 ++++++++++++++++++++++++++--------
src/bin/pg_dump/pg_dump.h | 8 ++++
src/bin/pg_dump/pg_dump_sort.c | 8 ++++
3 files changed, 82 insertions(+), 20 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index d99b61e621..8dc8a42964 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -208,6 +208,7 @@ static void dumpSequence(Archive *fout, TableInfo *tbinfo);
static void dumpSequenceData(Archive *fout, TableDataInfo *tdinfo);
static void dumpIndex(Archive *fout, IndxInfo *indxinfo);
static void dumpIndexAttach(Archive *fout, IndexAttachInfo *attachinfo);
+static void dumpIndexClusterOn(Archive *fout, IndexClusterInfo *clusterinfo);
static void dumpStatisticsExt(Archive *fout, StatsExtInfo *statsextinfo);
static void dumpConstraint(Archive *fout, ConstraintInfo *coninfo);
static void dumpTableConstraintComment(Archive *fout, ConstraintInfo *coninfo);
@@ -7092,6 +7093,11 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
i_inddependcollversions;
int ntups;
+ int ncluster = 0;
+ IndexClusterInfo *clusterinfo;
+ clusterinfo = (IndexClusterInfo *)
+ pg_malloc0(numTables * sizeof(IndexClusterInfo));
+
for (i = 0; i < numTables; i++)
{
TableInfo *tbinfo = &tblinfo[i];
@@ -7471,6 +7477,27 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
/* Plain secondary index */
indxinfo[j].indexconstraint = 0;
}
+
+ /* Record each table's CLUSTERed index, if any */
+ if (indxinfo[j].indisclustered)
+ {
+ IndxInfo *index = &indxinfo[j];
+ IndexClusterInfo *cluster = &clusterinfo[ncluster];
+
+ cluster->dobj.objType = DO_INDEX_CLUSTER_ON;
+ cluster->dobj.catId.tableoid = 0;
+ cluster->dobj.catId.oid = 0;
+ AssignDumpId(&cluster->dobj);
+ cluster->dobj.name = pg_strdup(index->dobj.name);
+ cluster->dobj.namespace = index->indextable->dobj.namespace;
+ cluster->index = index;
+ cluster->indextable = &tblinfo[i];
+
+ /* The CLUSTER ON depends on its index.. */
+ addObjectDependency(&cluster->dobj, index->dobj.dumpId);
+
+ ncluster++;
+ }
}
PQclear(res);
@@ -10323,6 +10350,9 @@ dumpDumpableObject(Archive *fout, DumpableObject *dobj)
case DO_SUBSCRIPTION:
dumpSubscription(fout, (SubscriptionInfo *) dobj);
break;
+ case DO_INDEX_CLUSTER_ON:
+ dumpIndexClusterOn(fout, (IndexClusterInfo *) dobj);
+ break;
case DO_PRE_DATA_BOUNDARY:
case DO_POST_DATA_BOUNDARY:
/* never dumped, nothing to do */
@@ -16543,6 +16573,41 @@ getAttrName(int attrnum, TableInfo *tblInfo)
return NULL; /* keep compiler quiet */
}
+/*
+ * dumpIndexClusterOn
+ * record that the index is clustered.
+ */
+static void
+dumpIndexClusterOn(Archive *fout, IndexClusterInfo *clusterinfo)
+{
+ DumpOptions *dopt = fout->dopt;
+ TableInfo *tbinfo = clusterinfo->indextable;
+ char *qindxname;
+ PQExpBuffer q;
+
+ if (dopt->dataOnly)
+ return;
+
+ q = createPQExpBuffer();
+ qindxname = pg_strdup(fmtId(clusterinfo->dobj.name));
+
+ /* index name is not qualified in this syntax */
+ appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER ON %s;\n",
+ fmtQualifiedDumpable(tbinfo), qindxname);
+
+ if (clusterinfo->dobj.dump & DUMP_COMPONENT_DEFINITION)
+ ArchiveEntry(fout, clusterinfo->dobj.catId, clusterinfo->dobj.dumpId,
+ ARCHIVE_OPTS(.tag = clusterinfo->dobj.name,
+ .namespace = tbinfo->dobj.namespace->dobj.name,
+ .owner = tbinfo->rolname,
+ .description = "INDEX CLUSTER ON",
+ .section = SECTION_POST_DATA,
+ .createStmt = q->data));
+
+ destroyPQExpBuffer(q);
+ free(qindxname);
+}
+
/*
* dumpIndex
* write out to fout a user-defined index
@@ -16597,16 +16662,6 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
* similar code in dumpConstraint!
*/
- /* If the index is clustered, we need to record that. */
- if (indxinfo->indisclustered)
- {
- appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER",
- fmtQualifiedDumpable(tbinfo));
- /* index name is not qualified in this syntax */
- appendPQExpBuffer(q, " ON %s;\n",
- qindxname);
- }
-
/*
* If the index has any statistics on some of its columns, generate
* the associated ALTER INDEX queries.
@@ -16933,16 +16988,6 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo)
* similar code in dumpIndex!
*/
- /* If the index is clustered, we need to record that. */
- if (indxinfo->indisclustered)
- {
- appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER",
- fmtQualifiedDumpable(tbinfo));
- /* index name is not qualified in this syntax */
- appendPQExpBuffer(q, " ON %s;\n",
- fmtId(indxinfo->dobj.name));
- }
-
/* If the index defines identity, we need to record that. */
if (indxinfo->indisreplident)
{
@@ -18448,6 +18493,7 @@ addBoundaryDependencies(DumpableObject **dobjs, int numObjs,
break;
case DO_INDEX:
case DO_INDEX_ATTACH:
+ case DO_INDEX_CLUSTER_ON:
case DO_STATSEXT:
case DO_REFRESH_MATVIEW:
case DO_TRIGGER:
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 1290f9659b..57def4c009 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -54,6 +54,7 @@ typedef enum
DO_ATTRDEF,
DO_INDEX,
DO_INDEX_ATTACH,
+ DO_INDEX_CLUSTER_ON,
DO_STATSEXT,
DO_RULE,
DO_TRIGGER,
@@ -386,6 +387,13 @@ typedef struct _indxInfo
DumpId indexconstraint;
} IndxInfo;
+typedef struct _indexClusterInfo
+{
+ DumpableObject dobj;
+ TableInfo *indextable; /* link to table the index is for */
+ IndxInfo *index; /* link to index itself */
+} IndexClusterInfo;
+
typedef struct _indexAttachInfo
{
DumpableObject dobj;
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 46461fb6a1..dd5b233196 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -75,6 +75,7 @@ enum dbObjectTypePriorities
PRIO_CONSTRAINT,
PRIO_INDEX,
PRIO_INDEX_ATTACH,
+ PRIO_INDEX_CLUSTER_ON,
PRIO_STATSEXT,
PRIO_RULE,
PRIO_TRIGGER,
@@ -108,6 +109,7 @@ static const int dbObjectTypePriority[] =
PRIO_ATTRDEF, /* DO_ATTRDEF */
PRIO_INDEX, /* DO_INDEX */
PRIO_INDEX_ATTACH, /* DO_INDEX_ATTACH */
+ PRIO_INDEX_CLUSTER_ON, /* DO_INDEX_CLUSTER_ON */
PRIO_STATSEXT, /* DO_STATSEXT */
PRIO_RULE, /* DO_RULE */
PRIO_TRIGGER, /* DO_TRIGGER */
@@ -136,6 +138,7 @@ static const int dbObjectTypePriority[] =
PRIO_PUBLICATION, /* DO_PUBLICATION */
PRIO_PUBLICATION_REL, /* DO_PUBLICATION_REL */
PRIO_SUBSCRIPTION /* DO_SUBSCRIPTION */
+
};
StaticAssertDecl(lengthof(dbObjectTypePriority) == (DO_SUBSCRIPTION + 1),
@@ -1348,6 +1351,11 @@ describeDumpableObject(DumpableObject *obj, char *buf, int bufsize)
"INDEX ATTACH %s (ID %d)",
obj->name, obj->dumpId);
return;
+ case DO_INDEX_CLUSTER_ON:
+ snprintf(buf, bufsize,
+ "INDEX CLUSTER ON %s (ID %d)",
+ obj->name, obj->dumpId);
+ return;
case DO_STATSEXT:
snprintf(buf, bufsize,
"STATISTICS %s (ID %d OID %u)",
--
2.17.0
--AA9g+nFNFPYNJKiL
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0002-Implement-CLUSTER-of-partitioned-table.patch"
^ permalink raw reply [nested|flat] 8+ messages in thread
* Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2023-09-20 12:58 Amul Sul <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Amul Sul @ 2023-09-20 12:58 UTC (permalink / raw)
To: pgsql-hackers
Hi,
On the latest master head, I can see a $subject bug that seems to be related
commit #b0e96f311985:
Here is the table definition:
create table foo(i int, j int, CONSTRAINT pk PRIMARY KEY(i) DEFERRABLE);
And after restore from the dump, it shows a descriptor where column 'i' not
marked NOT NULL:
=# \d foo
Table "public.foo"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
i | integer | | |
j | integer | | |
Indexes:
"pk" PRIMARY KEY, btree (i) DEFERRABLE
The pg_attribute entry:
=# select attname, attnotnull from pg_attribute
where attrelid = 'foo'::regclass and attnum > 0;
attname | attnotnull
---------+------------
i | f
j | f
(2 rows)
--
Regards,
Amul Sul
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2023-09-20 14:59 Alvaro Herrera <[email protected]>
parent: Amul Sul <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Alvaro Herrera @ 2023-09-20 14:59 UTC (permalink / raw)
To: Amul Sul <[email protected]>; +Cc: pgsql-hackers
On 2023-Sep-20, Amul Sul wrote:
> On the latest master head, I can see a $subject bug that seems to be related
> commit #b0e96f311985:
>
> Here is the table definition:
> create table foo(i int, j int, CONSTRAINT pk PRIMARY KEY(i) DEFERRABLE);
Interesting, thanks for the report. Your attribution to that commit is
correct. The table is dumped like this:
CREATE TABLE public.foo (
i integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT,
j integer
);
ALTER TABLE ONLY public.foo
ADD CONSTRAINT pk PRIMARY KEY (i) DEFERRABLE;
ALTER TABLE ONLY public.foo DROP CONSTRAINT pgdump_throwaway_notnull_0;
so the problem here is that the deferrable PK is not considered a reason
to keep attnotnull set, so we produce a throwaway constraint that we
then drop. This is already bogus, but what is more bogus is the fact
that the backend accepts the DROP CONSTRAINT at all.
The pg_dump failing should be easy to fix, but fixing the backend to
error out sounds more critical. So, the reason for this behavior is
that RelationGetIndexList doesn't want to list an index that isn't
marked indimmediate as a primary key. I can easily hack around that by
doing
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 7234cb3da6..971d9c8738 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4794,7 +4794,6 @@ RelationGetIndexList(Relation relation)
* check them.
*/
if (!index->indisunique ||
- !index->indimmediate ||
!heap_attisnull(htup, Anum_pg_index_indpred, NULL))
continue;
@@ -4821,6 +4820,9 @@ RelationGetIndexList(Relation relation)
relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
pkeyIndex = index->indexrelid;
+ if (!index->indimmediate)
+ continue;
+
if (!index->indisvalid)
continue;
But of course this is not great, since it impacts unrelated bits of code
that are relying on relation->pkindex or RelationGetIndexAttrBitmap
having their current behavior with non-immediate index.
I think a real solution is to stop relying on RelationGetIndexAttrBitmap
in ATExecDropNotNull(). (And, again, pg_dump needs some patch as well
to avoid printing a throwaway NOT NULL constraint at this point.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2023-09-22 10:55 Amul Sul <[email protected]>
parent: Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Amul Sul @ 2023-09-22 10:55 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: pgsql-hackers
On Wed, Sep 20, 2023 at 8:29 PM Alvaro Herrera <[email protected]>
wrote:
> On 2023-Sep-20, Amul Sul wrote:
>
> > On the latest master head, I can see a $subject bug that seems to be
> related
> > commit #b0e96f311985:
> >
> > Here is the table definition:
> > create table foo(i int, j int, CONSTRAINT pk PRIMARY KEY(i) DEFERRABLE);
>
> Interesting, thanks for the report. Your attribution to that commit is
> correct. The table is dumped like this:
>
> CREATE TABLE public.foo (
> i integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT,
> j integer
> );
> ALTER TABLE ONLY public.foo
> ADD CONSTRAINT pk PRIMARY KEY (i) DEFERRABLE;
> ALTER TABLE ONLY public.foo DROP CONSTRAINT pgdump_throwaway_notnull_0;
>
> so the problem here is that the deferrable PK is not considered a reason
> to keep attnotnull set, so we produce a throwaway constraint that we
> then drop. This is already bogus, but what is more bogus is the fact
> that the backend accepts the DROP CONSTRAINT at all.
>
> The pg_dump failing should be easy to fix, but fixing the backend to
> error out sounds more critical. So, the reason for this behavior is
> that RelationGetIndexList doesn't want to list an index that isn't
> marked indimmediate as a primary key. I can easily hack around that by
> doing
>
> diff --git a/src/backend/utils/cache/relcache.c
> b/src/backend/utils/cache/relcache.c
> index 7234cb3da6..971d9c8738 100644
> --- a/src/backend/utils/cache/relcache.c
> +++ b/src/backend/utils/cache/relcache.c
> @@ -4794,7 +4794,6 @@ RelationGetIndexList(Relation relation)
> * check them.
> */
> if (!index->indisunique ||
> - !index->indimmediate ||
> !heap_attisnull(htup,
> Anum_pg_index_indpred, NULL))
> continue;
>
> @@ -4821,6 +4820,9 @@ RelationGetIndexList(Relation relation)
> relation->rd_rel->relkind ==
> RELKIND_PARTITIONED_TABLE))
> pkeyIndex = index->indexrelid;
>
> + if (!index->indimmediate)
> + continue;
> +
> if (!index->indisvalid)
> continue;
>
>
> But of course this is not great, since it impacts unrelated bits of code
> that are relying on relation->pkindex or RelationGetIndexAttrBitmap
> having their current behavior with non-immediate index.
>
True, but still wondering why would relation->rd_pkattr skipped for a
deferrable primary key, which seems to be a bit incorrect to me since it
sensing that relation doesn't have PK at all. Anyway, that is unrelated.
> I think a real solution is to stop relying on RelationGetIndexAttrBitmap
> in ATExecDropNotNull(). (And, again, pg_dump needs some patch as well
> to avoid printing a throwaway NOT NULL constraint at this point.)
>
I might not have understood this, but I think, if it is ok to skip
throwaway NOT
NULL for deferrable PK then that would be enough for the reported issue
to be fixed. I quickly tried with the attached patch which looks sufficient
to skip that, but, TBH, I haven't thought carefully about this change.
Regards,
Amul
Attachments:
[application/x-patch] trial_skip_throwaway_non_null.patch (622B, ../../CAAJ_b94vRvR8AymA08Dz9Cs_3rHq+dRfY_-tnuJU48jkEuojwg@mail.gmail.com/3-trial_skip_throwaway_non_null.patch)
download | inline diff:
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index f7b61766921..5a4e915dc62 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8543,7 +8543,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
appendPQExpBufferStr(q,
"LEFT JOIN pg_catalog.pg_constraint copk ON "
"(copk.conrelid = src.tbloid\n"
- " AND copk.contype = 'p' AND "
+ " AND copk.contype = 'p' AND copk.condeferrable = false AND "
"copk.conkey @> array[a.attnum])\n"
"WHERE a.attnum > 0::pg_catalog.int2\n"
"ORDER BY a.attrelid, a.attnum");
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-01-20 02:24 vignesh C <[email protected]>
parent: Amul Sul <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: vignesh C @ 2024-01-20 02:24 UTC (permalink / raw)
To: Amul Sul <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers
On Fri, 22 Sept 2023 at 18:45, Amul Sul <[email protected]> wrote:
>
>
>
> On Wed, Sep 20, 2023 at 8:29 PM Alvaro Herrera <[email protected]> wrote:
>>
>> On 2023-Sep-20, Amul Sul wrote:
>>
>> > On the latest master head, I can see a $subject bug that seems to be related
>> > commit #b0e96f311985:
>> >
>> > Here is the table definition:
>> > create table foo(i int, j int, CONSTRAINT pk PRIMARY KEY(i) DEFERRABLE);
>>
>> Interesting, thanks for the report. Your attribution to that commit is
>> correct. The table is dumped like this:
>>
>> CREATE TABLE public.foo (
>> i integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT,
>> j integer
>> );
>> ALTER TABLE ONLY public.foo
>> ADD CONSTRAINT pk PRIMARY KEY (i) DEFERRABLE;
>> ALTER TABLE ONLY public.foo DROP CONSTRAINT pgdump_throwaway_notnull_0;
>>
>> so the problem here is that the deferrable PK is not considered a reason
>> to keep attnotnull set, so we produce a throwaway constraint that we
>> then drop. This is already bogus, but what is more bogus is the fact
>> that the backend accepts the DROP CONSTRAINT at all.
>>
>> The pg_dump failing should be easy to fix, but fixing the backend to
>> error out sounds more critical. So, the reason for this behavior is
>> that RelationGetIndexList doesn't want to list an index that isn't
>> marked indimmediate as a primary key. I can easily hack around that by
>> doing
>>
>> diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
>> index 7234cb3da6..971d9c8738 100644
>> --- a/src/backend/utils/cache/relcache.c
>> +++ b/src/backend/utils/cache/relcache.c
>> @@ -4794,7 +4794,6 @@ RelationGetIndexList(Relation relation)
>> * check them.
>> */
>> if (!index->indisunique ||
>> - !index->indimmediate ||
>> !heap_attisnull(htup, Anum_pg_index_indpred, NULL))
>> continue;
>>
>> @@ -4821,6 +4820,9 @@ RelationGetIndexList(Relation relation)
>> relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
>> pkeyIndex = index->indexrelid;
>>
>> + if (!index->indimmediate)
>> + continue;
>> +
>> if (!index->indisvalid)
>> continue;
>>
>>
>> But of course this is not great, since it impacts unrelated bits of code
>> that are relying on relation->pkindex or RelationGetIndexAttrBitmap
>> having their current behavior with non-immediate index.
>
>
> True, but still wondering why would relation->rd_pkattr skipped for a
> deferrable primary key, which seems to be a bit incorrect to me since it
> sensing that relation doesn't have PK at all. Anyway, that is unrelated.
>
>>
>> I think a real solution is to stop relying on RelationGetIndexAttrBitmap
>> in ATExecDropNotNull(). (And, again, pg_dump needs some patch as well
>> to avoid printing a throwaway NOT NULL constraint at this point.)
>
>
> I might not have understood this, but I think, if it is ok to skip throwaway NOT
> NULL for deferrable PK then that would be enough for the reported issue
> to be fixed. I quickly tried with the attached patch which looks sufficient
> to skip that, but, TBH, I haven't thought carefully about this change.
I did not see any test addition for this, can we add it?
Regards,
Vignesh
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-01-23 12:11 Amul Sul <[email protected]>
parent: vignesh C <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Amul Sul @ 2024-01-23 12:11 UTC (permalink / raw)
To: vignesh C <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers
On Sat, Jan 20, 2024 at 7:55 AM vignesh C <[email protected]> wrote:
> On Fri, 22 Sept 2023 at 18:45, Amul Sul <[email protected]> wrote:
> >
> >
> >
> > On Wed, Sep 20, 2023 at 8:29 PM Alvaro Herrera <[email protected]>
> wrote:
> >>
> >> On 2023-Sep-20, Amul Sul wrote:
> >>
> >> > On the latest master head, I can see a $subject bug that seems to be
> related
> >> > commit #b0e96f311985:
> >> >
> >> > Here is the table definition:
> >> > create table foo(i int, j int, CONSTRAINT pk PRIMARY KEY(i)
> DEFERRABLE);
> >>
> >> Interesting, thanks for the report. Your attribution to that commit is
> >> correct. The table is dumped like this:
> >>
> >> CREATE TABLE public.foo (
> >> i integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT,
> >> j integer
> >> );
> >> ALTER TABLE ONLY public.foo
> >> ADD CONSTRAINT pk PRIMARY KEY (i) DEFERRABLE;
> >> ALTER TABLE ONLY public.foo DROP CONSTRAINT pgdump_throwaway_notnull_0;
> >>
> >> so the problem here is that the deferrable PK is not considered a reason
> >> to keep attnotnull set, so we produce a throwaway constraint that we
> >> then drop. This is already bogus, but what is more bogus is the fact
> >> that the backend accepts the DROP CONSTRAINT at all.
> >>
> >> The pg_dump failing should be easy to fix, but fixing the backend to
> >> error out sounds more critical. So, the reason for this behavior is
> >> that RelationGetIndexList doesn't want to list an index that isn't
> >> marked indimmediate as a primary key. I can easily hack around that by
> >> doing
> >>
> >> diff --git a/src/backend/utils/cache/relcache.c
> b/src/backend/utils/cache/relcache.c
> >> index 7234cb3da6..971d9c8738 100644
> >> --- a/src/backend/utils/cache/relcache.c
> >> +++ b/src/backend/utils/cache/relcache.c
> >> @@ -4794,7 +4794,6 @@ RelationGetIndexList(Relation relation)
> >> * check them.
> >> */
> >> if (!index->indisunique ||
> >> - !index->indimmediate ||
> >> !heap_attisnull(htup,
> Anum_pg_index_indpred, NULL))
> >> continue;
> >>
> >> @@ -4821,6 +4820,9 @@ RelationGetIndexList(Relation relation)
> >> relation->rd_rel->relkind ==
> RELKIND_PARTITIONED_TABLE))
> >> pkeyIndex = index->indexrelid;
> >>
> >> + if (!index->indimmediate)
> >> + continue;
> >> +
> >> if (!index->indisvalid)
> >> continue;
> >>
> >>
> >> But of course this is not great, since it impacts unrelated bits of code
> >> that are relying on relation->pkindex or RelationGetIndexAttrBitmap
> >> having their current behavior with non-immediate index.
> >
> >
> > True, but still wondering why would relation->rd_pkattr skipped for a
> > deferrable primary key, which seems to be a bit incorrect to me since it
> > sensing that relation doesn't have PK at all. Anyway, that is unrelated.
> >
> >>
> >> I think a real solution is to stop relying on RelationGetIndexAttrBitmap
> >> in ATExecDropNotNull(). (And, again, pg_dump needs some patch as well
> >> to avoid printing a throwaway NOT NULL constraint at this point.)
> >
> >
> > I might not have understood this, but I think, if it is ok to skip
> throwaway NOT
> > NULL for deferrable PK then that would be enough for the reported issue
> > to be fixed. I quickly tried with the attached patch which looks
> sufficient
> > to skip that, but, TBH, I haven't thought carefully about this change.
>
> I did not see any test addition for this, can we add it?
>
Ok, added it in the attached version.
This was an experimental patch, I was looking for the comment on the
proposed
approach -- whether we could simply skip the throwaway NOT NULL constraint
for
deferred PK constraint. Moreover, skip that for any PK constraint.
Regards,
Amul
Attachments:
[application/octet-stream] trial_skip_throwaway_non_null_v2.patch (1.4K, ../../CAAJ_b951Qm4uH7ak_MvGDRQ1nUarPvk3Nr2pfmqySS_EtDzBhA@mail.gmail.com/3-trial_skip_throwaway_non_null_v2.patch)
download | inline diff:
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index bc20a025ce..c30eb95ce4 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8740,7 +8740,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
appendPQExpBufferStr(q,
"LEFT JOIN pg_catalog.pg_constraint copk ON "
"(copk.conrelid = src.tbloid\n"
- " AND copk.contype = 'p' AND "
+ " AND copk.contype = 'p' AND copk.condeferrable = false AND "
"copk.conkey @> array[a.attnum])\n"
"WHERE a.attnum > 0::pg_catalog.int2\n"
"ORDER BY a.attrelid, a.attnum");
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 3912dbf481..43913e3476 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -3605,12 +3605,12 @@ my %tests = (
'CREATE TABLE test_table_generated' => {
create_order => 3,
create_sql => 'CREATE TABLE dump_test.test_table_generated (
- col1 int primary key,
+ col1 int primary key deferrable,
col2 int generated always as (col1 * 2) stored
);',
regexp => qr/^
\QCREATE TABLE dump_test.test_table_generated (\E\n
- \s+\Qcol1 integer CONSTRAINT \E[a-z0-9_]*\Q NOT NULL NO INHERIT,\E\n
+ \s+\Qcol1 integer,\E\n
\s+\Qcol2 integer GENERATED ALWAYS AS ((col1 * 2)) STORED\E\n
\);
/xms,
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-04 12:34 Aleksander Alekseev <[email protected]>
parent: Amul Sul <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Aleksander Alekseev @ 2024-03-04 12:34 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Amul Sul <[email protected]>; vignesh C <[email protected]>; Alvaro Herrera <[email protected]>
Hi hackers,
>> I did not see any test addition for this, can we add it?
>
>
> Ok, added it in the attached version.
>
> This was an experimental patch, I was looking for the comment on the proposed
> approach -- whether we could simply skip the throwaway NOT NULL constraint for
> deferred PK constraint. Moreover, skip that for any PK constraint.
I confirm that the patch fixes the bug. All the tests pass. Looks like
RfC to me.
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
@ 2024-03-04 13:50 Dean Rasheed <[email protected]>
parent: Aleksander Alekseev <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Dean Rasheed @ 2024-03-04 13:50 UTC (permalink / raw)
To: Aleksander Alekseev <[email protected]>; +Cc: pgsql-hackers; Amul Sul <[email protected]>; vignesh C <[email protected]>; Alvaro Herrera <[email protected]>
On Mon, 4 Mar 2024 at 12:34, Aleksander Alekseev
<[email protected]> wrote:
>
> > This was an experimental patch, I was looking for the comment on the proposed
> > approach -- whether we could simply skip the throwaway NOT NULL constraint for
> > deferred PK constraint. Moreover, skip that for any PK constraint.
>
> I confirm that the patch fixes the bug. All the tests pass. Looks like
> RfC to me.
>
I don't think that this is the right fix. ISTM that the real issue is
that dropping a NOT NULL constraint should not mark the column as
nullable if it is part of a PK, whether or not that PK is deferrable
-- a deferrable PK still marks a column as not nullable.
The reason pg_dump creates these throwaway NOT NULL constraints is to
avoid a table scan to check for NULLs when the PK is later created.
That rationale still applies to deferrable PKs, so we still want the
throwaway NOT NULL constraints in that case, otherwise we'd be hurting
performance of restore.
Regards,
Dean
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2024-03-04 13:50 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-26 20:37 [PATCH v7 1/7] pg_dump: make CLUSTER ON a separate dump object.. Justin Pryzby <[email protected]>
2023-09-20 12:58 Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s). Amul Sul <[email protected]>
2023-09-20 14:59 ` Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s). Alvaro Herrera <[email protected]>
2023-09-22 10:55 ` Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s). Amul Sul <[email protected]>
2024-01-20 02:24 ` Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s). vignesh C <[email protected]>
2024-01-23 12:11 ` Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s). Amul Sul <[email protected]>
2024-03-04 12:34 ` Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s). Aleksander Alekseev <[email protected]>
2024-03-04 13:50 ` Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s). Dean Rasheed <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox