Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).

On Fri, 22 Sept 2023 at 18:45, Amul Sul <sulamul@gmail.com> wrote:
>
>
>
> On Wed, Sep 20, 2023 at 8:29 PM 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 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