Feedback-ID: ia2694551:Fastmail
Date: Wed, 20 Sep 2023 16:59:49 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amul Sul <sulamul@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY
 column(s).
Message-ID: <202309201459.jhuhi2pns3re@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAAJ_b94QonkgsbDXofakHDnORQNgafd1y3Oa5QXfpQNJyXyQ7A@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/202309201459.jhuhi2pns3re%40alvherre.pgsql>
Precedence: bulk

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.

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/