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

From: Amul Sul <[email-masked]>
Date: Wed, 20 Sep 2023 18:28:36 +0530

Hi,

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);

And after restore from the dump, it shows a descriptor where column 'i' not
marked NOT NULL:

=# \d foo
                Table "public.foo"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 i      | integer |           |          |
 j      | integer |           |          |
Indexes:
    "pk" PRIMARY KEY, btree (i) DEFERRABLE

The pg_attribute entry:

=# select attname, attnotnull from pg_attribute
where attrelid  = 'foo'::regclass and attnum > 0;

 attname | attnotnull
---------+------------
 i       | f
 j       | f
(2 rows)

-- 
Regards,
Amul Sul
EDB: http://www.enterprisedb.com

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 20 Sep 2023 16:59:49 +0200

> 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.

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.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

---

From: Amul Sul <[email-masked]>
Date: Fri, 22 Sep 2023 16:25:21 +0530

> 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.

> 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

---

From: vignesh C <[email-masked]>
Date: Sat, 20 Jan 2024 07:54:52 +0530

On Fri, 22 Sept 2023 at 18:45, Amul Sul <[email-masked]> wrote:
>
>
>
> On Wed, Sep 20, 2023 at 8:29 PM Alvaro Herrera <[email-masked]> wrote:
>>
> I might not have understood this, but I think, if it is ok to skip throwaway NOT

I did not see any test addition for this, can we add it?

Regards,
Vignesh

---

From: Amul Sul <[email-masked]>
Date: Tue, 23 Jan 2024 17:41:26 +0530

> 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

---

From: Aleksander Alekseev <[email-masked]>
Date: Mon, 4 Mar 2024 15:34:20 +0300

Hi hackers,

> 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

I confirm that the patch fixes the bug. All the tests pass. Looks like
RfC to me.

-- 
Best regards,
Aleksander Alekseev

---

From: Dean Rasheed <[email-masked]>
Date: Mon, 4 Mar 2024 13:50:21 +0000

> I confirm that the patch fixes the bug. All the tests pass. Looks like

I don't think that this is the right fix. ISTM that the real issue is
that dropping a NOT NULL constraint should not mark the column as
nullable if it is part of a PK, whether or not that PK is deferrable
-- a deferrable PK still marks a  column as not nullable.

The reason pg_dump creates these throwaway NOT NULL constraints is to
avoid a table scan to check for NULLs when the PK is later created.
That rationale still applies to deferrable PKs, so we still want the
throwaway NOT NULL constraints in that case, otherwise we'd be hurting
performance of restore.

Regards,
Dean