PG 12+ allows reusing a CHECK constraint when setting NOT NULL on existing column

[james-johnston-thumbtack]

The release notes for PG 12.0 note the following: https://www.postgresql.org/docs/release/12.0/

Allow ALTER TABLE ... SET NOT NULL to avoid unnecessary table scans (Sergei Kornilov)

This can be optimized when the table's column constraints can be recognized as disallowing nulls.

And the ALTER TABLE documentation now states:

SET NOT NULL may only be applied to a column provided none of the records in the table contain a NULL value for the column. Ordinarily this is checked during the ALTER TABLE by scanning the entire table; however, if a valid CHECK constraint is found which proves no NULL can exist, then the table scan is skipped.

I tested this on one of our large tables on a PG 13 installation, and it does seem to work as advertised. The VALIDATE CONSTRAINT step took a few minutes as expected. Then running ALTER TABLE ALTER COLUMN SET NOT NULL completed instantaneously (hardly any perceptible delay).

The documentation at https://squawkhq.com/docs/adding-not-nullable-field/#setting-an-existing-column-as-non-nullable could be updated for the existing column case as follows:

1. Create CHECK constraint that is NOT VALID, as already listed in the example.
2. Validate the CHECK constraint, as already listed in the example.
3. New step: set column NOT NULL: ALTER TABLE "recipe" ALTER COLUMN "view_count" SET NOT NULL;
4. New step: drop CHECK constraint: ALTER TABLE "recipe" DROP CONSTRAINT view_count_not_null;

The end state is as if you had never used a CHECK constraint at all, and had always used SET NOT NULL.

Of course, the lint rule will need to be updated since it does check for this. And the SQL is hard for linting because there is no way to tell PG to use an existing constraint. For example, ALTER TABLE ADD UNIQUE USING INDEX abc; provides no question that abc index will be used, and thus the linter can allow this. But there is no similar ALTER TABLE recipe ALTER COLUMN SET NOT NULL USING CONSTRAINT view_count_not_null; command. So without prior knowledge of whether that CHECK constraint exists, you can't be sure whether the operation is safe or not.

Some ideas:

• SELECT from a function (that would need to be created by the user) which raises an exception if a compatible constraint can't be found. The linter can assert that this function is called immediately prior to SET NOT NULL.
• If the user has tested and timed the migration on a copy of the production data set, and the migration that runs SET NOT NULL was tested to be short (potentially declared via a code comment), then the linter could allow it.

At the very least though, I think the documentation should note the new PG12+ capabilities.

[sbdchd]

Thanks for the thorough write up!

So the advantage with the outlined above approach vs the one in the docs is that you end up with a table that has set not null set for a field rather than having to use a check constraint? Which would probably make things like introspection easier

re: linting for this, I think your first idea would work, here's the existing test cases:

squawk/linter/src/rules/adding_not_null_field.rs

