Re: [HACKERS] Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
Excerpts from Noah Misch's message of jue ene 26 12:00:49 -0300 2012: On Wed, Jan 25, 2012 at 10:39:56PM -0500, Noah Misch wrote: In any event, the patch needed a rebase, so I've attached it rebased and with that comment edited to reference ri_GenerateQualCollation(), that being the most-relevant source for the assumption in question. Commit 9d35116611e6a1fc10f2298944fbf0e4e1a826be invalidated the test case hunks again. We'll need to either remove the test cases, as Robert chose to do for that other patch, or bolster them per http://archives.postgresql.org/message-id/20120126115536.GD15670%40tornado.leadboat.com Committed, removing the tests. I also chose to update catversion, even though I can't figure out how to make a Constraint node be stored anywhere. Maybe it's not even possible, but then I thought maybe I'm just lacking imagination. Thanks! -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
On Wed, Jan 25, 2012 at 10:39:56PM -0500, Noah Misch wrote: In any event, the patch needed a rebase, so I've attached it rebased and with that comment edited to reference ri_GenerateQualCollation(), that being the most-relevant source for the assumption in question. Commit 9d35116611e6a1fc10f2298944fbf0e4e1a826be invalidated the test case hunks again. We'll need to either remove the test cases, as Robert chose to do for that other patch, or bolster them per http://archives.postgresql.org/message-id/20120126115536.GD15670%40tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
Excerpts from Noah Misch's message of dom ene 22 02:05:31 -0300 2012: Thanks. I've attached a new version fixing this problem. Looks good to me. Can you please clarify this bit? * Since we elsewhere require that all collations share the same * notion of equality, don't compare collation here. Since I'm not familiar with this code, it's difficult for me to figure out where this elsewhere is, and what does same notion of equality mean. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
On Wed, Jan 25, 2012 at 11:17:27AM -0300, Alvaro Herrera wrote: Excerpts from Noah Misch's message of dom ene 22 02:05:31 -0300 2012: Thanks. I've attached a new version fixing this problem. Looks good to me. Can you please clarify this bit? * Since we elsewhere require that all collations share the same * notion of equality, don't compare collation here. Since I'm not familiar with this code, it's difficult for me to figure out where this elsewhere is, and what does same notion of equality mean. Good questions. See the comment in front of ri_GenerateQualCollation(), the comment in process_ordered_aggregate_single() at nodeAgg.c:605, the larger comment in add_sort_column(), and the two XXX comments in relation_has_unique_index_for(). We should probably document that assumption in xindex.sgml to keep type implementors apprised. Same notion of equality just means that a COLLATE x = b COLLATE y has the same value for every x, y. In any event, the patch needed a rebase, so I've attached it rebased and with that comment edited to reference ri_GenerateQualCollation(), that being the most-relevant source for the assumption in question. Thanks for reviewing, nm diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index cb8ac67..ba20950 100644 *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *** *** 276,281 static Oid transformFkeyCheckAttrs(Relation pkrel, --- 276,282 int numattrs, int16 *attnums, Oid *opclasses); static void checkFkeyPermissions(Relation rel, int16 *attnums, int natts); + static CoercionPathType findFkeyCast(Oid targetTypeId, Oid sourceTypeId, Oid *funcid); static void validateCheckConstraint(Relation rel, HeapTuple constrtup); static void validateForeignKeyConstraint(char *conname, Relation rel, Relation pkrel, *** *** 357,362 static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMOD --- 358,364 static void ATPostAlterTypeParse(Oid oldId, char *cmd, List **wqueue, LOCKMODE lockmode, bool rewrite); static void TryReuseIndex(Oid oldId, IndexStmt *stmt); + static void TryReuseForeignKey(Oid oldId, Constraint *con); static void change_owner_fix_column_acls(Oid relationOid, Oid oldOwnerId, Oid newOwnerId); static void change_owner_recurse_to_sequences(Oid relationOid, *** *** 5591,5596 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5593,5600 numpks; Oid indexOid; Oid constrOid; + boolold_check_ok; + ListCell *old_pfeqop_item = list_head(fkconstraint-old_conpfeqop); /* * Grab an exclusive lock on the pk table, so that someone doesn't delete *** *** 5707,5712 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5711,5723 (errcode(ERRCODE_INVALID_FOREIGN_KEY), errmsg(number of referencing and referenced columns for foreign key disagree))); + /* +* On the strength of a previous constraint, we might avoid scanning +* tables to validate this one. See below. +*/ + old_check_ok = (fkconstraint-old_conpfeqop != NIL); + Assert(!old_check_ok || numfks == list_length(fkconstraint-old_conpfeqop)); + for (i = 0; i numpks; i++) { Oid pktype = pktypoid[i]; *** *** 5721,5726 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5732,5738 Oid ppeqop; Oid ffeqop; int16 eqstrategy; + Oid pfeqop_right; /* We need several fields out of the pg_opclass entry */ cla_ht = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclasses[i])); *** *** 5763,5772 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, pfeqop = get_opfamily_member(opfamily, opcintype, fktyped, eqstrategy); if (OidIsValid(pfeqop)) ffeqop = get_opfamily_member(opfamily, fktyped, fktyped, eqstrategy); else ! ffeqop = InvalidOid;/* keep compiler quiet */ if (!(OidIsValid(pfeqop) OidIsValid(ffeqop))) { --- 5775,5791
Re: [HACKERS] Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
On Wed, Jan 4, 2012 at 12:35 PM, Noah Misch n...@leadboat.com wrote: I neglected to commit after revising the text of a few comments; use this version instead. Thanks. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers This is failing make check for me. *** /tmp/foo/src/test/regress/expected/alter_table.out Sat Jan 21 19:51:46 2012 --- /tmp/foo/src/test/regress/results/alter_table.out Sat Jan 21 19:54:18 2012 *** *** 1662,1667 --- 1662,1668 NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index norewrite1_parent_pkey for table norewrite1_parent DEBUG: building index norewrite1_parent_pkey on table norewrite1_parent CREATE TABLE norewrite1_child(c text REFERENCES norewrite1_parent); + DEBUG: building index pg_toast_41491_index on table pg_toast_41491 ALTER TABLE norewrite1_child ALTER c TYPE varchar; -- recheck FK DEBUG: validating foreign key constraint norewrite1_child_c_fkey CREATE DOMAIN other_int AS int; Settings: name | current_setting -+ version | PostgreSQL 9.2devel on x86_64-unknown-linux-gnu, compiled by gcc (GCC) 4.1.2 20070115 (SUSE Linux), 64-bit lc_collate | C lc_ctype| C max_connections | 100 max_stack_depth | 2MB server_encoding | SQL_ASCII shared_buffers | 32MB TimeZone| US/Pacific wal_buffers | 1MB Cheers, Jeff -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
On Sat, Jan 21, 2012 at 08:04:20PM -0800, Jeff Janes wrote: This is failing make check for me. *** /tmp/foo/src/test/regress/expected/alter_table.out Sat Jan 21 19:51:46 2012 --- /tmp/foo/src/test/regress/results/alter_table.out Sat Jan 21 19:54:18 2012 *** *** 1662,1667 --- 1662,1668 NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index norewrite1_parent_pkey for table norewrite1_parent DEBUG: building index norewrite1_parent_pkey on table norewrite1_parent CREATE TABLE norewrite1_child(c text REFERENCES norewrite1_parent); + DEBUG: building index pg_toast_41491_index on table pg_toast_41491 ALTER TABLE norewrite1_child ALTER c TYPE varchar; -- recheck FK DEBUG: validating foreign key constraint norewrite1_child_c_fkey CREATE DOMAIN other_int AS int; Thanks. I've attached a new version fixing this problem. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index cc210f0..bb2cf62 100644 *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *** *** 276,281 static Oid transformFkeyCheckAttrs(Relation pkrel, --- 276,282 int numattrs, int16 *attnums, Oid *opclasses); static void checkFkeyPermissions(Relation rel, int16 *attnums, int natts); + static CoercionPathType findFkeyCast(Oid targetTypeId, Oid sourceTypeId, Oid *funcid); static void validateCheckConstraint(Relation rel, HeapTuple constrtup); static void validateForeignKeyConstraint(char *conname, Relation rel, Relation pkrel, *** *** 357,362 static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMOD --- 358,364 static void ATPostAlterTypeParse(Oid oldId, char *cmd, List **wqueue, LOCKMODE lockmode, bool rewrite); static void TryReuseIndex(Oid oldId, IndexStmt *stmt); + static void TryReuseForeignKey(Oid oldId, Constraint *con); static void change_owner_fix_column_acls(Oid relationOid, Oid oldOwnerId, Oid newOwnerId); static void change_owner_recurse_to_sequences(Oid relationOid, *** *** 5574,5579 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5576,5583 numpks; Oid indexOid; Oid constrOid; + boolold_check_ok; + ListCell *old_pfeqop_item = list_head(fkconstraint-old_conpfeqop); /* * Grab an exclusive lock on the pk table, so that someone doesn't delete *** *** 5690,5695 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5694,5706 (errcode(ERRCODE_INVALID_FOREIGN_KEY), errmsg(number of referencing and referenced columns for foreign key disagree))); + /* +* On the strength of a previous constraint, we might avoid scanning +* tables to validate this one. See below. +*/ + old_check_ok = (fkconstraint-old_conpfeqop != NIL); + Assert(!old_check_ok || numfks == list_length(fkconstraint-old_conpfeqop)); + for (i = 0; i numpks; i++) { Oid pktype = pktypoid[i]; *** *** 5704,5709 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5715,5721 Oid ppeqop; Oid ffeqop; int16 eqstrategy; + Oid pfeqop_right; /* We need several fields out of the pg_opclass entry */ cla_ht = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclasses[i])); *** *** 5746,5755 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, pfeqop = get_opfamily_member(opfamily, opcintype, fktyped, eqstrategy); if (OidIsValid(pfeqop)) ffeqop = get_opfamily_member(opfamily, fktyped, fktyped, eqstrategy); else ! ffeqop = InvalidOid;/* keep compiler quiet */ if (!(OidIsValid(pfeqop) OidIsValid(ffeqop))) { --- 5758,5774 pfeqop = get_opfamily_member(opfamily, opcintype, fktyped, eqstrategy); if (OidIsValid(pfeqop)) + { + pfeqop_right = fktyped; ffeqop = get_opfamily_member(opfamily, fktyped, fktyped,
Re: [HACKERS] Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
On Sat, Jan 21, 2012 at 9:05 PM, Noah Misch n...@leadboat.com wrote: On Sat, Jan 21, 2012 at 08:04:20PM -0800, Jeff Janes wrote: This is failing make check for me. *** /tmp/foo/src/test/regress/expected/alter_table.out Sat Jan 21 19:51:46 2012 --- /tmp/foo/src/test/regress/results/alter_table.out Sat Jan 21 19:54:18 2012 *** *** 1662,1667 --- 1662,1668 NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index norewrite1_parent_pkey for table norewrite1_parent DEBUG: building index norewrite1_parent_pkey on table norewrite1_parent CREATE TABLE norewrite1_child(c text REFERENCES norewrite1_parent); + DEBUG: building index pg_toast_41491_index on table pg_toast_41491 ALTER TABLE norewrite1_child ALTER c TYPE varchar; -- recheck FK DEBUG: validating foreign key constraint norewrite1_child_c_fkey CREATE DOMAIN other_int AS int; Thanks. I've attached a new version fixing this problem. Thanks, I've verified that it now passes make check. Looking at the code now, I don't think I'll be able to do a meaningful review in a reasonable time. I'm not familiar with that part or the code, and it is too complex for me to learn right now. Cheers, Jeff -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
Git master can already avoid rewriting the table for column type changes like varchar(10) - varchar(20). However, if the column in question is on either side of a FK relationship, we always revalidate the foreign key. Concretely, I wanted these no-rewrite type changes to also assume FK validity: - Any typmod-only change - text - varchar - domain - base type To achieve that, this patch skips the revalidation when two conditions hold: (a) Old and new pg_constraint.conpfeqop match exactly. This is actually stronger than needed; we could loosen things by way of operator families. However, no core type would benefit, and my head exploded when I tried to define the more-generous test correctly. (b) The functions, if any, implementing a cast from the foreign type to the primary opcintype are the same. For this purpose, we can consider a binary coercion equivalent to an exact type match. When the opcintype is polymorphic, require that the old and new foreign types match exactly. (Since ri_triggers.c does use the executor, the stronger check for polymorphic types is no mere future-proofing. However, no core type exercises its necessity.) These follow from the rules used to decide when to rebuild an index. I further justify them in source comments. To implement this, I have ATPostAlterTypeParse() stash the content of the old pg_constraint.conpfeqop in the Constraint node. ATAddForeignKeyConstraint() notices that and evaluates the above rules. If they both pass, it omits the validation step just as though skip_validation had been given. Thanks, nm diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3b52415..0cde503 100644 *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *** *** 276,281 static Oid transformFkeyCheckAttrs(Relation pkrel, --- 276,282 int numattrs, int16 *attnums, Oid *opclasses); static void checkFkeyPermissions(Relation rel, int16 *attnums, int natts); + static CoercionPathType findFkeyCast(Oid targetTypeId, Oid sourceTypeId, Oid *funcid); static void validateCheckConstraint(Relation rel, HeapTuple constrtup); static void validateForeignKeyConstraint(char *conname, Relation rel, Relation pkrel, *** *** 357,362 static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMOD --- 358,364 static void ATPostAlterTypeParse(Oid oldId, char *cmd, List **wqueue, LOCKMODE lockmode, bool rewrite); static void TryReuseIndex(Oid oldId, IndexStmt *stmt); + static void TryReuseForeignKey(Oid oldId, Constraint *con); static void change_owner_fix_column_acls(Oid relationOid, Oid oldOwnerId, Oid newOwnerId); static void change_owner_recurse_to_sequences(Oid relationOid, *** *** 5696,5701 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5698,5705 numpks; Oid indexOid; Oid constrOid; + boolold_check_ok; + ListCell *old_pfeqop_item = list_head(fkconstraint-old_conpfeqop); /* * Grab an exclusive lock on the pk table, so that someone doesn't delete *** *** 5812,5817 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5816,5828 (errcode(ERRCODE_INVALID_FOREIGN_KEY), errmsg(number of referencing and referenced columns for foreign key disagree))); + /* +* On the strength of a previous constraint, we might avoid scanning +* tables to validate this one. See below. +*/ + old_check_ok = (fkconstraint-old_conpfeqop != NIL); + Assert(!old_check_ok || numfks == list_length(fkconstraint-old_conpfeqop)); + for (i = 0; i numpks; i++) { Oid pktype = pktypoid[i]; *** *** 5826,5831 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5837,5843 Oid ppeqop; Oid ffeqop; int16 eqstrategy; + Oid pfeqop_right; /* We need several fields out of the pg_opclass entry */ cla_ht = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclasses[i])); *** *** 5868,5877 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, pfeqop = get_opfamily_member(opfamily, opcintype, fktyped, eqstrategy); if (OidIsValid(pfeqop)) ffeqop =
Re: [HACKERS] Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE
I neglected to commit after revising the text of a few comments; use this version instead. Thanks. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3b52415..9eba8e8 100644 *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *** *** 276,281 static Oid transformFkeyCheckAttrs(Relation pkrel, --- 276,282 int numattrs, int16 *attnums, Oid *opclasses); static void checkFkeyPermissions(Relation rel, int16 *attnums, int natts); + static CoercionPathType findFkeyCast(Oid targetTypeId, Oid sourceTypeId, Oid *funcid); static void validateCheckConstraint(Relation rel, HeapTuple constrtup); static void validateForeignKeyConstraint(char *conname, Relation rel, Relation pkrel, *** *** 357,362 static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMOD --- 358,364 static void ATPostAlterTypeParse(Oid oldId, char *cmd, List **wqueue, LOCKMODE lockmode, bool rewrite); static void TryReuseIndex(Oid oldId, IndexStmt *stmt); + static void TryReuseForeignKey(Oid oldId, Constraint *con); static void change_owner_fix_column_acls(Oid relationOid, Oid oldOwnerId, Oid newOwnerId); static void change_owner_recurse_to_sequences(Oid relationOid, *** *** 5696,5701 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5698,5705 numpks; Oid indexOid; Oid constrOid; + boolold_check_ok; + ListCell *old_pfeqop_item = list_head(fkconstraint-old_conpfeqop); /* * Grab an exclusive lock on the pk table, so that someone doesn't delete *** *** 5812,5817 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5816,5828 (errcode(ERRCODE_INVALID_FOREIGN_KEY), errmsg(number of referencing and referenced columns for foreign key disagree))); + /* +* On the strength of a previous constraint, we might avoid scanning +* tables to validate this one. See below. +*/ + old_check_ok = (fkconstraint-old_conpfeqop != NIL); + Assert(!old_check_ok || numfks == list_length(fkconstraint-old_conpfeqop)); + for (i = 0; i numpks; i++) { Oid pktype = pktypoid[i]; *** *** 5826,5831 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5837,5843 Oid ppeqop; Oid ffeqop; int16 eqstrategy; + Oid pfeqop_right; /* We need several fields out of the pg_opclass entry */ cla_ht = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclasses[i])); *** *** 5868,5877 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, pfeqop = get_opfamily_member(opfamily, opcintype, fktyped, eqstrategy); if (OidIsValid(pfeqop)) ffeqop = get_opfamily_member(opfamily, fktyped, fktyped, eqstrategy); else ! ffeqop = InvalidOid;/* keep compiler quiet */ if (!(OidIsValid(pfeqop) OidIsValid(ffeqop))) { --- 5880,5896 pfeqop = get_opfamily_member(opfamily, opcintype, fktyped, eqstrategy); if (OidIsValid(pfeqop)) + { + pfeqop_right = fktyped; ffeqop = get_opfamily_member(opfamily, fktyped, fktyped, eqstrategy); + } else ! { ! /* keep compiler quiet */ ! pfeqop_right = InvalidOid; ! ffeqop = InvalidOid; ! } if (!(OidIsValid(pfeqop) OidIsValid(ffeqop))) { *** *** 5893,5899 ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, --- 5912,5921 target_typeids[1] = opcintype; if (can_coerce_type(2, input_typeids, target_typeids, COERCION_IMPLICIT)) + {