Re: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL
От | Peter Smith |
---|---|
Тема | Re: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL |
Дата | |
Msg-id | CAHut+PuzhADSevvypRikx4mtT_1B6_14Z1iCcfhEYVM5EsgGGQ@mail.gmail.com обсуждение исходный текст |
Ответ на | RE: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL ("Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>) |
Список | pgsql-hackers |
Here are some review comments for the v5 patch ====== Commit message. 1. 89e46d allowed using indexes other than PRIMARY KEY or REPLICA IDENTITY on the subscriber, but only the BTree index could be used. This commit extends the limitation, now the Hash index can be also used. These 2 types of indexes are the only ones supported because only these - use fix strategy numbers - implement the "equality" strategy - supported by tuples_equal() - implement the function amgettuple() ~ 1a. /fix/fixed/ ~ 1b. /supported by tuples_equal()/are supported by tuples_equal()/ ~~~ 2. Index access methods other than them don't have a fixed strategy for equality operation. Note that in other index access methods, the support routines of each operator class interpret the strategy numbers according to the operator class's definition. In build_replindex_scan_key(), the operator which corresponds to "equality comparison" is searched by using the strategy number, but it is difficult for such indexes. ~ /Index access methods other than them/Other index access methods/ ~~~ 3. tuples_equal() cannot accept a datatype that does not have an operator class for Btree or Hash. One motivation for other types of indexes to be used is to define an index to attributes such as datatypes like point and box. But lookup_type_cache(), which is called from tuples_equal(), cannot return the valid value if input datatypes do not have such a opclass. ~ That paragraph looks the same as the code comment in IsIndexUsableForReplicaIdentityFull(). I wrote some review comments about that (see #5d below) so the same maybe applies here too. ====== src/backend/executor/execReplication.c 4. +/* + * Return the strategy number which corresponds to the equality operator for + * given index access method. + * + * XXX: Currently, only Btree and Hash indexes are supported. This is because + * index access methods other than them don't have a fixed strategy for + * equality operation. Note that in other index access methods, the support + * routines of each operator class interpret the strategy numbers according to + * the operator class's definition. So, we return the InvalidStrategy number + * for index access methods other than BTREE and HASH. + */ +int +get_equal_strategy_number_for_am(Oid am) The function comment seems a bit long. Maybe it can be simplified a bit: SUGGESTION Return the strategy number which corresponds to the equality operator for the given index access method, otherwise, return InvalidStrategy. XXX: Currently, only Btree and Hash indexes are supported. The other index access methods don't have a fixed strategy for equality operation - instead, the support routines of each operator class interpret the strategy numbers according to the operator class's definition. ====== src/backend/replication/logical/relation.c 5. FindUsableIndexForReplicaIdentityFull /* * Returns true if the index is usable for replica identity full. For details, * see FindUsableIndexForReplicaIdentityFull. + * + * XXX: Currently, only Btree and Hash indexes can be returned as usable one. + * This is because mainly two reasons: + * + * 1) Other index access methods other than Btree and Hash don't have a fixed + * strategy for equality operation. Refer comments atop + * get_equal_strategy_number_for_am. + * 2) tuples_equal cannot accept a datatype that does not have an operator + * class for Btree or Hash. One motivation for other types of indexes to be + * used is to define an index to attributes such as datatypes like point and + * box. But lookup_type_cache, which is called from tuples_equal, cannot return + * the valid value if input datatypes do not have such a opclass. + * + * Furthermore, BRIN and GIN indexes do not implement "amgettuple". + * RelationFindReplTupleByIndex assumes that tuples will be fetched one by + * one via index_getnext_slot, but this currently requires the "amgettuple" + * function. */ 5a. /as usable one./as useable./ ~ 5b. /Other index access methods other than Btree and Hash/Other index access methods/ ~ 5c. Maybe a blank line before 2) will help readability. ~ 5d. "One motivation for other types of indexes to be used is to define an index to attributes such as datatypes like point and box." Isn't it enough to omit that sentence and just say: SUGGESTION 2) tuples_equal() cannot accept a datatype (e.g. point or box) that does not have an operator class for Btree or Hash. This is because lookup_type_cache(), which is called from tuples_equal(), cannot return the valid value if input datatypes do not have such an opclass. ~~~ 6. FindUsableIndexForReplicaIdentityFull + /* Check whether the index is supported or not */ + if (get_equal_strategy_number_for_am(indexInfo->ii_Am) == InvalidStrategy) + return false; 6a. Really, this entire function is for checking "is supported or not" so IMO this is not the correct comment just for this statement. BTW, I noted Onder suggested keeping this as a variable assignment (called 'has_equal_strategy') [1]. I also think having a variable is better because then this extra comment would be unnecessary. ~ 6b. IMO the code is readable with the early exit, but it is fine also if you want to revert it to how Onder suggested [1]. I think it is not worth worrying too much here because it seems the Sawada-san patch [2] might have intentions to refactor all this same function anyhow. ------ [1] Onder suggestion - https://www.postgresql.org/message-id/CACawEhXvVqxoaqj5aanaT02DHYUJwpkssS4RTZRSuqEOpT0zQg%40mail.gmail.com [2] Sawada-san other thread - https://www.postgresql.org/message-id/CAD21AoAKx%2BFY4OPPj%2BMEF0gM-TAV0%3Dfd3EfPoEsa%2BcMQLiWjyA%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia
В списке pgsql-hackers по дате отправления:
Следующее
От: Peter SmithДата:
Сообщение: Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.