Lines 71 to 154 in def03ea

	mod test_rules {
	use std::str::FromStr;
	use crate::{
	check_sql_with_rule,
	versions::Version,
	violations::{RuleViolation, RuleViolationKind},
	ViolationMessage,
	};
	fn lint_sql(sql: &str, pg_version: Option<Version>) -> Vec<RuleViolation> {
	check_sql_with_rule(
	sql,
	&RuleViolationKind::AddingNotNullableField,
	pg_version,
	false,
	)
	.unwrap()
	}
	use insta::assert_debug_snapshot;
	#[test]
	fn set_null() {
	let sql = r#"
	ALTER TABLE "core_recipe" ALTER COLUMN "foo" SET NOT NULL;
	"#;
	let res = lint_sql(sql, None);
	assert_eq!(res.len(), 1);
	assert_eq!(res[0].kind, RuleViolationKind::AddingNotNullableField);
	assert_eq!(
	res[0].messages,
	vec![
	ViolationMessage::Note(
	"Setting a column NOT NULL blocks reads while the table is scanned.".into()
	),
	ViolationMessage::Help("Use a check constraint instead.".into())
	]
	);
	}
	#[test]
	fn test_adding_field_that_is_not_nullable() {
	let bad_sql = r#"
	BEGIN;
	--
	-- Add field foo to recipe
	--
	ALTER TABLE "core_recipe" ADD COLUMN "foo" integer DEFAULT 10 NOT NULL;
	ALTER TABLE "core_recipe" ALTER COLUMN "foo" DROP DEFAULT;
	COMMIT;
	"#;
	assert_debug_snapshot!(lint_sql(bad_sql, None));
	let bad_sql = r#"
	-- not sure how this would ever work, but might as well test it
	ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL;
	"#;
	assert_debug_snapshot!(lint_sql(bad_sql, None));
	}
	#[test]
	fn test_adding_field_that_is_not_nullable_in_version_11() {
	let ok_sql = r#"
	BEGIN;
	--
	-- Add field foo to recipe
	--
	ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL;
	COMMIT;
	"#;
	assert_debug_snapshot!(lint_sql(ok_sql, Some(Version::from_str("11.0.0").unwrap()),));
	}
	#[test]
	fn allow_not_null_field_with_default() {
	let ok_sql = r#"
	ALTER TABLE "foo_tbl" ADD COLUMN IF NOT EXISTS "bar_col" TEXT DEFAULT 'buzz' NOT NULL;
	"#;
	assert_eq!(lint_sql(ok_sql, None), vec![]);
	}

code comment approach would benefit from #149 (which I see you've already posted an idea for a fix for!)

[james-johnston-thumbtack]

So the advantage with the outlined above approach vs the one in the docs is that you end up with a table that has set not null set for a field rather than having to use a check constraint? Which would probably make things like introspection easier

Yeah, that's exactly right. I've run into multiple situations where a code generator tool does not know about these CHECK constraints. Here is one example of a 3rd-party tool that doesn't know about CHECK constraints: https://sqlc.dev/

TL;DR the sqlc tool will take a file containing schema DDL statements (e.g. we get ours from running pg_dump), another file containing queries for the application, and generate type-safe Go code from it. For example, suppose we give it schema file:

CREATE TABLE todo (
    todo_pk bigint primary key,
    todo_nullable bigint,
    todo_not_null bigint NOT NULL,
    todo_check bigint CHECK (todo_check IS NOT NULL)
)

It would then generate model code like this from that schema:

type Todo struct {
	TodoPk       int64
	TodoNullable sql.NullInt64
	TodoNotNull  int64
	TodoCheck    sql.NullInt64      // <------ did not notice the constraint
}

because it only knows about the NOT NULL attribute of a column, and doesn't look at any constraints: https://github.com/kyleconroy/sqlc/blob/81b15a3b4e2275da367bee1f1b9990d1f763cdea/internal/sql/catalog/table.go#L162-L198

We also have some code generator tools developed in-house as well, and again it's been an issue where they didn't account for CHECK constraints that enforce NOT NULL. I think a lot of people probably don't think to look for this sort of thing when doing introspection.

Maybe it made sense in the past to try to update this various tooling to try to parse out NOT NULL-like CHECK constraints, but at this point now that PG 12+ can reuse those CHECK constraints itself to make a column NOT NULL, IMHO the best practice should be to do the 4 step migration procedure I outlined in the original message, rather than add parsing code to other tools to try to parse the meaning of a CHECK constraint.

[sbdchd]

if I understand correctly, the goal is to make:

ALTER TABLE "recipe" ADD CONSTRAINT view_count_not_null
    CHECK ("view_count" IS NOT NULL) NOT VALID;

ALTER TABLE "recipe" VALIDATE CONSTRAINT view_count_not_null;

ALTER TABLE "recipe" ALTER COLUMN "view_count" SET NOT NULL;

ALTER TABLE "recipe" DROP CONSTRAINT view_count_not_null;

error?

[james-johnston-thumbtack]

if I understand correctly, the goal is to make:

<snip>

error?

The example you cited is exactly how one should be doing it. So it should not be an error to do it that way.

(One caveats with your example though: the VALIDATE CONSTRAINT needs to be done in a separate transaction / migration from the ADD CONSTRAINT statement. Otherwise the validation will inadvertently be holding an AccessExclusiveLock from the first ALTER TABLE command.)

However, just doing a simple

ALTER TABLE "recipe" ALTER COLUMN "view_count" SET NOT NULL;

without the initial constraint creation should still be an error, for rationales that are well known: it'll hold that AccessExclusiveLock while it checks all the rows.

[sbdchd]

Docs have been updated via #412!