Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

Поиск
Список
Период
Сортировка
От Önder Kalacı
Тема Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher
Дата
Msg-id CACawEhUrbDtU7oouZLzBes=+Uyk4x2DTKX8347zpXHa+PSmEjA@mail.gmail.com
обсуждение исходный текст
Ответ на Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher  (Amit Kapila <amit.kapila16@gmail.com>)
Список pgsql-hackers
Hi Amit, all

Few comments:
=============
1.
+get_usable_indexoid(ApplyExecutionData *edata, ResultRelInfo *relinfo)
{
...
+ if (targetrelkind == RELKIND_PARTITIONED_TABLE)
+ {
+ /* Target is a partitioned table, so find relmapentry of the partition */
+ TupleConversionMap *map = ExecGetRootToChildMap(relinfo, edata->estate);
+ AttrMap    *attrmap = map ? map->attrMap : NULL;
+
+ relmapentry =
+ logicalrep_partition_open(relmapentry, relinfo->ri_RelationDesc,
+   attrmap);


When will we hit this part of the code? As per my understanding, for
partitioned tables, we always follow apply_handle_tuple_routing()
which should call logicalrep_partition_open(), and do the necessary
work for us.


Looking closer, there is really no need for that. I changed the
code such that we pass usableLocalIndexOid. It looks simpler
to me. Do you agree?

 
2. In logicalrep_partition_open(), it would be better to set
localrelvalid after finding the required index. The entry should be
marked valid after initializing/updating all the required members. I
have changed this in the attached.


makes sense
 
3.
@@ -31,6 +32,7 @@ typedef struct LogicalRepRelMapEntry
  Relation localrel; /* relcache entry (NULL when closed) */
  AttrMap    *attrmap; /* map of local attributes to remote ones */
  bool updatable; /* Can apply updates/deletes? */
+ Oid usableIndexOid; /* which index to use, or InvalidOid if none */

Would it be better to name this new variable as localindexoid to match
it with the existing variable localreloid? Also, the camel case for
this variable appears odd.

yes, both makes sense
 

4. If you agree with the above, then we should probably change the
name of functions get_usable_indexoid() and
FindLogicalRepUsableIndex() accordingly.

I dropped get_usable_indexoid() as noted in (1).

Changed FindLogicalRepUsableIndex->FindLogicalRepLocalIndex



5.
+ {
+ /*
+ * If we had a primary key or relation identity with a unique index,
+ * we would have already found and returned that oid. At this point,
+ * the remote relation has replica identity full.

These comments are not required as this just states what the code just
above is doing.

I don't have any strong opinions on adding this comment, applied your
suggestion.
 

Apart from the above, I have made some modifications in the other
comments. If you are convinced with those, then kindly include them in
the next version.


Sure, they all look good. I think I have lost (and caused the reviewers to lose)
quite a bit of time on the comment reviews. Next time, I'll try to be more prepared
for the comments.

Attached v34

Thanks,
Onder KALACI
Вложения

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Önder Kalacı
Дата:
Сообщение: Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher
Следующее
От: Maxim Orlov
Дата:
Сообщение: Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)