MIME-Version: 1.0
References: 
 <CAAJ_b94QonkgsbDXofakHDnORQNgafd1y3Oa5QXfpQNJyXyQ7A@mail.gmail.com>
 <202309201459.jhuhi2pns3re@alvherre.pgsql>
 <CAAJ_b94vRvR8AymA08Dz9Cs_3rHq+dRfY_-tnuJU48jkEuojwg@mail.gmail.com>
In-Reply-To: 
 <CAAJ_b94vRvR8AymA08Dz9Cs_3rHq+dRfY_-tnuJU48jkEuojwg@mail.gmail.com>
From: vignesh C <vignesh21@gmail.com>
Date: Sat, 20 Jan 2024 07:54:52 +0530
Message-ID: 
 <CALDaNm0ELv1v+DUw3iOfbOpCHqW49kFmaU2QN3APFeFp_ge2dg@mail.gmail.com>
Subject: Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY
 column(s).
To: Amul Sul <sulamul@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CALDaNm0ELv1v%2BDUw3iOfbOpCHqW49kFmaU2QN3APFeFp_ge2dg%40mail.gmail.com>
Precedence: bulk

On Fri, 22 Sept 2023 at 18:45, Amul Sul <sulamul@gmail.com> wrote:
>
>
>
> On Wed, Sep 20, 2023 at 8:29=E2=80=AFPM Alvaro Herrera <alvherre@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 r=
elated
>> > 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/ut=
ils/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_indp=
red, NULL))
>>                                 continue;
>>
>>         @@ -4821,6 +4820,9 @@ RelationGetIndexList(Relation relation)
>>                                  relation->rd_rel->relkind =3D=3D RELKIN=
D_PARTITIONED_TABLE))
>>                                 pkeyIndex =3D 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 throwa=
way 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 suffici=
ent
> 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