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

On Wed, Sep 20, 2023 at 8:29 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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