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

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.

Regards,
Amul