On Sat, Jan 20, 2024 at 7:55 AM vignesh C <vignes...@gmail.com> wrote:
> 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? > 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
trial_skip_throwaway_non_null_v2.patch
Description: Binary data
