On Mon, Sep 2, 2019 at 6:31 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > I wrote: > > As far as 4) goes, I think the code in ExtractReplicaIdentity is pretty > > duff anyway, because it doesn't bother to check for the defined failure > > return for RelationIdGetRelation. But if we're concerned about the > > cost of recalculating this stuff per-row, couldn't we cache it a little > > better? It should be safe to assume the set of index columns isn't > > changing intra-query. > > ... in fact, isn't all the infrastructure for that present already? > > Why is this code looking directly at the index at all, rather than > > using the relcache's rd_idattr bitmap? > > Here's a proposed patch along those lines. It fixes Hadi's original > crash case and passes check-world.
Agree that this patch would be a better solution for Hadi's report, although I also agree that the situation with index locking for DELETE isn't perfect. > I'm a bit suspicious of the exclusion for idattrs being empty, but > if I remove that, some of the contrib/test_decoding test results > change. Anybody want to comment on that? If that's actually an > expected situation, why is there an elog(DEBUG) in that path? ISTM that the exclusion case may occur with the table's replica identity being REPLICA_IDENTITY_DEFAULT and there being no primary index defined, in which case nothing needs to get logged. The elog(DEBUG) may just be a remnant from the days when this was being developed. I couldn't find any notes on it though in the archives [1] though. Thanks, Amit [1] https://www.postgresql.org/message-id/flat/20131204155510.GO24801%40awork2.anarazel.de
