Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher
От | Peter Smith |
---|---|
Тема | Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher |
Дата | |
Msg-id | CAHut+PuHkYcg=BX1nBNNUxU0eRaQj+Xh6LaowA937W9MvuDACw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher (Önder Kalacı <onderkalaci@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 |
On Mon, Mar 6, 2023 at 10:18 PM Önder Kalacı <onderkalaci@gmail.com> wrote: > >> 4. IdxIsRelationIdentityOrPK >> >> +/* >> + * Given a relation and OID of an index, returns true if the >> + * index is relation's replica identity index or relation's >> + * primary key's index. >> + * >> + * Returns false otherwise. >> + */ >> +bool >> +IdxIsRelationIdentityOrPK(Relation rel, Oid idxoid) >> +{ >> + Assert(OidIsValid(idxoid)); >> + >> + return GetRelationIdentityOrPK(rel) == idxoid; >> +} >> >> I think you've "simplified" this function in v28 but AFAICT now it has >> a different logic to v27. >> >> PREVIOUSLY it was coded like >> + return RelationGetReplicaIndex(rel) == idxoid || >> + RelationGetPrimaryKeyIndex(rel) == idxoid; >> >> But now in that scenario, it won't >> even check the PK if there was a valid RI. So it might return false >> when previously it returned true. Is it deliberate? >> > > Thanks for detailed review/investigation on this. But, I also agree that > there is no difference in terms of correctness. Also, it is probably better > to be consistent with the existing code. So, making IdxIsRelationIdentityOrPK() > relying on GetRelationIdentityOrPK() still sounds better to me. > >> You can see if 'idxoid' did NOT match RI but if it DID match PK >> previously it would return true. > > > Still, I cannot see how this check would yield a different result with how > RI/PK works -- as Amit also noted in the next e-mail. > > Do you see any cases where this check would produce a different result? > I cannot, but wanted to double check with you. > > Let me give an example to demonstrate why I thought something is fishy here: Imagine rel has a (non-default) REPLICA IDENTITY with Oid=1111. Imagine the same rel has a PRIMARY KEY with Oid=2222. --- +/* + * Get replica identity index or if it is not defined a primary key. + * + * If neither is defined, returns InvalidOid + */ +Oid +GetRelationIdentityOrPK(Relation rel) +{ + Oid idxoid; + + idxoid = RelationGetReplicaIndex(rel); + + if (!OidIsValid(idxoid)) + idxoid = RelationGetPrimaryKeyIndex(rel); + + return idxoid; +} + +/* + * Given a relation and OID of an index, returns true if the + * index is relation's replica identity index or relation's + * primary key's index. + * + * Returns false otherwise. + */ +bool +IdxIsRelationIdentityOrPK(Relation rel, Oid idxoid) +{ + Assert(OidIsValid(idxoid)); + + return GetRelationIdentityOrPK(rel) == idxoid; +} + So, according to the above function comment/name, I will expect calling IdxIsRelationIdentityOrPK passing Oid=1111 or Oid-2222 will both return true, right? But AFAICT IdxIsRelationIdentityOrPK(rel, 1111) --> GetRelationIdentityOrPK(rel) returns 1111 (the valid oid of the RI) --> 1111 == 1111 --> true; IdxIsRelationIdentityOrPK(rel, 2222) --> GetRelationIdentityOrPK(rel) returns 1111 (the valid oid of the RI) --> 1111 == 2222 --> false; ~ Now two people are telling me this is OK, but I've been staring at it for too long and I just don't see how it can be. (??) ------ Kind Regards, Peter Smith. Fujitsu Australia
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Alexander KukushkinДата:
Сообщение: Re: pg_rewind: Skip log directory for file type check like pg_wal
Следующее
От: Soumyadeep ChakrabortyДата:
Сообщение: Re: pg_rewind: Skip log directory for file type check like pg_wal