Re: 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

I neglected to commit after revising the text of a few comments; use this
version instead. Thanks.

On Wed, Jan 4, 2012 at 12:35 PM, Noah Misch <noah(at)leadboat(dot)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(at)postgresql(dot)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

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.

On Sat, Jan 21, 2012 at 9:05 PM, Noah Misch <noah(at)leadboat(dot)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

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 <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

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

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 <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support