On Wed, Sep 20, 2023 at 8:29 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> 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
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");
