On 2019/04/25 19:02, Amit Langote wrote:
> On 2019/04/25 11:21, Amit Langote wrote:
>> On 2019/04/25 8:27, Tom Lane wrote:
>>> BTW, I hadn't ever looked closely at what the index reuse code
>>> does, and now that I have, my heart just sinks. I think that
>>> logic needs to be nuked from orbit. RelationPreserveStorage was
>>> never meant to be abused in this way; I invite you to contemplate
>>> whether it's not a problem that it doesn't check the backend and
>>> nestLevel fields of PendingRelDelete entries before killing them.
>>> (In its originally-designed use for mapped rels at transaction end,
>>> that wasn't a problem, but I'm afraid that it may be one now.)
>>>
>>> The right way to do this IMO would be something closer to the
>>> heap-swap logic in cluster.c, where we exchange the relfilenodes
>>> of two live indexes, rather than what is happening now. Or for
>>> that matter, do we really need to delete the old indexes at all?
>>
>> Yeah, we wouldn't need TryReuseIndex and subsequent
>> RelationPreserveStorage if we hadn't dropped the old indexes to begin
>> with. Instead, in ATPostAlterTypeParse, check if the index after ALTER
>> TYPE is still incompatible (CheckIndexCompatible) and if it is, don't add
>> a new AT_ReAddIndex command. If it's not, *then* drop the index, and
>> recreate the index from scratch using an IndexStmt generated from the old
>> index definition. I guess We can get rid of IndexStmt.oldNode too.
>
> Thinking on this more and growing confident that we could indeed avoid
> drop index + recreate-it-while-preserving-storage, instead by just not
> doing anything when CheckIndexCompatible says the old index will be fine
> despite ALTER TYPE, but only if the table is not rewritten. I gave this a
> try and came up with the attached patch. It fixes the bug related to
> partitioned indexes (the originally reported one) and then some.
>
> Basically, I aimed to rewrite the code in ATPostAlterTypeCleanup and
> ATPostAlterTypeParse such that we no longer have to rely on an
> implementation based on setting "oldNode" to preserve old indexes. With
> the attached, for the cases in which the table won't be rewritten and
> hence the indexes not rebuilt, ATPostAlterTypeParse() simply won't queue a
> AT_ReAddIndex command to rebuild index while preserving the storage. That
> means both ATAddIndex() and DefineIndex can be freed of the duty of
> looking out for the "oldNode" case, because that case no longer exists.
>
> Another main change is that inherited (!conislocal) constraints are now
> recognized by ATPostAlterTypeParse directly, instead of
> ATPostAlterTypeCleanup checking for them and skipping
> ATPostAlterTypeParse() as a whole for such constraints. For one, I had to
> make that change to make the above-described approach work. Also, doing
> that allowed to fix another bug whereby the comments of child constraints
> would go away when they're reconstructed. Notice what happens on
> un-patched PG 11:
>
> create table pp (a int, b text, unique (a, b), c varchar(64)) partition by
> list (a);
> create table pp1 partition of pp for values in (1);
> create table pp2 partition of pp for values in (2);
> alter table pp add constraint c_chk check (c <> '');
> comment on constraint c_chk ON pp is 'parent check constraint';
> comment on constraint c_chk ON pp1 is 'child check constraint 1';
> comment on constraint c_chk ON pp2 is 'child check constraint 2';
> select conname, obj_description(oid, 'pg_constraint') from pg_constraint
> where conname = 'c_chk';
> conname │ obj_description
> ─────────┼──────────────────────────
> c_chk │ parent check constraint
> c_chk │ child check constraint 1
> c_chk │ child check constraint 2
> (3 rows)
>
> alter table pp alter c type varchar(64);
>
> select conname, obj_description(oid, 'pg_constraint') from pg_constraint
> where conname = 'c_chk';
> conname │ obj_description
> ─────────┼─────────────────────────
> c_chk │ parent check constraint
> c_chk │
> c_chk │
> (3 rows)
>
> The patch fixes that with some surgery of RebuildConstraintComment
> combined with aforementioned changes. With the patch:
>
> alter table pp alter c type varchar(64);
>
> select conname, obj_description(oid, 'pg_constraint') from pg_constraint
> where conname = 'c_chk';
> conname │ obj_description
> ─────────┼──────────────────────────
> c_chk │ parent check constraint
> c_chk │ child check constraint 1
> c_chk │ child check constraint 2
> (3 rows)
>
> alter table pp alter c type varchar(128);
>
> select conname, obj_description(oid, 'pg_constraint') from pg_constraint
> where conname = 'c_chk';
> conname │ obj_description
> ─────────┼──────────────────────────
> c_chk │ parent check constraint
> c_chk │ child check constraint 1
> c_chk │ child check constraint 2
> (3 rows)
>
> Also for index comments, but only in the case when indexes are not rebuilt.
>
> comment on index pp_a_b_key is 'parent index';
> comment on index pp1_a_b_key is 'child index 1';
> comment on index pp2_a_b_key is 'child index 2';
>
> select relname, relfilenode, obj_description(oid,'pg_class') from pg_class
> where relname like 'pp%key';
> relname │ relfilenode │ obj_description
> ─────────────┼─────────────┼─────────────────
> pp1_a_b_key │ 17280 │ child index 1
> pp2_a_b_key │ 17284 │ child index 2
> pp_a_b_key │ 17271 │ parent index
> (3 rows)
>
> -- no rewrite, indexes untouched, comments preserved
> alter table pp alter b type varchar(128);
>
> select relname, relfilenode, obj_description(oid,'pg_class') from pg_class
> where relname like 'pp%key';
> relname │ relfilenode │ obj_description
> ─────────────┼─────────────┼─────────────────
> pp1_a_b_key │ 17280 │ child index 1
> pp2_a_b_key │ 17284 │ child index 2
> pp_a_b_key │ 17271 │ parent index
> (3 rows)
>
> -- table rewritten, indexes rebuild, child indexes' comments gone
> alter table pp alter b type varchar(64);
>
> select relname, relfilenode, obj_description(oid,'pg_class') from pg_class
> where relname like 'pp%key';
> relname │ relfilenode │ obj_description
> ─────────────┼─────────────┼─────────────────
> pp1_a_b_key │ 17294 │
> pp2_a_b_key │ 17298 │
> pp_a_b_key │ 17285 │ parent index
> (3 rows)
>
>
> I've also added tests for both the originally reported bug and the comment
> ones.
>
> The patch applies to PG 11.
Per Alvaro's report, regression tests added weren't portable. Fixed that
in the attached updated patch.
Thanks,
Amit