2017-12-14 15:08 GMT+07:00 Michael Paquier <[email protected]>:
>
> On Thu, Dec 14, 2017 at 2:49 PM, Justin Pryzby <[email protected]> wrote:
> > On Thu, Dec 14, 2017 at 08:51:06AM +0700, Ali Akbar wrote:
> >> 2017-12-13 15:37 GMT+07:00 Amit Langote <[email protected]>:
> >> Patch for adding check in pg_upgrade. Going through git history, the check
> >> for inconsistency in NOT NULL constraint has ben there since a long time
> >> ago. In this patch the check will be applied for all old cluster version.
> >> I'm not sure in which version was the release of table inheritance.
> >
> > Here are some spelling and grammar fixes to that patch:
> >
> > but nullabe in its children: nullable
> > child column is not market: marked
> > with adding: by adding
> > and restart: and restarting
> > the problem columns: the problematic columns
> > 9.5, 9.6, 10: 9.5, 9.6, and 10
> > restore, that will cause error.: restore phase of pg_upgrade, that would
> > cause an error.
Thanks. Typo fixed.
>
> I agree that we should do better in pg_upgrade for HEAD and
> REL_10_STABLE as it is not cool to fail late in the upgrade process
> especially if you are using the --link mode.
>
> + * Currently pg_upgrade will fail if there are any inconsistencies in NOT
> NULL
> + * constraint inheritance: In Postgres version 9.5, 9.6, 10 we can
> have a column
> + * that is NOT NULL in parent, but nullabe in its children. But during
> schema
> + * restore, that will cause error.
> On top of the multiple typos here, there is no need to mention
> anything other than "PostgreSQL 9.6 and older versions did not add NOT
> NULL constraints when a primary key was added to to a parent, so check
> for inconsistent NOT NULL constraints in inheritance trees".
In my case, my database becomes like that because accidental ALTER
COLUMN x DROP NOT NULL, and no, not the Primary Key.
Something like this:
CREATE TABLE parent (id SERIAL PRIMARY KEY, name VARCHAR(52) NOT NULL);
CREATE TABLE child () INHERITS (parent);
ALTER TABLE child ALTER COLUMN name DROP NOT NULL;
And yes, in current version 10 (and HEAD), we can still do that.
Because both version currently accepts that inconsistency, ideally
pg_upgrade must be able to upgrade the cluster without problem. Hence
the comment: "Currently pg_upgrade will fail if there are any
inconsistencies in NOT NULL constraint inheritance".
> You seem to take a path similar to what is done with the jsonb check.
> Why not. I would find cleaner to tell also the user about using ALTER
> TABLE to add the proper constraints.
>
> Note that I have not checked or tested the query in details, but
> reading through the thing looks basically correct as it handles
> inheritance chains with WITH RECURSIVE.
Any suggestion to make it more readable? Maybe by separating column
query with schema & table name by using another CTE?
> @@ -99,6 +100,7 @@ check_and_dump_old_cluster(bool live_check)
> check_for_prepared_transactions(&old_cluster);
> check_for_reg_data_type_usage(&old_cluster);
> check_for_isn_and_int8_passing_mismatch(&old_cluster);
> + check_for_not_null_inheritance(&old_cluster);
> The check needs indeed to happen in check_and_dump_old_cluster(), but
> there is no point to check for it in servers with version 10 and
> upwards, hence you should make it conditional with GET_MAJOR_VERSION()
> <= 906.
No, currently it can happen again in version 10 (and above) because of
the problem above.
By the way, should i add this patch to the current commitfest?
Thanks,
Ali Akbar
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 1b9083597c..0bd2cd0ed1 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -26,6 +26,7 @@ static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
static void check_for_reg_data_type_usage(ClusterInfo *cluster);
static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
static void check_for_pg_role_prefix(ClusterInfo *cluster);
+static void check_for_not_null_inheritance(ClusterInfo *cluster);
static char *get_canonical_locale_name(int category, const char *locale);
@@ -99,6 +100,7 @@ check_and_dump_old_cluster(bool live_check)
check_for_prepared_transactions(&old_cluster);
check_for_reg_data_type_usage(&old_cluster);
check_for_isn_and_int8_passing_mismatch(&old_cluster);
+ check_for_not_null_inheritance(&old_cluster);
/*
* Pre-PG 10 allowed tables with 'unknown' type columns and non WAL logged
@@ -1096,6 +1098,105 @@ check_for_pg_role_prefix(ClusterInfo *cluster)
check_ok();
}
+/*
+ * check_for_not_null_inheritance()
+ *
+ * Currently pg_upgrade will fail if there are any inconsistencies in NOT NULL
+ * constraint inheritance: In PostgreSQL, we can have a column that is NOT NULL
+ * in parent, but nullable in its children. So check for inconsistent NOT NULL
+ * constraints in inheritance tree.
+ */
+static void
+check_for_not_null_inheritance(ClusterInfo *cluster)
+{
+ int dbnum;
+ FILE *script = NULL;
+ bool found = false;
+ char output_path[MAXPGPATH];
+
+ prep_status("Checking for NOT NULL inconsistencies in inheritance");
+
+ snprintf(output_path, sizeof(output_path), "not_null_inconsistent_columns.txt");
+
+ for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ PGresult *res;
+ bool db_used = false;
+ int ntups;
+ int rowno;
+ int i_nspname,
+ i_relname,
+ i_attname;
+ DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
+ PGconn *conn = connectToServer(cluster, active_db->db_name);
+
+ res = executeQueryOrDie(conn,
+ "WITH RECURSIVE parents AS ( "
+ " SELECT i.inhrelid, i.inhparent "
+ " FROM pg_catalog.pg_inherits i "
+ " UNION ALL "
+ " SELECT p.inhrelid, i.inhparent "
+ " FROM parents p "
+ " JOIN pg_catalog.pg_inherits i "
+ " ON i.inhrelid = p.inhparent "
+ ") "
+ "SELECT n.nspname, c.relname, ac.attname "
+ "FROM parents p, "
+ " pg_catalog.pg_attribute ac, "
+ " pg_catalog.pg_attribute ap, "
+ " pg_catalog.pg_class c, "
+ " pg_catalog.pg_namespace n "
+ "WHERE NOT ac.attnotnull AND "
+ " ac.attrelid = p.inhrelid AND "
+ " ap.attrelid = p.inhparent AND "
+ " ac.attname = ap.attname AND "
+ " ap.attnotnull AND "
+ " c.oid = ac.attrelid AND "
+ " c.relnamespace = n.oid");
+
+ ntups = PQntuples(res);
+ i_nspname = PQfnumber(res, "nspname");
+ i_relname = PQfnumber(res, "relname");
+ i_attname = PQfnumber(res, "attname");
+ for (rowno = 0; rowno < ntups; rowno++)
+ {
+ found = true;
+ if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %s\n", output_path,
+ strerror(errno));
+ if (!db_used)
+ {
+ fprintf(script, "Database: %s\n", active_db->db_name);
+ db_used = true;
+ }
+ fprintf(script, " %s.%s.%s\n",
+ PQgetvalue(res, rowno, i_nspname),
+ PQgetvalue(res, rowno, i_relname),
+ PQgetvalue(res, rowno, i_attname));
+ }
+
+ PQclear(res);
+
+ PQfinish(conn);
+ }
+
+ if (script)
+ fclose(script);
+
+ if (found)
+ {
+ pg_log(PG_REPORT, "fatal\n");
+ pg_fatal("Your installation contains has inconsistencies in NOT NULL\n"
+ "constraint inheritance: child column is not marked NOT NULL\n"
+ "while parent column has NOT NULL constraint. You can fix the\n"
+ "inconsistency by adding NOT NULL constraint and restarting the\n"
+ "upgrade.\n"
+ "A list of the problematic columns is in the file:\n"
+ " %s\n\n", output_path);
+ }
+ else
+ check_ok();
+}
/*
* get_canonical_locale_name
