On Fri, 22 Sept 2023 at 18:45, Amul Sul <sula...@gmail.com> wrote: > > > > 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.
I did not see any test addition for this, can we add it? Regards, Vignesh
