Feedback-ID: ia2694551:Fastmail
Date: Tue, 24 Sep 2024 12:51:54 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: jian he <jian.universality@gmail.com>
Cc: Tender Wang <tndrwang@gmail.com>,
	Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: not null constraints, again
Message-ID: <202409241051.xhdeeqazf63f@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CACJufxGgO8QDt4mwT7PksskpFK-sFbJdNvqBcD4+KaQGu2OTZw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202409241051.xhdeeqazf63f%40alvherre.pgsql>
Precedence: bulk

On 2024-Sep-24, jian he wrote:

> static void
> set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, bool recurse,
>                LOCKMODE lockmode)
> {

> What do you think of the above refactor?
> (I intentionally deleted empty new line)

Looks nicer ... but you know what?  After spending some more time with
it, I realized that one caller is dead code (in
AttachPartitionEnsureIndexes) and another caller doesn't need to ask for
recursion, because it recurses itself (in ATAddCheckNNConstraint).  So
that leaves us with a grand total of zero callers that need the
recursion here ... which means we can simplify it to the case that it
only examines a single relation and never recurses.

So I've stripped it down to its bare minimum:

/*
 * Helper to set pg_attribute.attnotnull if it isn't set, and to tell phase 3
 * to verify it.
 *
 * When called to alter an existing table, 'wqueue' must be given so that we
 * can queue a check that existing tuples pass the constraint.  When called
 * from table creation, 'wqueue' should be passed as NULL.
 */
static void
set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum,
			   LOCKMODE lockmode)
{
	Oid			reloid = RelationGetRelid(rel);
	HeapTuple	tuple;
	Form_pg_attribute attForm;

	CheckAlterTableIsSafe(rel);

	tuple = SearchSysCacheCopyAttNum(reloid, attnum);
	if (!HeapTupleIsValid(tuple))
		elog(ERROR, "cache lookup failed for attribute %d of relation %u",
			 attnum, reloid);
	attForm = (Form_pg_attribute) GETSTRUCT(tuple);
	if (!attForm->attnotnull)
	{
		Relation	attr_rel;

		attr_rel = table_open(AttributeRelationId, RowExclusiveLock);

		attForm->attnotnull = true;
		CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);

		if (wqueue && !NotNullImpliedByRelConstraints(rel, attForm))
		{
			AlteredTableInfo *tab;

			tab = ATGetQueueEntry(wqueue, rel);
			tab->verify_new_notnull = true;
		}

		CommandCounterIncrement();

		table_close(attr_rel, RowExclusiveLock);
	}

	heap_freetuple(tuple);
}

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