Hi Hayato, all
> > - return is_btree && !is_partial && !is_only_on_expression; > > + /* Check whether the index is supported or not */ > > + is_suitable_type = ((get_equal_strategy_number_for_am(indexInfo->ii_Am) > > + != InvalidStrategy)); > > + > > + is_partial = (indexInfo->ii_Predicate != NIL); > > + is_only_on_expression = IsIndexOnlyOnExpression(indexInfo); > > + > > + return is_suitable_type && !is_partial && !is_only_on_expression; > > > I don't want to repeat this too much, as it is a minor note. Just sharing my perspective here. As discussed in the other email [1], I feel like keeping IsIndexUsableForReplicaIdentityFull() function readable is useful for documentation purposes as well. So, I'm more inclined to see something like your old code, maybe with a different variable name. bool is_btree = (indexInfo->ii_Am == BTREE_AM_OID); to > bool has_equal_strategy = get_equal_strategy_number_for_am... > .... > return has_equal_strategy && !is_partial && !is_only_on_expression; > 4a. > > The code can be written more optimally, because if it is deemed > > already that 'is_suitable_type' is false, then there is no point to > > continue to do the other checks and function calls. Sure, there are maybe few cycles of CPU gains, but this code is executed infrequently, and I don't see much value optimizing it. I think keeping it slightly more readable is nicer. Other than that, I think the code/test looks good. For the comments/documentation, I think Amit and Peter have already given quite a bit of useful feedback, so nothing much to add from my end. Thanks, Onder [1]: https://www.postgresql.org/message-id/CACawEhUWH1qAZ8QNeCve737Qe1_ye%3DvTW9P22ePiFssT7%2BHaaQ%40mail.gmail.com Hayato Kuroda (Fujitsu) <kuroda.hay...@fujitsu.com>, 12 Tem 2023 Çar, 10:07 tarihinde şunu yazdı: > Dear Amit, > > Thanks for checking my patch! The patch can be available at [1]. > > > > > ====== > > > > .../subscription/t/032_subscribe_use_index.pl > > > > > > > > 11. > > > > AFAIK there is no test to verify that the leftmost index field must > be > > > > a column (e.g. cannot be an expression). Yes, there are some tests > for > > > > *only* expressions like (expr, expr, expr), but IMO there should be > > > > another test for an index like (expr, expr, column) which should also > > > > fail because the column is not the leftmost field. > > > > > > I'm not sure they should be fixed in the patch. You have raised these > points > > > at the Sawada-san's thread[1] and not sure he has done. > > > Furthermore, these points are not related with our initial motivation. > > > Fork, or at least it should be done by another patch. > > > > > > > I think it is reasonable to discuss the existing code-related > > improvements in a separate thread rather than trying to change this > > patch. > > OK, so I will not touch in this thread. > > > I have some other comments about your patch: > > > > 1. > > + * 1) Other indexes do not have a fixed set of strategy numbers. > > + * 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. For example, GiST index operator classes for > > + * two-dimensional geometric objects have a comparison "same", but its > > strategy > > + * number is different from the gist_int4_ops, which is a GiST index > operator > > + * class that implements the B-tree equivalent. > > + * > > ... > > + */ > > +int > > +get_equal_strategy_number_for_am(Oid am) > > ... > > > > I think this comment is slightly difficult to understand. Can we make > > it a bit generic by saying something like: "The index access methods > > other than BTREE and HASH 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 > > InvalidStrategy number for index access methods other than BTREE and > > HASH." > > Sounds better. Fixed with some adjustments. > > > 2. > > + * 2) Moreover, 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 > "amgetuple" > > + * function. To make it use index_getbitmap() must be used, which > fetches all > > + * the tuples at once. > > + */ > > +int > > +get_equal_strategy_number_for_am(Oid am) > > { > > .. > > > > I don't think this is a good place for such a comment. We can probably > > move this atop IsIndexUsableForReplicaIdentityFull(). I think you need > > to mention two reasons in IsIndexUsableForReplicaIdentityFull() why we > > support only BTREE and HASH index access methods (a) Refer to comments > > atop get_equal_strategy_number_for_am(); (b) mention the reason > > related to tuples_equal() as discussed in email [1]. Then you can say > > that we also need to ensure that the index access methods that we > > support must have an implementation "amgettuple" as later while > > searching tuple via RelationFindReplTupleByIndex, we need the same. > > Fixed, and based on that I modified the commit message accordingly. > How do you feel? > > > We can probably have an assert for this as well. > > Added. > > [1]: > https://www.postgresql.org/message-id/TYAPR01MB5866B4F938ADD7088379633CF536A%40TYAPR01MB5866.jpnprd01.prod.outlook.com > > Best Regards, > Hayato Kuroda > FUJITSU LIMITED > > > >